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.

Sign up for GitHub

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

Jump to bottom

Npm 5.8.0 #19560

Closed
wants to merge 2 commits into from
Closed

Npm 5.8.0 #19560

wants to merge 2 commits into from

Conversation

FallenRiteMonk
Copy link
Contributor

Fixes: #19271

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. 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. tools Issues and PRs related to the tools directory. labels Mar 23, 2018
@FallenRiteMonk
Copy link
Contributor Author

As allready questioned in #19298, this time i followed https://github.com/nodejs/node/blob/master/doc/guides/maintaining-npm.md

@FallenRiteMonk
Copy link
Contributor Author

Rebased

@targos
Copy link
Member

targos commented Mar 23, 2018

Thank you. I can reproduce the same diff.

@targos targos added semver-minor PRs that contain new features and should be released in the next minor version. and removed build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Mar 23, 2018
@targos
Copy link
Member

targos commented Mar 23, 2018

CI: https://ci.nodejs.org/view/All/job/node-test-npm/35/

@addaleax
Copy link
Member

Just going to point out that 5.8.0 is a pre-release at this point and we probably shouldn't merge this PR until it is ready?

@richardlau richardlau added the blocked PRs that are blocked by other issues or PRs. label Mar 23, 2018
@richardlau
Copy link
Member

Marking blocked until 5.8.0 is actually released.

@Trott
Copy link
Member

Trott commented Mar 23, 2018

Marking blocked until 5.8.0 is actually released.

Stating the obvious just in case it's not obvious: Once it's released, this will no doubt need to be updated with whatever the actual release contains...

@ckotzbauer
Copy link

5.8 is marked as latest now

@targos targos removed the blocked PRs that are blocked by other issues or PRs. label Mar 24, 2018
@richardlau
Copy link
Member

Please check before landing that this matches the released npm 5.8.0 (i.e. nothing was changed in npm between 5.8.0-next and 5.8.0).

Also I think we'll need to refloat cbd6349 or backport npm/npm@f721eec since the upgrade process removes the entire npm directory in deps.

@targos
Copy link
Member

targos commented Mar 25, 2018

Please check before landing that this matches the released npm 5.8.0 (i.e. nothing was changed in npm between 5.8.0-next and 5.8.0).

I did this verification.

Also I think we'll need to refloat cbd6349 or backport npm/npm@f721eec since the upgrade process removes the entire npm directory in deps.

+1. @FallenRiteMonk can you please cherry-pick this commit: cbd6349

@FallenRiteMonk
Copy link
Contributor Author

@targos done!

@richardlau richardlau mentioned this pull request Mar 29, 2018
Closed
@vsemozhetbyt
Copy link
Contributor

Is it OK to land?

We have a pending issue #19405 to resolve before the v10 release, maybe it is worth to have more time to test the fix with npm 5.8.0.

@ChALkeR
Copy link
Member

ChALkeR commented Mar 31, 2018

Is deps/npm-latest.zip needed?

@ChALkeR
Copy link
Member

ChALkeR commented Mar 31, 2018

Also, is there a reason why this keeps deps/npm/node_modules/node-gyp intact?

@MylesBorins
Copy link
Member

MylesBorins commented Apr 5, 2018 •
edited

should it not be there?

@ChALkeR node-gyp exists on every branch right now

master

https://github.com/nodejs/node/tree/master/deps/npm/node_modules/node-gyp

v9.x

https://github.com/nodejs/node/tree/v9.x-staging/deps/npm/node_modules/node-gyp

v8.x

https://github.com/nodejs/node/tree/v8.x-staging/deps/npm/node_modules/node-gyp

v6.x

https://github.com/nodejs/node/tree/v6.x-staging/deps/npm/node_modules/node-gyp

v4.x

https://github.com/nodejs/node/tree/v4.x-staging/deps/npm/node_modules/node-gyp

@ChALkeR
Copy link
Member

ChALkeR commented Apr 5, 2018 •
edited

@MylesBorins Yes, it is present in previous npm versions, but it was moved in 5.8.0 afaik.
So the question is — why does that tooling keep it?

@targos
Copy link
Member

targos commented Apr 5, 2018

@ChALkeR We don't have special tooling. We just run make release from the npm repo. I can't explain why it would generate something different from the official npm release.

@ChALkeR
Copy link
Member

ChALkeR commented Apr 5, 2018 •
edited

@targos I just ran make release locally on npm 5.8.0 and it doesn't produce that dir in archives.

@targos
Copy link
Member

targos commented Apr 5, 2018

@ChALkeR Got it. I had to remove the npm repo and get a fresh clone. Now it's right on my side.

@FallenRiteMonk Would you like to do that and update this PR?

@FallenRiteMonk
Copy link
Contributor Author

@targos at the moment I'm a bit short on time, so if you are faster go ahead and do it, otherwise I'll update once I finde time to update.

FallenRiteMonk and others added 2 commits April 5, 2018 11:54
@FallenRiteMonk @MylesBorins
deps: upgrade npm to 5.8.0
13fe212
@MylesBorins
deps: manually add 10.x support to npm
8f15080 
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
Copy link
Member

Updated without the node-gyp folder... testing locally

@MylesBorins
Copy link
Member

landed in b29c36b...55557ba

@MylesBorins MylesBorins closed this Apr 5, 2018
MylesBorins pushed a commit that referenced this pull request Apr 5, 2018
@FallenRiteMonk @MylesBorins
deps: upgrade npm to 5.8.0
25a816d 
PR-URL: #19560
Fixes: #19271
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins added a commit that referenced this pull request Apr 5, 2018
@MylesBorins
deps: manually add 10.x support to npm
55557ba 
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: #17535
Backport-PR-URL: #19560
PR-URL: #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
Copy link
Member

This needs to be manually backported to 8.x, I will get on doing this soon

@addaleax
Copy link
Member

addaleax commented Apr 5, 2018

@MylesBorins This broke CI + local make test… :(

Revert: #19837

@addaleax
Copy link
Member

addaleax commented Apr 5, 2018

@FallenRiteMonk or @MylesBorins can one of you open a new PR? Re-opening already landed PRs doesn’t play well with our tooling, iirc…

@MylesBorins
Copy link
Member

@addaleax on it. My apologies for breaking all the things

@MylesBorins
Copy link
Member

MylesBorins commented Apr 5, 2018 •
edited

ALSO LOOOOOL that what broke this is the missing node-gyp

@refack
Copy link
Contributor

refack commented Apr 5, 2018

So what's the tl;dr? is npm's make release incomplete (not pulling the new npm-lifecycle)?

@MylesBorins MylesBorins mentioned this pull request Apr 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MylesBorins MylesBorins MylesBorins approved these changes

@targos targos targos approved these changes

Assignees
No one assigned
Labels
npm Issues and PRs related to the npm client dependency or the npm registry. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

npm 5.7.x in 8.x LTS