update npm to 5.8.0 #19840

MylesBorins · 2018-04-05T22:35:43Z

Another attempt after #19560 broke master. Because npm removed node-gyp we now need to vendor it. I've added it to tools/node_modules similar to our other vendored dependencies. I've also gone ahead and make a script to update the dep, update the license builder, update the license, and update the Makefile.

I'm going to run CITGM on this... I'm curious if this will have any effect on native modules at all.

/cc @iarna why did y'all remove node-gyp, and are there any edge cases we should expect?

MylesBorins · 2018-04-05T22:37:50Z

Also, it would be possible to move the node-gyp folder out of the npm directory before updating npm. This may make the diff a bit smaller when updating gyp via the script... although I'm not sure if this really makes a difference for git internals... just wanted to make an offer to do so

Currently npm explicitly doesn't support 10.x and will fail on master. This patch manually adds support for 10.x so that we can keep an up to date version of npm on master. refs: nodejs#17535 PR-URL: nodejs#17777 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

MylesBorins · 2018-04-05T22:39:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14081/
CITGM: https://ci.nodejs.org/job/citgm-smoker/1356/

MylesBorins · 2018-04-05T22:39:54Z

/cc @nodejs/build and @nodejs/tsc as this change has gotten a bit bigger in scope

jasnell · 2018-04-05T23:07:23Z

Wait... 5.8 removed node-gyp in a minor release? Ugh.

addaleax · 2018-04-05T23:19:33Z

@jasnell No, the files just moved around in-tree

jasnell · 2018-04-05T23:38:52Z

No, the files just moved around in-tree

Phew... ok good.... :-) gave me a slight mini heart attack there.

rvagg · 2018-04-05T23:39:19Z

so wait, was it removed or moved? did @MylesBorins add it to node_modules or .. what is going on? is there a reference for what this is all about?

addaleax · 2018-04-05T23:49:05Z

$ grep version tools/node_modules/node-gyp/package.json 
  "version": "3.6.2"
$ grep version deps/npm/node_modules/npm-lifecycle/node_modules/node-gyp/package.json 
  "version": "3.6.2"

This PR is adding a second copy.

I think we could just hoist node-gyp back to its old location in the npm tree manually

MylesBorins · 2018-04-06T00:24:03Z

Ahhh I don't know why I thought it had been removed instead of just moved. I'll update the PR I prefer to not change the location in the tree

…

On Thu, Apr 5, 2018, 7:49 PM Anna Henningsen ***@***.***> wrote: $ grep version tools/node_modules/node-gyp/package.json "version": "3.6.2" $ grep version deps/npm/node_modules/npm-lifecycle/node_modules/node-gyp/package.json "version": "3.6.2" This PR is adding a second copy. I think we could just hoist node-gyp back to its old location in the npm tree manually — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19840 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecVzKY-tJXNzMAqFpRho6zZ6lDEx_yks5tlq13gaJpZM4TJQlN> .

node-gyp has been moved in the tree of npm. This update fixes references to the location in the Makefile

MylesBorins · 2018-04-06T01:47:50Z

rolled back the node-gyp stuff and simply updated the Makefile to reference the new location in the npm node_modules tree.

CI: https://ci.nodejs.org/job/node-test-pull-request/14084/
CI for master: https://ci.nodejs.org/job/node-test-commit/17479/ (there were weird failures in last CI)

richardlau

vcbuild.bat will also need to be updated with the new node-gyp location.

richardlau · 2018-04-06T02:35:44Z

The AIX failure is real. node-gyp's lib/find-node-directory.js assumes it lives in npm/node_modules/node-gyp/lib.

Cc @nodejs/platform-aix

rvagg · 2018-04-06T03:31:50Z

The AIX failure is real. node-gyp's lib/find-node-directory.js assumes it lives in npm/node_modules/node-gyp/lib.

I've seen a lot of code (and maybe I've written some ...) that makes this assumption, moving it could cause some yelping. +1 to hoisting it although we should probably have this discussion with npm folks.

ChALkeR · 2018-04-06T06:40:23Z

I just observed Atom text editor failing to install or update any plugins because of this with system-wide npm@5.8.0 install.

https://github.com/npm/npm/blob/master/bin/node-gyp-bin/node-gyp — npm itself fails on 5.8.0 when executing this file.

Related: npm/npm#20163, mapbox/node-pre-gyp#362

zkat · 2018-04-06T15:06:43Z

As pointed out: we didn't remove node-gyp, but instead moved it so it lives under npm-lifecycle now, as part of refactoring these bits into separate pieces.

MylesBorins · 2018-04-06T15:08:51Z

@zkat I think the issue is that node-gyp itself has code internally expecting to live at that location... And there is a non trivial amount of code in the ecosystem making similar assumption

zkat · 2018-04-06T15:29:00Z

@MylesBorins npm/npm#20276

If this works, it should be pretty easy to backport as well.

MylesBorins · 2018-04-06T15:52:51Z

@zkat 🎉

ChALkeR · 2018-04-06T16:44:14Z

Yes, npm/npm#20276 looks like an ideal solution (especially the tests part). Thanks, @zkat!

