Skip to content
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

windows: Fix rust tasks #13413

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

JunkuiZhang
Copy link
Contributor

Recording.2024-06-23.164516.mp4

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jun 23, 2024
for package in metadata.packages {
for target in package.targets {
let is_bin = target.kind.iter().any(|kind| kind == "bin");
if target.src_path == abs_path && is_bin {
Copy link
Contributor Author

@JunkuiZhang JunkuiZhang Jun 23, 2024

Choose a reason for hiding this comment

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

On windows, the target.src_path could be C:\\A\\B\\main.rs, and abs_path could be C:\\A\\B/main.rs, so a simple string comparing may always return false.

command.push_str(&arg);
#[cfg(target_os = "windows")]
command.push_str(&task::to_powershell_variable(arg));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PowerShell uses syntax like $env:SOME_VAR instead of the Unix shell's $SOME_VAR.

Comment on lines 278 to 295
pub fn to_powershell_variable(input: String) -> String {
if let Some(var_str) = input.strip_prefix("${") {
if var_str.find(':').is_none() {
// If the input starts with "${", remove the trailing "}"
format!("$env:{}", &var_str[..var_str.len() - 1])
} else {
// `${SOME_VAR:-SOME_DEFAULT}`, we currently do not handle this situation,
// which will result in the task failing to run in such cases.
input
}
} else if let Some(var_str) = input.strip_prefix('$') {
// If the input starts with "$", directly append to "$env:"
format!("$env:{}", var_str)
} else {
// If no prefix is found, return the input as is
input
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, we only handle $SOME_VAR and ${SOME_VAR}, but not default parameters like ${SOME_VAR:-SOME_DEFAULT}. Handling such cases in PowerShell is pretty complex. In lower versions of PowerShell, you would use if-else statements, while in higher versions, you can use a ternary-like syntax. See here.

@osiewicz
Copy link
Contributor

Just a note: ideally we shouldn't make adjustments for specific shells in tasks, as a better solution would be to have a task specify a shell that it should be spawned with. Then, if we could somehow ensure a presence of Unix-esque shell on Windows, we could just use that to spawn tasks.

I'm not opposed to this PR per se, as we still may need windows-specific workarounds (around e.g. parsing paths from cargo metadata output and whatnot), but ideally we'd keep it to minimum.

@JunkuiZhang
Copy link
Contributor Author

JunkuiZhang commented Jun 23, 2024

Just a note: ideally we shouldn't make adjustments for specific shells in tasks, as a better solution would be to have a task specify a shell that it should be spawned with. Then, if we could somehow ensure a presence of Unix-esque shell on Windows, we could just use that to spawn tasks.

I'm not opposed to this PR per se, as we still may need windows-specific workarounds (around e.g. parsing paths from cargo metadata output and whatnot), but ideally we'd keep it to minimum.

This is Microsoft. Windows does not have an out-of-the-box Unix-like shell. Users can install Cygwin or MSYS/MinGW, which consist of libraries with Linux syscalls mapped to the Windows kernel, to get a unix-like shell working on Windows. Windows comes with cmd and PowerShell, each with its own syntax ($SOME_VAR on Unix, $env:SOME_VAR on PowerShell, %SOME_VAR% on cmd). VSCode defaults to launching PowerShell, so I suppose this change might be inevitable.

I guess this PR might be in a state where it has minimal impact, any suggestions are welcome. To implement shell override, some special handling may be required.

Comment on lines +279 to +295
pub fn to_windows_variable(shell: &str, input: String) -> String {
match shell {
"powershell" => to_powershell_variable(input),
"cmd" => to_cmd_variable(input),
some_shell => {
if some_shell.ends_with("powershell.exe") {
to_powershell_variable(input)
} else if some_shell.ends_with("cmd.exe") {
to_cmd_variable(input)
} else {
// Someother shell detected, the user might install and use a
// unix-like shell.
input
}
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might make this more general.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants