FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Npm 5.8.0 by FallenRiteMonk · Pull Request #19560 · nodejs/node · GitHub

/ node Public

Npm 5.8.0 - #19560

Closed
FallenRiteMonk wants to merge 2 commits into
nodejs:masterfrom
FallenRiteMonk:npm-5.8.0
ConversationCommits (2)ChecksFiles changed
Closed

Npm 5.8.0#19560
FallenRiteMonk wants to merge 2 commits into
nodejs:masterfrom
FallenRiteMonk:npm-5.8.0

Conversation

FallenRiteMonk commented Mar 23, 2018

Copy link
Copy Markdown
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 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 commented Mar 23, 2018

Copy link
Copy Markdown
Contributor Author

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

FallenRiteMonk commented Mar 23, 2018

Copy link
Copy Markdown
Contributor Author

Rebased

targos commented Mar 23, 2018

Copy link
Copy Markdown
Member

Thank you. I can reproduce the same diff.

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 commented Mar 23, 2018

Copy link
Copy Markdown
Member

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

addaleax commented Mar 23, 2018

Copy link
Copy Markdown
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 added the blocked PRs that are blocked by other issues or PRs. label Mar 23, 2018

richardlau commented Mar 23, 2018

Copy link
Copy Markdown
Member

Marking blocked until 5.8.0 is actually released.

Trott commented Mar 23, 2018

Copy link
Copy Markdown
Member

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 commented Mar 24, 2018

Copy link
Copy Markdown

5.8 is marked as latest now

targos removed the blocked PRs that are blocked by other issues or PRs. label Mar 24, 2018

richardlau commented Mar 25, 2018

Copy link
Copy Markdown
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 commented Mar 25, 2018

Copy link
Copy Markdown
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).

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 commented Mar 26, 2018

Copy link
Copy Markdown
Contributor Author

@targos done!

richardlau mentioned this pull request Mar 29, 2018
Closed

vsemozhetbyt commented Mar 29, 2018

Copy link
Copy Markdown
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 commented Mar 31, 2018

Copy link
Copy Markdown
Member

Is deps/npm-latest.zip needed?

ChALkeR commented Mar 31, 2018

Copy link
Copy Markdown
Member

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

MylesBorins commented Apr 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Apr 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@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 commented Apr 5, 2018

Copy link
Copy Markdown
Member

@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 commented Apr 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

targos commented Apr 5, 2018

Copy link
Copy Markdown
Member

@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 commented Apr 5, 2018

Copy link
Copy Markdown
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
deps: upgrade npm to 5.8.0
13fe212
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 commented Apr 5, 2018

Copy link
Copy Markdown
Contributor

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

MylesBorins commented Apr 5, 2018

Copy link
Copy Markdown
Contributor

landed in b29c36b...55557ba

MylesBorins closed this Apr 5, 2018
MylesBorins pushed a commit that referenced this pull request Apr 5, 2018
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
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 commented Apr 5, 2018

Copy link
Copy Markdown
Contributor

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

addaleax commented Apr 5, 2018

Copy link
Copy Markdown
Member

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

Revert: #19837

addaleax commented Apr 5, 2018

Copy link
Copy Markdown
Member

@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 commented Apr 5, 2018

Copy link
Copy Markdown
Contributor

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

MylesBorins commented Apr 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

refack commented Apr 5, 2018

Copy link
Copy Markdown
Contributor

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

MylesBorins mentioned this pull request Apr 5, 2018
Closed
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
MylesBorins MylesBorins approved these changes
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

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
Back | FazBrowse Home | New Git URL