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

build: windows releases should not include npm tests #22901

Closed
NN--- opened this issue Sep 17, 2018 · 6 comments · Fixed by #23001
Closed

build: windows releases should not include npm tests #22901

NN--- opened this issue Sep 17, 2018 · 6 comments · Fixed by #23001
Assignees
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.

Comments

@NN---
Copy link

NN--- commented Sep 17, 2018

Node 10.10 with NPM 6.4.1

Is there any reason why npm has test directory inside ?
It creates a long path for Windows and fails to install:

node_modules\npm\test\npm_cache\content-v2\sha512\76\39\4b378512c68bf209b433e06b71df27a45f7e7be35f174a0f83bce7799628b74dbe993c18b1c12e899a1ed7b159470b382180d1f0a5c4098ac6092cda1a8f

@vsemozhetbyt
Copy link
Contributor

I have the same file from v8-canary install on Windows 7 x64 (the instalation was successful though):
c:\Program Files\nodejs\node_modules\npm\test\npm_cache\content-v2\sha512\76\39\4b378512c68bf209b433e06b71df27a45f7e7be35f174a0f83bce7799628b74dbe993c18b1c12e899a1ed7b159470b382180d1f0a5c4098ac6092cda1a8f

See:
https://github.com/nodejs/node/tree/master/deps/npm/test/npm_cache/content-v2/sha512/76/39
https://github.com/nodejs/node/tree/master/deps/npm/test/npm_cache/_cacache/content-v2/sha512/76/39

cc @nodejs/npm

@targos
Copy link
Member

targos commented Sep 17, 2018

I don't think npm tests are needed to run npm? We could try to remove them from the binary distribution.

@joyeecheung
Copy link
Member

Relavent code is here:

robocopy /e ..\deps\npm node-v%FULLVERSION%-win-%target_arch%\node_modules\npm > nul

@joyeecheung joyeecheung added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Sep 19, 2018
@joyeecheung
Copy link
Member

Hmm, any reason vcbuild.bat does not use tools/install.py? That script does skip the test directory. cc @nodejs/build-files

subdirs[:] = filter('test'.__ne__, subdirs) # skip test suites

@joyeecheung joyeecheung changed the title npm\test build: windows releases should not include npm tests Sep 19, 2018
@richardlau
Copy link
Member

richardlau commented Sep 19, 2018

Well for one thing the install location is different (node_modules\npm on Windows vs lib/node_modules/npm/ via install.py):

target_path = 'lib/node_modules/npm/'

and symlinks created by the following aren't there or used on Windows:

node/tools/install.py

Lines 91 to 107 in a7b59d6

# create/remove symlink
link_path = abspath(install_path, 'bin/npm')
if action == uninstall:
action([link_path], 'bin/npm')
elif action == install:
try_symlink('../lib/node_modules/npm/bin/npm-cli.js', link_path)
else:
assert(0) # unhandled action type
# create/remove symlink
link_path = abspath(install_path, 'bin/npx')
if action == uninstall:
action([link_path], 'bin/npx')
elif action == install:
try_symlink('../lib/node_modules/npm/bin/npx-cli.js', link_path)
else:
assert(0) # unhandled action type

edit: Also note that bin/npm and bin/npx are actual files which are kept as files on Windows, but overwritten with the symlinks by install.py everywhere else.

@richardlau
Copy link
Member

richardlau commented Sep 21, 2018

Turns out this is a one line change to vcbuild.bat: #23001

(Edit: more changes were necessary for the installer)

richardlau added a commit to richardlau/node-1 that referenced this issue Oct 2, 2018
npm test directories are excluded on other platforms by
`tools/install.py`. Do the same on Windows.

Fixes: nodejs#22901

PR-URL: nodejs#23001
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: João Reis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants