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

feat: add ps1 scripts #6548

Merged
merged 2 commits into from
Jun 26, 2023
Merged

feat: add ps1 scripts #6548

merged 2 commits into from
Jun 26, 2023

Conversation

mribbons
Copy link
Contributor

@mribbons mribbons commented Jun 9, 2023

Resolves UNC path issues because powershell.exe
supports UNC, while cmd.exe doesn't.
(Which is why npm.cmd doesn't work within
UNC paths, even under powershell)

Added ps1 versions of npm and npx

Closes #6280

@mribbons mribbons requested a review from a team as a code owner June 9, 2023 11:27
@lukekarrys
Copy link
Contributor

I'd like to get a better idea of how you are currently running these scripts on your system. You mention installing them here $(Get-Command "npm").Source, what directory is that on your machine? How have you previously installed Node on this machine (and npm if that was installed differently)?

This can be merged into npm, but I believe it will require coordination with Node in order for this to land and have it work when installing Node on Windows. Currently the Node Windows installers (of which I know very little about) reference npm.cmd in the locations below. I'm assuming something similar will need to be done with npm.ps1 to have it work as expected in these system.

@mribbons Would you be able to open an issue or PR on https://github.com/nodejs/node to get feedback on how the Windows installers work, and if it would be appropriate for npm to ship these ps1 scripts in our bin/ dir and have them picked up by the installer?

Also in the meantime, would shipping these scripts in the npm bin dir, even if they dont get linked via the Windows installer, fix #6280?

@lukekarrys lukekarrys self-assigned this Jun 15, 2023
@lukekarrys lukekarrys changed the title add ps1 scripts feat: add ps1 scripts Jun 15, 2023
@lukekarrys
Copy link
Contributor

Also this will need to land with some tests in test/bin/windows-shims.js.

@mribbons If you want to attempt that, go for it. Otherwise I will work on those.

Copy link
Contributor

@lukekarrys lukekarrys left a comment

Choose a reason for hiding this comment

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

Requesting changes until we have feedback from Node and tests.

@mribbons
Copy link
Contributor Author

@mribbons If you want to attempt that, go for it. Otherwise I will work on those.

I'll give it a shot!

@mribbons
Copy link
Contributor Author

@lukekarrys

what directory is that on your machine?

C:\Program Files\nodejs\npm.ps1

How have you previously installed Node on this machine

Using Windows Installer from node

Would you be able to open an issue or PR

nodejs/node#48471

Also in the meantime, would shipping these scripts in the npm bin dir, even if they dont get linked via the Windows installer, fix #6280?

My guess is no, the new scripts probably won't be deployed by the installer.

Thanks for your help.

@lukekarrys
Copy link
Contributor

lukekarrys commented Jun 23, 2023

@mribbons I pushed a commit to this PR implementing tests for the powershell scripts you added.

I did make one minor change to the scripts to match the behavior of the other bin scripts, so they now run node npm-cli.js prefix -g to see if there is a symlinked npm that should be used instead.

I'm not sure if all the tests will pass cleanly here since I've been doing some Windows cleanup in #6586 that might need to merge first. But I tested the combination of that PR and this one on my local machine and it worked.

Edit: tests are failing but will be fixed once #6586 and eol normalization lands. Once it does I'll rebase this branch to make sure tests are passing.

@lukekarrys lukekarrys self-requested a review June 23, 2023 04:45
@mribbons
Copy link
Contributor Author

@lukekarrys You are the man, thankyou!

@lukekarrys lukekarrys force-pushed the win-wsl-ps1-fix branch 4 times, most recently from 26a0b81 to 52118a4 Compare June 23, 2023 06:56
@lukekarrys lukekarrys changed the base branch from latest to lk/workspace-symlinks June 23, 2023 23:07
@lukekarrys lukekarrys dismissed their stale review June 23, 2023 23:17

Code has been updated and needs a new reviewer thats not me

@lukekarrys lukekarrys requested review from a team and removed request for lukekarrys June 23, 2023 23:17
@lukekarrys
Copy link
Contributor

This has been rebased to include the changes from #6586 so that tests will pass. That PR will need to be merged first.

lukekarrys added a commit to lukekarrys/node that referenced this pull request Jun 24, 2023
npm has PowerShell scripts that should be installed alongside the cmd
and shell scripts on Windows. The PoweShell scripts should only be placed
if they exist in npm's bin directory.

Closes: nodejs#48471
Ref: npm/cli#6548
@wraithgar wraithgar changed the base branch from lk/workspace-symlinks to latest June 26, 2023 14:18
mribbons and others added 2 commits June 26, 2023 07:25
Resolves UNC path issues because powershell.exe
supports UNC, while cmd.exe doesn't.
(Which is why npm.cmd doesn't work within
UNC paths, even under powershell)

Resolves npm#6280
@wraithgar wraithgar merged commit 89b2741 into npm:latest Jun 26, 2023
@github-actions github-actions bot mentioned this pull request Jun 26, 2023
lukekarrys added a commit to nodejs/node that referenced this pull request Mar 7, 2024
npm 9.8.0 added PowerShell scripts that should be installed alongside
the cmd and shell scripts on Windows.

Closes: #48471
Ref: npm/cli#6548
lukekarrys added a commit to nodejs/node that referenced this pull request Mar 7, 2024
npm 9.8.0 added PowerShell scripts that should be installed alongside
the cmd and shell scripts on Windows.

Fixes: #48471
Refs: npm/cli#6548
nodejs-github-bot pushed a commit to nodejs/node that referenced this pull request Mar 27, 2024
npm 9.8.0 added PowerShell scripts that should be installed alongside
the cmd and shell scripts on Windows.

Fixes: #48471
Refs: npm/cli#6548
PR-URL: #52009
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit to nodejs/node that referenced this pull request May 2, 2024
npm 9.8.0 added PowerShell scripts that should be installed alongside
the cmd and shell scripts on Windows.

Fixes: #48471
Refs: npm/cli#6548
PR-URL: #52009
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit to nodejs/node that referenced this pull request May 3, 2024
npm 9.8.0 added PowerShell scripts that should be installed alongside
the cmd and shell scripts on Windows.

Fixes: #48471
Refs: npm/cli#6548
PR-URL: #52009
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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.

[BUG] using npm in windows on a wsl file mount path causes error
3 participants