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 5.4.2 #15486

Closed
wants to merge 1 commit into from
Closed

deps: update npm to 5.4.2 #15486

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Sep 20, 2017

Followed the guide to update npm to 5.4.2.

/cc @nodejs/npm @zkat @Fishrock123

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

@nodejs-github-bot nodejs-github-bot added the npm Issues and PRs related to the npm client dependency or the npm registry. label Sep 20, 2017
@targos
Copy link
Member Author

targos commented Sep 20, 2017

I don't know if make test-npm is supposed to pass now on Linux, but I'm getting a scary error:

/home/mzasso/git/nodejs/node/out/Release/node[3473]: ../src/node_zlib.cc:438:static void node::{anonymous}::ZCtx::Init(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `args.Length() == 7 && "init(windowBits, level, memLevel, strategy, writeResult, writeCallback," " dictionary)"' failed.
 1: node::Abort() [npm]
 2: node::Assert(char const* const (*) [4]) [npm]
 3: 0x12e40aa [npm]
 4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [npm]
 5: 0xb05dec [npm]
 6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [npm]
 7: 0x1010d2f0463d
tools/test-npm.sh: line 38:  3473 Aborted                 (core dumped) node bin/npm-cli.js install --ignore-scripts --no-save
make: *** [Makefile:406: test-npm] Error 134

@targos
Copy link
Member Author

targos commented Sep 20, 2017

I found the problem. The CHECK is triggered by a call from minizlib:

    this[_handle].init(opts.windowBits || constants.Z_DEFAULT_WINDOWBITS,
                       level,
                       opts.memLevel || constants.Z_DEFAULT_MEMLEVEL,
                       strategy,
                       opts.dictionary)

The library is directly calling the internal function with 5 arguments. The function's signature was changed four months ago from 4/5 arguments to 7 in add4b0a (zlib: improve performance).

@targos
Copy link
Member Author

targos commented Sep 20, 2017

minizlib is a dependency of the tar package (that has 9M downloads/month and 764 dependents, including npm).
/cc @nodejs/tsc

@Fishrock123
Copy link
Contributor

Let's let the npm folks chime in before deciding anything. Also looping in @isaacs who is the author of minizlib.

@addaleax
Copy link
Member

I guess that might be the root cause of #14161 then … sigh.

@MylesBorins
Copy link
Contributor

@targos could you open an issue on minizlib with the findings?

@gibfahn
Copy link
Member

gibfahn commented Sep 24, 2017

Given that #13322 hasn't landed on 8.x, could this be retargeted to 8.x for now?

I know it's not ideal for npm on 8.x to be ahead of master, but it sounds like this update fixes a bunch of serious bugs people are hitting, so getting this out would be a priority.

@zkat
Copy link
Contributor

zkat commented Sep 25, 2017

This LGTM -- would be good for @iarna to eyeball it too.

This includes three npm releases, one of which is pretty big (5.4.0).

Changes of Note

  • Significant performance boost for installations from cache (~10%+)
  • A number of bugfixes for important and severe bugs affecting installation on all supported platforms, but particularly noticeable on Windows.
  • More Windows-related fixes for npx.
  • b6d5549d2
    #17844
    Make package-lock.json sorting locale-agnostic. This will cause some users to see seemingly spurious diffs on their pkglocks if they were using previous versions of npm5, because, for example, JSONStream is sorted into a different location. This is fine and will go away as people upgrade.
    (@LotharSee)

Changelogs

@targos
Copy link
Member Author

targos commented Sep 25, 2017

Thanks @zkat! I updated the commit message with you notes.

PR against v8.x: #15600

@gibfahn gibfahn mentioned this pull request Sep 25, 2017
2 tasks
This includes three npm releases, one of which is pretty big (5.4.0).

Changes of Note

* Significant performance boost for installations from cache (~10%+)
* A number of bugfixes for important and severe bugs affecting
  installation on all supported platforms, but particularly noticeable
  on Windows.
* More Windows-related fixes for `npx`.
* [nodejs#17844](npm/npm#17844)
  Make package-lock.json sorting locale-agnostic. This will cause
  some users to see seemingly spurious diffs on their pkglocks if
  they were using previous versions of npm5, because, for example,
  `JSONStream` is sorted into a different location. This is fine and
  will go away as people upgrade.

Changelogs

* [`v5.4.0`](https://github.com/npm/npm/releases/tag/v5.4.0)
* [`v5.4.1`](https://github.com/npm/npm/releases/tag/v5.4.1)
* [`v5.4.2`](https://github.com/npm/npm/releases/tag/v5.4.2)
@MylesBorins
Copy link
Contributor

ping regarding getting this to work on master

@targos targos closed this Oct 23, 2017
@targos targos deleted the npm-5.4.2 branch October 23, 2017 13:04
@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 23, 2017

@targos is there a PR to replace this?

@targos
Copy link
Member Author

targos commented Oct 23, 2017

@MylesBorins No. There is no stable npm version that fixes the zlib issue at the moment.

@MylesBorins
Copy link
Contributor

@targos this should likely be kept open then in case we end up reverting the zlib fix

@targos
Copy link
Member Author

targos commented Oct 23, 2017

If that happens, we should update to the current version. It's not difficult to do. Feel free to ping me in case I miss it.

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.

7 participants