Skip to content

Npm 5.8.0#19560

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
Closed
FallenRiteMonk wants to merge 2 commits into from
Closed

Npm 5.8.0 #19560

FallenRiteMonk wants to merge 2 commits into from

Conversation

@FallenRiteMonk
Copy link
Contributor

@FallenRiteMonkFallenRiteMonk commented Mar 23, 2018

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-botnodejs-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
ContributorAuthor

FallenRiteMonk commented Mar 23, 2018

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

@FallenRiteMonk
Copy link
ContributorAuthor

FallenRiteMonk commented Mar 23, 2018

Rebased

@targos
Copy link
Member

targos commented Mar 23, 2018

Thank you. I can reproduce the same diff.

@targostargos 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

addaleax commented Mar 23, 2018

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?

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

richardlau commented Mar 23, 2018

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

ckotzbauer commented Mar 24, 2018

5.8 is marked as latest now

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

richardlau 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).

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
ContributorAuthor

FallenRiteMonk commented Mar 26, 2018

@targos done!

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

vsemozhetbyt commented Mar 29, 2018

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
Contributor

MylesBorins commented Apr 5, 2018•
edited
Loading

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
Loading

@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
Loading

@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
ContributorAuthor

FallenRiteMonk commented Apr 5, 2018

@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.

FallenRiteMonkand 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 5, 2018

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

@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 5, 2018

landed in b29c36b...55557ba

MylesBorins pushed a commit that referenced this pull request Apr 5, 2018
@FallenRiteMonk@MylesBorins
deps: upgrade npm to 5.8.0
25a816d
PR-URL: #19560Fixes: #19271 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Myles Borins <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 5, 2018

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
Contributor

MylesBorins commented Apr 5, 2018

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

@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 5, 2018•
edited
Loading

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)?

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

Reviewers

@targostargostargos approved these changes

+1 more reviewer

@MylesBorinsMylesBorinsMylesBorins approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

npmIssues and PRs related to the npm client dependency or the npm registry.semver-minorPRs 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

11 participants

@FallenRiteMonk@targos@addaleax@richardlau@Trott@ckotzbauer@vsemozhetbyt@ChALkeR@MylesBorins@refack@nodejs-github-bot