Skip to content

Fix windows shell#1057

Open
hossam-houssien wants to merge 4 commits into
alexpasmantier:mainfrom
hossam-houssien:fix-windows-shell
Open

Fix windows shell#1057
hossam-houssien wants to merge 4 commits into
alexpasmantier:mainfrom
hossam-houssien:fix-windows-shell

Conversation

@hossam-houssien

Copy link
Copy Markdown

📺 PR Description

issue #987 and #1004
panic caused by std::process::Command on Windows throwing program not found
the shell names were missing the .exe extension on Windows.
also implemented a default to pwsh.exe if available, gracefully falling back to powershell.exe.

Checklist

@alexpasmantier alexpasmantier left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to fix this. The solution looks fine to me, just left a couple of comments I think we should address before merging.

Comment thread Cargo.lock Outdated
[[package]]
name = "television"
version = "0.15.6"
version = "0.15.6-hos-fix"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you want to include this in the PR

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right , I though i remove it

Comment thread television/utils/shell.rs
Comment on lines +137 to +190
Shell::Bash => {
if cfg!(target_os = "windows") {
"bash.exe"
} else {
"bash"
}
}
Shell::Zsh => {
if cfg!(target_os = "windows") {
"zsh.exe"
} else {
"zsh"
}
}
Shell::Fish => {
if cfg!(target_os = "windows") {
"fish.exe"
} else {
"fish"
}
}
Shell::Cmd => {
if cfg!(target_os = "windows") {
"cmd.exe"
} else {
"cmd"
}
}
Shell::Nu => {
if cfg!(target_os = "windows") {
"nu.exe"
} else {
"nu"
}
}
Shell::Psh => {
if cfg!(target_os = "windows") {
static PSH_EXE: std::sync::OnceLock<&'static str> =
std::sync::OnceLock::new();
PSH_EXE.get_or_init(|| {
if std::process::Command::new("pwsh.exe")
.arg("-Version")
.output()
.is_ok()
{
"pwsh.exe"
} else {
"powershell.exe"
}
})
} else {
"pwsh"
}
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid all that branching, let's just duplicate the executable function and have one for windows and one for unix

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a using the which crate and a match be better here?

Suggested change
Shell::Bash => {
if cfg!(target_os = "windows") {
"bash.exe"
} else {
"bash"
}
}
Shell::Zsh => {
if cfg!(target_os = "windows") {
"zsh.exe"
} else {
"zsh"
}
}
Shell::Fish => {
if cfg!(target_os = "windows") {
"fish.exe"
} else {
"fish"
}
}
Shell::Cmd => {
if cfg!(target_os = "windows") {
"cmd.exe"
} else {
"cmd"
}
}
Shell::Nu => {
if cfg!(target_os = "windows") {
"nu.exe"
} else {
"nu"
}
}
Shell::Psh => {
if cfg!(target_os = "windows") {
static PSH_EXE: std::sync::OnceLock<&'static str> =
std::sync::OnceLock::new();
PSH_EXE.get_or_init(|| {
if std::process::Command::new("pwsh.exe")
.arg("-Version")
.output()
.is_ok()
{
"pwsh.exe"
} else {
"powershell.exe"
}
})
} else {
"pwsh"
}
}
Shell::Bash => "bash",
Shell::Zsh => "zsh",
Shell::Fish => "fish",
Shell::Cmd => "cmd",
Shell::Nu => "nu",
Shell::Psh => {
match which("pwsh") {
Ok(path) => "pwsh",
Err(_) => "powershell.exe",
}
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexpasmantier it still branching anyway, but if using std::process::Command as @HeyItsGilbert mentioned, this would cut the branch,
@HeyItsGilbert std::process::Command wasn't used anywhere, only used in my update, and you're right it dosen't require the .exe extension,however your update still won't work (I guess you're asuming that crate utils::shell uses std::process::Command, it's not)

@HeyItsGilbert

Copy link
Copy Markdown
Contributor

I'm not sure specifying the extension is needed. https://doc.rust-lang.org/std/process/struct.Command.html#platform-specific-behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants