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

deps: update npm to v5.5.1 #16509

Closed
wants to merge 3 commits into from
Closed

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Oct 26, 2017

Followed guide

This PR manually floats 4ca6958196 from npm upstream to update zlib. This can obviously be trumped by a 5.5.2 release if we can get it in before 9.x, but it seemed like to best option to be able to ship today and remove a bit of the pressure for next weeks release (and get 2 factor into the LTS release 🎉).

Refs: npm/npm@4ca6958
Closes: #16280
Fixes: #14161

/cc @iarna @zkat

@MylesBorins MylesBorins added this to the 9.0.0 milestone Oct 26, 2017
@nodejs-github-bot nodejs-github-bot added the npm Issues and PRs related to the npm client dependency or the npm registry. label Oct 26, 2017
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Oct 26, 2017

I had to float another patch to explicitly support Node 9.x

sent a pr upstream npm/npm#18964

edit: it is worth mentioning that the tests pass on my local machine once these two extra patches are floated on the 5.5.1 release

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of feel like floating a patch should happen in a separate commit, but this should be fine either way. Thanks!

@MylesBorins
Copy link
Contributor Author

@addaleax done 😄

@MylesBorins
Copy link
Contributor Author

could someone test locally and see if it works for them?

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Oct 27, 2017

so I'm a bit concerned about a couple things.

when I followed the instructions from the npm wiki there were a whole bunch of files that git ignored when I tried to add them. I ended up adding the entire npm folder with "add -f"

Now we are seeing +118,079 −43,651 in the diff... which may be correct, and may not be correct.

What it could mean is that we have been shipping a broken npm in other versions. I'm going to dig in and review the process to see if we have been accidentally shipping a bad tree

edit: one of the culprits may be my global git ignore... especially ignoring a folder called tar

edit 2: verified that 5.3.0 is fine, but heed this warning future backporters. We should likely downstream the updating npm guide... PR incoming

edit 3: By using whitespace fix I took the diff down to +109,421 −35,117

MylesBorins and others added 3 commits October 26, 2017 23:27
Original commit message:

    minizlib@1.0.4

    Fixes Node 9 compatibility.

    Credit: @isaacs
@targos
Copy link
Member

targos commented Oct 29, 2017

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I have about the same amount of changes if I do the update on my side.

@jasnell
Copy link
Member

jasnell commented Oct 29, 2017

This has to land before noon pacific time today in order to make it in to 9.0.0

@MylesBorins
Copy link
Contributor Author

landed in 64168eb...b8888f5

MylesBorins added a commit that referenced this pull request Oct 30, 2017
Closes: #16280

PR-URL: #16509
Fixes: #14161
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins added a commit that referenced this pull request Oct 30, 2017
Original commit message:

    minizlib@1.0.4

    Fixes Node 9 compatibility.

    Credit: @isaacs

PR-URL: #16509
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins added a commit that referenced this pull request Oct 30, 2017
refs: npm/npm#18964

PR-URL: #16509
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins added a commit that referenced this pull request Oct 30, 2017
Closes: #16280

PR-URL: #16509
Fixes: #14161
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Closes: nodejs/node#16280

PR-URL: nodejs/node#16509
Fixes: nodejs/node#14161
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Original commit message:

    minizlib@1.0.4

    Fixes Node 9 compatibility.

    Credit: @isaacs

PR-URL: nodejs/node#16509
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
refs: npm/npm#18964

PR-URL: nodejs/node#16509
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
This LTS release comes with 87 commits. This includes 30 that are
updates to lib/ or src/, 20 that are test related, 13 that are doc
related, 19 which are build / tools related, and 4 commits which are
updates to dependencies.

Notable Changes:

* doc:
  - add Gibson Fahnestock to Release team (Gibson Fahnestock)
    nodejs/node#16620
* deps:
  - update npm to 5.5.1 (Myles Borins)
    nodejs/node#16509
* http2:
  - The exposed http2 socket is no longer manipulatable
    (Anatoli Papirovski)
    nodejs/node#16330
* module:
  - support custom paths to require.resolve() (cjihrig)
    nodejs/node#16397
* util:
  - util.TextEncoder and util.TextDecoder are no longer experimental.
    There will no longer be a warning when they are used
    (James M Snell)
    nodejs/node#15743

PR-URL: nodejs/node#16630
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Closes: nodejs/node#16280

PR-URL: nodejs/node#16509
Fixes: nodejs/node#14161
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Original commit message:

    minizlib@1.0.4

    Fixes Node 9 compatibility.

    Credit: @isaacs

PR-URL: nodejs/node#16509
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
refs: npm/npm#18964

PR-URL: nodejs/node#16509
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
This LTS release comes with 87 commits. This includes 30 that are
updates to lib/ or src/, 20 that are test related, 13 that are doc
related, 19 which are build / tools related, and 4 commits which are
updates to dependencies.

Notable Changes:

