-
Notifications
You must be signed in to change notification settings - Fork 5.5k
util: Fix shell kind in windows based on program path #39696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
util: Fix shell kind in windows based on program path #39696
Conversation
Signed-off-by: Marco Mihai Condrache <[email protected]>
crates/util/src/shell.rs
Outdated
let program = program.as_ref(); | ||
let Some(program) = program.file_stem().and_then(|s| s.to_str()) else { | ||
return if cfg!(windows) { | ||
return if cfg!(windows) && program.to_string_lossy().contains("\\") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering, should we even bother with early return here? Let's suppose that program.file_stem() == None
. Then, we could unwrap into program
itself and do matching that follows. Something like this:
let program = program.file_stem().unwrap_or_else(|| program.as_os_str()).to_str();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kubkon, that's a good idea. Conversion to a string doesn't work without passing through an option or result, so in this case, only to_string_lossy
would work. I don't think this is a problem since we are only retrieving the filename, which would simply fallback
crates/util/src/shell.rs
Outdated
ShellKind::Posix | ||
} else { | ||
if cfg!(windows) { | ||
if cfg!(windows) && program.contains("\\") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why you think this check is valid? What does the program
path look like when executing a command on the remote server while host is WIndows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zed/crates/project/src/terminals.rs
Line 123 in d04ac86
let shell_kind = ShellKind::new(&shell); |
The input of the shell kind struct it's a shell program, that on remote clients like WSL are unix programs.
Take as example the command of the original issue:
/usr/bin/zsh -C 'pwd'
If our host is Windows then we should fallback to PowerShell construction only if the program it's a windows path style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do this in a more reliable way by passing an is_windows: bool
argument, which we can get off the project in callers by project.path_style(cx).is_windows()
, or client.path_style().is_windows()
if we have a RemoteClient
in the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cole-miller, that's true. I've added the argument and updated ShellBuilder, along with all its usages, to pass this argument. Please double-check the changes!
Signed-off-by: Marco Mihai Condrache <[email protected]>
Signed-off-by: Marco Mihai Condrache <[email protected]>
Signed-off-by: Marco Mihai Condrache <[email protected]>
Closes #39614
The
ShellKind
struct is built on Windows' side, meaning that when connecting to remotes, we fall back to PowerShell construction, even if the shell program we are spawning is a unix program.This broke tasks creation since we are using the shell kind to construct args:
zed/crates/project/src/terminals.rs
Line 149 in d04ac86
In normal terminals this only affected activation scripts (only place where shell kind is used)
I don't have a Windows machine to test it, so I would appreciate any help with testing!
Release Notes: