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

Fix npm run on Windows #223

Merged
merged 1 commit into from
Feb 19, 2024
Merged

Fix npm run on Windows #223

merged 1 commit into from
Feb 19, 2024

Conversation

aborg-dev
Copy link

@aborg-dev aborg-dev commented Feb 15, 2024

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

It is inspired by https://github.com/oscartbeaumont/rspc/blob/03240ddca56dc5a473a8652296f648c2e1d3987b/crates/create-rspc-app/src/post_gen.rs#L48

@aborg-dev aborg-dev marked this pull request as ready for review February 15, 2024 14:22
@aborg-dev aborg-dev force-pushed the debug_windows_ci branch 3 times, most recently from 0b00ebe to 4dc1eab Compare February 16, 2024 10:41
Comment on lines +86 to +87
let output = std::process::Command::new("cmd")
.args(["/C", "npm"])
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

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.

Copy link

@mooori mooori left a comment

Choose a reason for hiding this comment

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

Thanks for the references! Given that standard lib docs and other popular projects do the same, I suppose it is the right thing to do.

@aborg-dev aborg-dev added this pull request to the merge queue Feb 19, 2024
Merged via the queue into main with commit 8da9e89 Feb 19, 2024
22 checks passed
@aborg-dev aborg-dev deleted the debug_windows_ci branch February 19, 2024 13:10
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