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.

Sign up for GitHub

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

Fix npm run on Windows #223

Merged
merged 1 commit into from
Feb 19, 2024
Merged

Fix npm run on Windows #223

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions cranelift/filetests/src/zkasm_runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,24 @@ pub fn run_zkasm_path(input_path: &Path) -> anyhow::Result<Vec<ExecutionResult>>
)?;

let mut output_file = NamedTempFile::new()?;
let common_args = [
"--prefix",
"../../tests/zkasm",
"test",
input_path.to_str().unwrap(),
output_file.path().to_str().unwrap(),
];

#[cfg(target_os = "windows")]
let output = std::process::Command::new("cmd")
.args(["/C", "npm"])
Comment on lines +86 to +87
Copy link

Choose a reason for hiding this comment

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

This PR customized the path to NPM that we use on the Windows platform.

Isn't it the same path, but another way of invoking npm? That's my interpretation based on this post, though I'm not familiar with running commands on Windows.

Copy link
Author

Choose a reason for hiding this comment

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

I'm also confused by this - my understanding so far, is that direct npm invocation does not work because of the different handling of PATH variable on Windows.
Adding cmd here is supposed to add another layer of indirection that allows the GitHub action runner to inject the correct PATH variable (which is also defined in GITHUB_PATH workflow variable).

I struggle to find any decent documentation about this PATH behaviour, but FWIW, the example from Rust docs uses cmd on Windows: https://doc.rust-lang.org/nightly/std/process/struct.Command.html

Copy link
Collaborator

@nagisa nagisa Feb 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't necessarily disagree with the fix, but beware that invoking through cmd like this can change the interpretation of the arguments. This can lead to some extremely frustrating to debug problems later down the line.

If it does work currently, then its fine, of course, especially since nobody out of us seriously uses windows for development and thus GHA is the only thing that needs to work. Using the which crate as suggested in the referenced PR sounds like it may be a straightforward improvement in the reliability aspect (though with its own tradeoffs -- adding a dependency potentially means more difficult merges down the line.)

Adding cmd here is supposed to add another layer of indirection that allows the GitHub action runner to inject the correct PATH variable (which is also defined in GITHUB_PATH workflow variable).

FWIW the rust test runner is already invoked by cmd so the %PATH% should be already correct.

.args(common_args)
.output()?;
#[cfg(not(target_os = "windows"))]
let output = std::process::Command::new("npm")
.args([
"--prefix",
"../../tests/zkasm",
"test",
input_path.to_str().unwrap(),
output_file.path().to_str().unwrap(),
])
.args(common_args)
.output()?;

if !output.status.success() {
return Err(anyhow::anyhow!(
"Failed to run `npm`: {}; stderr: {}",
Expand Down