BridgeAR · 2018-04-22T17:59:14Z

Should this stay open due to #20190?

MylesBorins · 2018-04-22T18:52:42Z

We are likely going to need to update the 5.x release line before 6.x lands At the very least this can re target 8.x

…

On Sun, Apr 22, 2018, 1:59 PM Ruben Bridgewater ***@***.***> wrote: Should this stay open due to #20190 <#20190>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19840 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV5QHmnZW126g4nqlF6Fe4QnCZX3rks5trMT4gaJpZM4TJQlN> .

p0psicles · 2018-05-02T08:08:21Z

@MylesBorins does this mean there is a chance that npm 5.8.x get's released (packaged) with a node 8.x.x version?

BridgeAR · 2018-05-15T15:37:26Z

Ping @MylesBorins about retargeting this PR

MylesBorins · 2018-05-15T16:06:25Z

@BridgeAR I'm going to go ahead and close this for now. We would need a different 5.x version to fix the node-gyp issue. If 6.x lands on master in a timely fashion we can backport in an 8.x minor, which is coming in the next month

MylesBorins requested review from addaleax and iarna April 5, 2018 22:35

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. npm Issues and PRs related to the npm client dependency or the npm registry. labels Apr 5, 2018

FallenRiteMonk and others added 2 commits April 5, 2018 18:38

deps: upgrade npm to 5.8.0

41bc096

MylesBorins force-pushed the npm-5.8.0 branch from 5efd4a2 to e7728b5 Compare April 5, 2018 22:38

tools: update location of node-gyp in MakeFile

0e55a61

node-gyp has been moved in the tree of npm. This update fixes references to the location in the Makefile

MylesBorins force-pushed the npm-5.8.0 branch from e7728b5 to 0e55a61 Compare April 6, 2018 01:46

richardlau reviewed Apr 6, 2018

View reviewed changes

jdalton mentioned this pull request Apr 6, 2018

Graceful node path detection. nodejs/node-gyp#1408

Closed

MylesBorins mentioned this pull request Apr 11, 2018

node-gyp@3.6.2 npm/npm#20276

Closed

This was referenced Apr 15, 2018

NPM Version and Node Version Out Of Sync #20038

Closed

10.0.0 Proposal #19091

Merged

mapsam mentioned this pull request Apr 27, 2018

node-gyp failure with npm 5.8.0 mapbox/node-pre-gyp#362

Open

MylesBorins closed this May 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update npm to 5.8.0 #19840

update npm to 5.8.0 #19840

MylesBorins commented Apr 5, 2018

MylesBorins commented Apr 5, 2018

MylesBorins commented Apr 5, 2018

MylesBorins commented Apr 5, 2018

jasnell commented Apr 5, 2018

addaleax commented Apr 5, 2018

jasnell commented Apr 5, 2018

rvagg commented Apr 5, 2018

addaleax commented Apr 5, 2018

MylesBorins commented Apr 6, 2018 via email

MylesBorins commented Apr 6, 2018

richardlau left a comment

richardlau commented Apr 6, 2018

rvagg commented Apr 6, 2018

ChALkeR commented Apr 6, 2018 •

edited

Loading

zkat commented Apr 6, 2018

MylesBorins commented Apr 6, 2018

zkat commented Apr 6, 2018 •

edited

Loading

MylesBorins commented Apr 6, 2018

ChALkeR commented Apr 6, 2018 •

edited

Loading

BridgeAR commented Apr 22, 2018

MylesBorins commented Apr 22, 2018 via email

p0psicles commented May 2, 2018

BridgeAR commented May 15, 2018

MylesBorins commented May 15, 2018

update npm to 5.8.0 #19840

update npm to 5.8.0 #19840

Conversation

MylesBorins commented Apr 5, 2018

MylesBorins commented Apr 5, 2018

MylesBorins commented Apr 5, 2018

MylesBorins commented Apr 5, 2018

jasnell commented Apr 5, 2018

addaleax commented Apr 5, 2018

jasnell commented Apr 5, 2018

rvagg commented Apr 5, 2018

addaleax commented Apr 5, 2018

MylesBorins commented Apr 6, 2018 via email

MylesBorins commented Apr 6, 2018

richardlau left a comment

Choose a reason for hiding this comment

richardlau commented Apr 6, 2018

rvagg commented Apr 6, 2018

ChALkeR commented Apr 6, 2018 • edited Loading

zkat commented Apr 6, 2018

MylesBorins commented Apr 6, 2018

zkat commented Apr 6, 2018 • edited Loading

MylesBorins commented Apr 6, 2018

ChALkeR commented Apr 6, 2018 • edited Loading

BridgeAR commented Apr 22, 2018

MylesBorins commented Apr 22, 2018 via email

p0psicles commented May 2, 2018

BridgeAR commented May 15, 2018

MylesBorins commented May 15, 2018

ChALkeR commented Apr 6, 2018 •

edited

Loading

zkat commented Apr 6, 2018 •

edited

Loading

ChALkeR commented Apr 6, 2018 •

edited

Loading