diff --git a/src/error.rs b/src/error.rs index 6e1106ad71..144c8c5ca3 100644 --- a/src/error.rs +++ b/src/error.rs @@ -94,6 +94,10 @@ pub(crate) enum Error<'src> { ExcessInvocations { invocations: usize, }, + ExecutableNotFound { + name: String, + suggestion: Option>, + }, ExpectedSubmoduleButFoundRecipe { path: String, }, @@ -409,6 +413,12 @@ impl ColorDisplay for Error<'_> { ExcessInvocations { invocations } => { write!(f, "Expected 1 command-line recipe invocation but found {invocations}.")?; }, + ExecutableNotFound { name, suggestion } => { + write!(f, "Could not find executable `{name}` in PATH")?; + if let Some(suggestion) = suggestion { + write!(f, "\n{suggestion}")?; + } + } ExpectedSubmoduleButFoundRecipe { path } => { write!(f, "Expected submodule at `{path}` but found recipe.")?; }, diff --git a/src/evaluator.rs b/src/evaluator.rs index e3a7166bab..50231327f3 100644 --- a/src/evaluator.rs +++ b/src/evaluator.rs @@ -267,11 +267,16 @@ impl<'src, 'run> Evaluator<'src, 'run> { } pub(crate) fn run_command(&self, command: &str, args: &[&str]) -> Result { + let working_dir = self.context.working_directory(); let mut cmd = self .context .module .settings - .shell_command(self.context.config); + .shell_command(self.context.config, &working_dir) + .map_err(|error| OutputError::Io(io::Error::new( + io::ErrorKind::NotFound, + format!("{}", error.color_display(Color::never())), + )))?; cmd .arg(command) diff --git a/src/executor.rs b/src/executor.rs index 49c7c89bd0..0b64e76bec 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -15,7 +15,11 @@ impl Executor<'_> { ) -> RunResult<'src, Command> { match self { Self::Command(interpreter) => { - let mut command = Command::new(&interpreter.command.cooked); + // Resolve executable to absolute path using PATH + let working_dir = working_directory.unwrap_or_else(|| Path::new(".")); + let resolved = which::resolve_executable(&interpreter.command.cooked, working_dir)?; + + let mut command = Command::new(resolved); if let Some(working_directory) = working_directory { command.current_dir(working_directory); diff --git a/src/justfile.rs b/src/justfile.rs index 558a6317ed..80fc4a9c75 100644 --- a/src/justfile.rs +++ b/src/justfile.rs @@ -153,7 +153,7 @@ impl<'src> Justfile<'src> { binary, arguments, .. } => { let mut command = if config.shell_command { - let mut command = self.settings.shell_command(config); + let mut command = self.settings.shell_command(config, &search.working_directory)?; command.arg(binary); command } else { diff --git a/src/platform/windows.rs b/src/platform/windows.rs index 13c5f32936..d0742d06f1 100644 --- a/src/platform/windows.rs +++ b/src/platform/windows.rs @@ -27,8 +27,14 @@ impl PlatformInterface for Platform { Cow::Owned(cygpath.output_guard_stdout()?) } else { - // …otherwise use it as-is. - Cow::Borrowed(shebang.interpreter) + // …otherwise resolve using PATH to respect PATH order + let working_dir = working_directory.unwrap_or_else(|| Path::new(".")); + let resolved = which::resolve_executable(shebang.interpreter, working_dir) + .map_err(|error| OutputError::Io(io::Error::new( + io::ErrorKind::NotFound, + format!("{}", error.color_display(Color::never())), + )))?; + Cow::Owned(resolved.to_string_lossy().to_string()) }; let mut cmd = Command::new(command.as_ref()); diff --git a/src/recipe.rs b/src/recipe.rs index 11b8f2edac..f6dae44c75 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -296,10 +296,11 @@ impl<'src, D> Recipe<'src, D> { continue; } - let mut cmd = context.module.settings.shell_command(config); + let working_dir = context.working_directory(); + let mut cmd = context.module.settings.shell_command(config, &working_dir)?; - if let Some(working_directory) = self.working_directory(context) { - cmd.current_dir(working_directory); + if let Some(recipe_working_directory) = self.working_directory(context) { + cmd.current_dir(recipe_working_directory); } cmd.arg(command); diff --git a/src/settings.rs b/src/settings.rs index 1b352d61f6..1d9eeb266b 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -102,14 +102,21 @@ impl<'src> Settings<'src> { settings } - pub(crate) fn shell_command(&self, config: &Config) -> Command { + pub(crate) fn shell_command( + &self, + config: &Config, + working_directory: &Path, + ) -> RunResult<'static, Command> { let (command, args) = self.shell(config); - let mut cmd = Command::new(command); + // Resolve executable to absolute path using PATH + let resolved = which::resolve_executable(command, working_directory)?; + + let mut cmd = Command::new(resolved); cmd.args(args); - cmd + Ok(cmd) } pub(crate) fn shell<'a>(&'a self, config: &'a Config) -> (&'a str, Vec<&'a str>) { diff --git a/src/subcommand.rs b/src/subcommand.rs index 9b15b52044..43453fa42e 100644 --- a/src/subcommand.rs +++ b/src/subcommand.rs @@ -233,7 +233,7 @@ impl Subcommand { let result = justfile .settings - .shell_command(config) + .shell_command(config, &search.working_directory)? .arg(&chooser) .current_dir(&search.working_directory) .stdin(Stdio::piped()) diff --git a/src/which.rs b/src/which.rs index 1a1e4de0a7..2ee8d5fe55 100644 --- a/src/which.rs +++ b/src/which.rs @@ -1,5 +1,83 @@ use super::*; +/// Resolve an executable name to an absolute path using PATH. +/// +/// On Windows, also checks PATHEXT extensions (.exe, .bat, .cmd, etc.) +/// This function respects PATH order, unlike Windows' `CreateProcess` which +/// prioritizes System32. +pub(crate) fn resolve_executable( + name: &str, + working_directory: &Path, +) -> RunResult<'static, PathBuf> { + let name = Path::new(name); + + // If already absolute, validate it exists and return as-is + if name.is_absolute() { + let candidate = name.lexiclean(); + if is_executable::is_executable(&candidate) { + return Ok(candidate); + } + return Err(Error::ExecutableNotFound { + name: name.to_string_lossy().to_string(), + suggestion: None, + }); + } + + // Check if it's a relative path (contains path separators) + let candidates = if name.components().count() > 1 { + // Relative path - resolve relative to working directory + vec![working_directory.join(name)] + } else { + // Simple command name - search PATH + #[allow(unused_mut)] // mut is needed on Windows for PATHEXT extensions + let mut candidates: Vec = env::split_paths( + &env::var_os("PATH") + .ok_or_else(|| Error::internal("PATH environment variable not set"))?, + ) + .map(|path| path.join(name)) + .collect(); + + // On Windows, also try with PATHEXT extensions + #[cfg(windows)] + { + let pathext = env::var_os("PATHEXT") + .unwrap_or_else(|| OsString::from(".EXE;.COM;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH")); + + let extensions: Vec = env::split_paths(&pathext) + .filter_map(|ext| ext.to_str().map(|s| s.to_string())) + .collect(); + + // For each PATH entry, try each extension + for path in env::split_paths(&env::var_os("PATH").unwrap()) { + for ext in &extensions { + candidates.push(path.join(format!("{}{}", name.to_string_lossy(), ext))); + } + } + } + + candidates + }; + + // Try each candidate + for mut candidate in candidates { + if candidate.is_relative() { + candidate = working_directory.join(candidate); + } + + candidate = candidate.lexiclean(); + + if is_executable::is_executable(&candidate) { + return Ok(candidate); + } + } + + // Not found - provide helpful error + Err(Error::ExecutableNotFound { + name: name.to_string_lossy().to_string(), + suggestion: None, + }) +} + pub(crate) fn which(context: function::Context, name: &str) -> Result, String> { let name = Path::new(name); diff --git a/tests/shell.rs b/tests/shell.rs index 22106c0db5..63c9204a03 100644 --- a/tests/shell.rs +++ b/tests/shell.rs @@ -202,3 +202,103 @@ fn backtick_recipe_shell_not_found_error_message() { .status(1) .run(); } + +/// Test that shell resolution respects PATH order +/// This verifies that executables are resolved according to PATH order, +/// not system-specific priority (like System32 on Windows) +#[test] +fn shell_respects_path_order() { + let tmp = tempdir(); + let path = PathBuf::from(tmp.path()); + + // Create a custom shell in a directory that comes before system PATH + let custom_dir = path.join("custom"); + fs::create_dir_all(&custom_dir).unwrap(); + + // Create a wrapper script that uses the real sh but identifies itself + // We'll use a simple approach: create a script that calls sh + let script = if cfg!(windows) { + r#"@echo off +echo CUSTOM_SHELL +sh -c %* +"# + } else { + "#!/bin/sh\necho CUSTOM_SHELL\nsh -c \"$@\"\n" + }; + + Test::with_tempdir(tmp) + .write("custom/sh.exe", script) + .make_executable("custom/sh.exe") + .justfile( + " + set shell := ['sh.exe', '-c'] + + default: + echo hello + ", + ) + .env("PATH", custom_dir.to_str().unwrap()) + .shell(false) + // Should use our custom sh.exe wrapper + .stdout(if cfg!(windows) { + "CUSTOM_SHELL\r\nhello\r\n" + } else { + "CUSTOM_SHELL\nhello\n" + }) + .run(); +} + +/// Test that executable not found gives a clear error message +#[test] +fn shell_executable_not_found_error() { + Test::new() + .justfile( + " + set shell := ['nonexistent-shell-xyz123', '-c'] + + default: + echo hello + ", + ) + .shell(false) + .stderr_regex(r"(?s).*error: Could not find executable `nonexistent-shell-xyz123` in PATH.*") + .status(EXIT_FAILURE) + .run(); +} + +/// Test that absolute paths work for shell setting +#[test] +fn shell_absolute_path() { + let tmp = tempdir(); + let path = PathBuf::from(tmp.path()); + let shell_path = path.join("custom-shell.exe"); + + let script = if cfg!(windows) { + r#"@echo off +echo custom +%* +"# + } else { + "#!/bin/sh\necho custom\nexec sh -c \"$@\"\n" + }; + + Test::with_tempdir(tmp) + .write("custom-shell.exe", script) + .make_executable("custom-shell.exe") + .justfile(format!( + " + set shell := ['{}', '-c'] + + default: + echo hello + ", + shell_path.display() + )) + .shell(false) + .stdout(if cfg!(windows) { + "custom\r\nhello\r\n" + } else { + "custom\nhello\n" + }) + .run(); +} diff --git a/tests/windows_shell.rs b/tests/windows_shell.rs index ca6beaaa58..f0eed30367 100644 --- a/tests/windows_shell.rs +++ b/tests/windows_shell.rs @@ -52,3 +52,135 @@ fn windows_powershell_setting_uses_powershell() { .stderr("Write-Output bar\n") .run(); } + +/// Test that Windows PATH resolution respects PATH order (not System32 priority) +/// This test verifies the fix for issue #2947 +#[test] +#[cfg(windows)] +fn windows_path_resolution_respects_path_order() { + let tmp = tempdir(); + let path = PathBuf::from(tmp.path()); + + // Create a custom shell in a test directory (simulating Git Bash) + let custom_shell_dir = path.join("custom-shell"); + fs::create_dir_all(&custom_shell_dir).unwrap(); + + // Create a simple shell script that echoes its path and executes the command + let shell_script = r#"@echo off +echo CUSTOM_SHELL +%* +"#; + + Test::with_tempdir(tmp) + .write("custom-shell/test-shell.exe", shell_script) + .justfile( + r#" + set shell := ["test-shell.exe", "/c"] + + default: + echo hello + "#, + ) + .env("PATH", custom_shell_dir.to_str().unwrap()) + .shell(false) + // Should use our custom shell, not System32's + .stdout("CUSTOM_SHELL\r\nhello\r\n") + .run(); +} + +/// Test that PATHEXT extensions are checked on Windows +/// This test verifies the fix for issue #2926 +#[test] +#[cfg(windows)] +fn windows_pathext_extensions_checked() { + let tmp = tempdir(); + let path = PathBuf::from(tmp.path()); + + // Create a test executable without .exe extension + let test_dir = path.join("test-dir"); + fs::create_dir_all(&test_dir).unwrap(); + + let script = r#"@echo off +echo found +"#; + + Test::with_tempdir(tmp) + .write("test-dir/testcmd.bat", script) + .justfile( + r#" + set shell := ["testcmd", "/c"] + + default: + echo hello + "#, + ) + .env("PATH", test_dir.to_str().unwrap()) + .shell(false) + .stdout("found\r\nhello\r\n") + .run(); +} + +/// Test that script-interpreter respects PATH order +#[test] +#[cfg(windows)] +fn windows_script_interpreter_path_resolution() { + let tmp = tempdir(); + let path = PathBuf::from(tmp.path()); + + let custom_interpreter_dir = path.join("interpreter"); + fs::create_dir_all(&custom_interpreter_dir).unwrap(); + + // Create an interpreter that echoes and then executes the script file + let interpreter_script = r#"@echo off +echo CUSTOM_INTERPRETER +type %1 +"#; + + Test::with_tempdir(tmp) + .write("interpreter/custom.exe", interpreter_script) + .justfile( + r#" + set unstable + set script-interpreter := ["custom.exe", "/c"] + + [script] + default: + echo hello + "#, + ) + .env("PATH", custom_interpreter_dir.to_str().unwrap()) + .shell(false) + .stdout("CUSTOM_INTERPRETER\r\necho hello\r\n") + .run(); +} + +/// Test that shebang interpreters respect PATH order on Windows +#[test] +#[cfg(windows)] +fn windows_shebang_interpreter_path_resolution() { + let tmp = tempdir(); + let path = PathBuf::from(tmp.path()); + + let custom_interpreter_dir = path.join("interpreter"); + fs::create_dir_all(&custom_interpreter_dir).unwrap(); + + // Create an interpreter that echoes and then executes the script file + let interpreter_script = r#"@echo off +echo SHEBANG_INTERPRETER +type %1 +"#; + + Test::with_tempdir(tmp) + .write("interpreter/python.exe", interpreter_script) + .justfile( + r#" + default: + #!python.exe + echo hello + "#, + ) + .env("PATH", custom_interpreter_dir.to_str().unwrap()) + .shell(false) + .stdout("SHEBANG_INTERPRETER\r\necho hello\r\n") + .run(); +}