fix(nodeenv): install npm from the registry tarball on Windows - #425
Merged
Merged
Conversation
--with-npm takes install_npm_win on Windows, a path the ubuntu integration job never reaches, so a broken npm there went unnoticed (#310). The new job builds an environment with the command from the issue (--node=17.4.0 --npm=8.3.1) and with the default --npm=latest, then runs activate.bat and `npm install` in a project, as the reporter did.
install_npm_win downloaded github.com/npm/cli/archive/v<ver>.zip, the source repository of npm rather than the published package. That tree is not an installable npm: - npm 8.x links its workspaces into node_modules with symlinks, and zipfile.extractall writes them as text files holding the link target, so npm dies with "Unexpected token '.'" on node_modules/libnpmfund - npm >= 9 has no workspace packages in node_modules at all, so even `npm --version` fails with "Cannot find module '@npmcli/config'" - the default --npm=latest asked for archive/vlatest.zip, a 404 Resolve the version or dist-tag through registry.npmjs.org and unpack the tarball its metadata points to, with filter='data' as for the node archive. The tarball carries bin/npm too, so the Cygwin branch copies it instead of fetching it from raw.githubusercontent.com. The mock-based TestInstallNpmWin tests pinned the GitHub URL and zipfile calls; they now run the real unpacking against a fake registry. Fixes #310
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #310
What happens
install_npm_win, which--with-npmuses on Windows and Cygwin, downloadedgithub.com/npm/cli/archive/v<ver>.zip: the source repository of npm, not the published package. That tree is not an installable npm:node_moduleswith symlinks. A GitHub zip stores a symlink as a file holding its target, andzipfile.extractallwrites it out as such, sonode_modules/libnpmfundbecomes a text file containing../workspaces/libnpmfundand npm dies withUnexpected token '.', the error from the issue.node_modulesat all, so evennpm --versionfails withCannot find module '@npmcli/config'.--npm=latestasked forarchive/vlatest.zip, which is a 404.Unpacked the way
install_npm_windid, npm 8.3.1, 8.19.4, 9.9.4, 10.9.9, 11.20.0 and 12.1.0 are all broken, while the registry tarball of each one works.What changes
registry.npmjs.org/npm/<spec>, and the tarball itsdist.tarballpoints to is unpacked, withfilter='data'on Python >= 3.12 as for the node archive.bin/npm, so the Cygwin branch copies it instead of downloading it fromraw.githubusercontent.com.Tests
TestInstallNpmWinpinned the old implementation with mocks (the GitHub URL, thezipfilecalls,cli-<ver>). It now serves a fake registry with a real.tgzand checks the files that end up in the environment: the published package for a version and forlatest, a reinstall over an existing npm, the Cygwinbin/npm, and the refusal of a member pointing out of the unpack directory.test_smoke_with_npm_winbuilds an environment with the command from the issue (--node=17.4.0 --npm=8.3.1) and with the default--npm=latest, runsactivate.bat, thennpm installin a project, and checks that the npm that answered is the one in the environment. It runs in a newwith-npm-windowsjob, since the ubuntu integration job never takesinstall_npm_win.The test commit was pushed on its own first:
with-npm-windowsfailed withnpm ERR! Unexpected token '.'for the issue case and with thevlatest.zip404 for the default one, while the other 19 jobs passed. With the fix all 20 jobs are green.