Conversation
Absolutize relative executable paths against the shell working directory before spawning external commands. Fixes reubeno#1251
|
@DaZuo0122 Thanks for the contribution (and for finding the issue)! I'd like to find a way to resolve this without making any meaningful behavioral changes to non-Windows code paths. (We can also reuse the path normalization helper crate that we use elsewhere in the project -- that may help to simplify.) Are you open to me collaborating with you on this and refactoring it a bit? |
Performance Benchmark Report
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is Test Summary: bash-completion test suite
|
|
@reubeno Absolutely, and yes, I’m happy to collaborate on this! I agree that the fix should avoid meaningful behavioral changes on non-Windows platforms. I can rework it so the change is Windows-specific, likely gated behind Feel free to let me know what direction you prefer and I’ll update it. |
reubeno
left a comment
There was a problem hiding this comment.
Thanks for being willing to iterate on this! I've got 2 main comments here.
It would also be ideal if you can find a way to test any non-trivial logic that gets added to the Windows code path.
| let normalized_executable = if Path::new(executable_path).is_relative() { | ||
| let joined = context.shell.working_dir().join(executable_path); | ||
|
|
||
| let mut normalized = PathBuf::new(); |
There was a problem hiding this comment.
request(blocking): We already use the normalize_path crate elsewhere in this project. Could you look at using that instead of hand-normalizing?
| .is_some_and(|f| f.is_terminal()); | ||
|
|
||
| // On Windows, normalize relative executable paths before spawning. | ||
| #[cfg(windows)] |
There was a problem hiding this comment.
request(blocking): While we have some still sprinkled around, our strong preference is for platform-specific code to be invoked via the sys module in the crate. For non-Windows platforms, it should ideally be a no-op/pass-through that doesn't impact the code path; for Windows, it can perform this additional manipulation.
Don't hesitate to holler or ask questions if you could use additional pointers/examples on how best to do this.
Fixes #1251.
On Windows, relative external command paths were resolved against the
brush process's OS current directory instead of the shell's working
directory. This caused commands such as:
cd target ./release/brush.exe --versionto fail with
ERROR_PATH_NOT_FOUND, even though the executable existedrelative to the shell's current working directory.
Fix
Resolve relative executable paths against
context.shell.working_dir()before composing the
std::process::Command.Absolute paths and
PATH-resolved commands continue to work.p.s. I have added
#[allow(clippy::too_many_lines)]toexecute_external_commandto pass thecargo xtask ci pre-commitcheck.