* doc:
  - add Gibson Fahnestock to Release team (Gibson Fahnestock)
    nodejs/node#16620
* deps:
  - update npm to 5.5.1 (Myles Borins)
    nodejs/node#16509
* http2:
  - The exposed http2 socket is no longer manipulatable
    (Anatoli Papirovski)
    nodejs/node#16330
* module:
  - support custom paths to require.resolve() (cjihrig)
    nodejs/node#16397
* util:
  - util.TextEncoder and util.TextDecoder are no longer experimental.
    There will no longer be a warning when they are used
    (James M Snell)
    nodejs/node#15743

PR-URL: nodejs/node#16630
@MylesBorins MylesBorins deleted the npm-5.5.1 branch November 14, 2017 17:44
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Closes: nodejs/node#16280

PR-URL: nodejs/node#16509
Fixes: nodejs/node#14161
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Original commit message:

    minizlib@1.0.4

    Fixes Node 9 compatibility.

    Credit: @isaacs

PR-URL: nodejs/node#16509
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
refs: npm/npm#18964

PR-URL: nodejs/node#16509
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
This LTS release comes with 87 commits. This includes 30 that are
updates to lib/ or src/, 20 that are test related, 13 that are doc
related, 19 which are build / tools related, and 4 commits which are
updates to dependencies.

Notable Changes:

* doc:
  - add Gibson Fahnestock to Release team (Gibson Fahnestock)
    nodejs/node#16620
* deps:
  - update npm to 5.5.1 (Myles Borins)
    nodejs/node#16509
* http2:
  - The exposed http2 socket is no longer manipulatable
    (Anatoli Papirovski)
    nodejs/node#16330
* module:
  - support custom paths to require.resolve() (cjihrig)
    nodejs/node#16397
* util:
  - util.TextEncoder and util.TextDecoder are no longer experimental.
    There will no longer be a warning when they are used
    (James M Snell)
    nodejs/node#15743

PR-URL: nodejs/node#16630
@kasicka
Copy link

kasicka commented Feb 5, 2018

Can we backport npm/npm@4ca6958 to v8.x branch too? I get npm ERR! invalid: minizlib@1.0.3 /usr/local/lib/node_modules/npm/node_modules/tar/node_modules/minizlib when I try npm ls -g with npm bundled with v8.9.4, backporting 4ca695819 fixes the error.. Or should people from npm fix this?

@richardlau
Copy link
Member

@kasicka We usually try not to float patches on top of npm. npm/npm@4ca6958 is included in npm@5.6.0 so it probably makes more sense to backport either #17535 or #17777 to Node.js 8.

cc @nodejs/lts @nodejs/npm

@kasicka
Copy link

kasicka commented Feb 5, 2018

@richardlau npm@5.6.0 is included in v8.9.4, yet the error is present.. tl;dr is minizlib should be updated to 1.0.4

@richardlau
Copy link
Member

npm@5.6.0 is included in v8.9.4, yet the error is present.. tl;dr is minizlib should be updated to 1.0.4

@gibfahn Did something get left out of the npm update for v8.9.4?

@gibfahn
Copy link
Member

gibfahn commented Feb 6, 2018

It was backported by @MylesBorins according to #16509 (comment), so question for him 🤷‍♂️

@kasicka
Copy link

kasicka commented Feb 6, 2018

Seems like only 0d7e4d2 was backported to v8.x branches

@gibfahn
Copy link
Member

gibfahn commented Feb 6, 2018

Okay, sounds like we need to backport npm/npm@4ca6958 (9f33a24) in the upcoming 8.x release.

@kasicka
Copy link

kasicka commented Feb 7, 2018

Should b8888f5 be backported too?

@richardlau
Copy link
Member

Should b8888f5 be backported too?

IMO yes it should, because 8.x is supposed to contain npm@5.6.0 and without it the files in our v8.x branch do not match the equivalent in npm's source tree:

https://github.com/npm/npm/blob/v5.6.0/lib/utils/unsupported.js#L3-L9

https://github.com/nodejs/node/blob/v8.9.4/deps/npm/lib/utils/unsupported.js#L3-L8

We're obviously going to have to be careful with backports when we float patches on top of npm on master/current (e.g., 9f33a24 and b8888f5) because it means those diffs aren't included in the subsequent "update npm to vX" PRs.

cc @nodejs/lts

MylesBorins added a commit to MylesBorins/node that referenced this pull request Feb 7, 2018
When the original backport was landed there were some missing commits
on v8.x. This commit completely re lands npm 5.6.0 to make sure the
version on 8.x has the correct file tree

Refs: nodejs#16509 (comment)
gibfahn pushed a commit that referenced this pull request Feb 8, 2018
When the original backport was landed there were some missing commits
on v8.x. This commit completely re lands npm 5.6.0 to make sure the
version on 8.x has the correct file tree

Refs: #16509 (comment)
PR-URL: #18625
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Issues and PRs related to the npm client dependency or the npm registry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants