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

Get correct npm prefix on all Windows unix shells #2789

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Feb 26, 2021

Get correct npm prefix on all Windows unix shells

  1. Set the shebang to /usr/bin/env bash instead of /bin/sh (which might
    be dash or some other shell)
  2. Use Unix-style line endings, not Windows-style (Cygwin accepts
    either, but mingw bash sometimes objects, and WSL bash always does)
  3. Test against paths using wslpath if available, but still pass win32
    paths to node.exe, since it is a Windows binary that only knows how
    to handle Windows paths.

This makes npm as installed by the Node.js Windows MSI installer behave
properly under WSL, Cygwin, MINGW Git Bash, and the internal MINGW Git
Bash when posix CLI utilities are exposed to the cmd.exe shell.

The test is not quite as comprehensive as I'd like. It runs on the
various Windows bash implementations if they are found in their expected
locations, skipping any that are not installed. Short of shipping
mingw, cygwin, and wsl as test fixtures, I'm not sure how we could do
much better, however. At least, we can use this test to assist debug
and catch issues on Windows machines (ours or users who report
problems).

@isaacs isaacs requested a review from a team as a code owner February 26, 2021 21:49
@isaacs isaacs force-pushed the isaacs/fix-windows-shell-bins branch from 70f2750 to 5029587 Compare February 26, 2021 21:53
@isaacs
Copy link
Contributor Author

isaacs commented Feb 26, 2021

Test failure is WSL complaining about no distro being installed.

Windows Subsystem for
Linux has no installed
distributions.

Distributions can be
installed by visiting the
Microsoft Store:

https://aka.ms/wslstore

Guess I have to check for that along with statting the WSL bash file.

@isaacs isaacs force-pushed the isaacs/fix-windows-shell-bins branch from 5029587 to 284b581 Compare February 26, 2021 22:04
@isaacs isaacs requested review from nlf and wraithgar February 27, 2021 00:45
@isaacs
Copy link
Contributor Author

isaacs commented Feb 27, 2021

One problem with this: running all these bash shells has resulted in an un-deletable folder on my windows machine at test/bin/windows-shims. I'm going to try to clean that up, do not merge until we do so, because the symlink (junction) in that folder makes it really obnoxious when running npm test a second time.

@ruyadorno ruyadorno added the pr: needs tests requires tests before merging label Mar 1, 2021
@ruyadorno
Copy link
Contributor

Adding "Needs Tests" label to signal it's not ready until the clean up is ready to go 😊

@isaacs
Copy link
Contributor Author

isaacs commented Mar 1, 2021

So the locked folder thing, I have no idea why it happened. I can't reproduce it now.

@nlf
Copy link
Contributor

nlf commented Mar 1, 2021

WINDOOOWS

@ruyadorno ruyadorno added Release 7.x work is associated with a specific npm 7 release release: next These items should be addressed in the next release semver:patch semver patch level for changes and removed pr: needs tests requires tests before merging labels Mar 1, 2021
@isaacs
Copy link
Contributor Author

isaacs commented Mar 1, 2021

This is good now. Passes tests without admin privs, npm script shim working in all windows bashes I could find.

1. Set the shebang to /usr/bin/env bash instead of /bin/sh (which might
   be dash or some other shell)
2. Use Unix-style line endings, not Windows-style (Cygwin accepts
   either, but mingw bash sometimes objects, and WSL bash always does)
3. Test against paths using wslpath if available, but still pass win32
   paths to node.exe, since it is a Windows binary that only knows how
   to handle Windows paths.

This makes npm as installed by the Node.js Windows MSI installer behave
properly under WSL, Cygwin, MINGW Git Bash, and the internal MINGW Git
Bash when posix CLI utilities are exposed to the cmd.exe shell.

The test is not quite as comprehensive as I'd like.  It runs on the
various Windows bash implementations if they are found in their expected
locations, skipping any that are not installed.  Short of shipping
mingw, cygwin, and wsl as test fixtures, I'm not sure how we could do
much better, however.  At least, we can use this test to assist debug
and catch issues on Windows machines (ours or users who report
problems).

PR-URL: #2789
Credit: @isaacs
Close: #2789
Reviewed-by: @nlf
This was referenced Mar 12, 2021
@nlf nlf deleted the isaacs/fix-windows-shell-bins branch March 28, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants