Skip to content

deps: update npm to 5.2.0#14163

Closed
zkat wants to merge 2 commits intonodejs:masterfrom
npm:npm-5.2.0
Closed

deps: update npm to 5.2.0#14163
zkat wants to merge 2 commits intonodejs:masterfrom
npm:npm-5.2.0

Conversation

@zkat
Copy link
Contributor

@zkat zkat commented Jul 11, 2017

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
Affected core subsystem(s)
  • deps
  • build
Description of Changes

Hey y'all! This release is a bit of a doozy:

  • It includes a ton of bugfixes that should seriously stabilize npm after that big 5.0 release (more coming!). A bunch of folks who couldn't use npm5 are now able to use it!
  • A semver-minor bump with several small feature additions because technically-semver
  • Another semver-minor bump that adds of npx to the primary npm distribution. This change in particular required modification of the node installers, based on the discussion in deps: Including additional binaries with npm? #13640.
Changelogs

@nodejs-github-bot nodejs-github-bot added the npm Issues and PRs related to the npm client dependency or the npm registry. label Jul 11, 2017
@zkat
Copy link
Contributor Author

zkat commented Jul 11, 2017

cc @addaleax @MylesBorins @Fishrock123 @refack

refack
refack approved these changes Jul 11, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

installers LGTM
rubberstamp the npm code

tools/msvs/msi/product.wxs Outdated
Copy link
Contributor

@refack refack Jul 11, 2017

Choose a reason for hiding this comment

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

👍 Ohh nice!
I just assumed you'd leave that to us.

@refack
Copy link
Contributor

refack commented Jul 11, 2017

We forgot the macOS installer https://github.com/nodejs/node/tree/master/tools/osx-pkg.pmdoc

@zkat
Copy link
Contributor Author

zkat commented Jul 11, 2017

@refack added. I think. 👍

@refack
Copy link
Contributor

refack commented Jul 11, 2017

@nodejs/platform-macos @nodejs/build who can verify macOS installer is correct?

@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 11, 2017 via email

@Fishrock123 Fishrock123 self-requested a review July 11, 2017 15:22
Fishrock123
Fishrock123 reviewed Jul 11, 2017
View reviewed changes
tools/osx-pkg-postinstall.sh
# TODO Can we extract $PREFIX from the installer?
cd /usr/local/bin
ln -sf ../lib/node_modules/npm/bin/npm-cli.js npm
ln -sf ../lib/node_modules/npm/bin/npx-cli.js npx
Copy link
Contributor

@Fishrock123 Fishrock123 Jul 11, 2017

Choose a reason for hiding this comment

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

IIRC this file doesn't actually run or something, but no harm in putting it here I suppose.

Copy link
Contributor

@MylesBorins MylesBorins Jul 11, 2017

Choose a reason for hiding this comment

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

that is correct... that script does nothing sadly... although I agree that we should either be explicit or just remove it

Copy link
Contributor

@refack refack Jul 14, 2017

Choose a reason for hiding this comment

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

So what does install npm on osx?

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 11, 2017

Is this semver-minor since npx would not be installed by default prior to this? I feel like it might be.

@Fishrock123 Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 11, 2017
@gibfahn
Copy link
Member

gibfahn commented Jul 11, 2017 •
edited
Loading

Is this semver-minor since npx would not be installed by default prior to this? I feel like it might be.

Seems minor to me, it's a new command that's available now but wasn't before. It doesn't change existing functionality.

Fishrock123
Fishrock123 suggested changes Jul 11, 2017
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm getting two failures:

EDIT2: both of these failures still appear after updating node properly.

test/tap/spec-local-specifiers.js ................... 68/76 22s
  removal > should remove the symlink
  not ok removed transitive dep
    +++ found
    --- wanted
    -not exists
    +exists
    compare: >-
      fs.stat(/Users/Jeremiah/Documents/node/test-npm/test/tap/spec-local-specifiers/testdir/remove-behavior/rmsymlink/dep1/node_modules/dep2)
    at:
      line: 70
      column: 7
      file: test/tap/spec-local-specifiers.js
      function: noFileExists
    stack: |
      noFileExists (test/tap/spec-local-specifiers.js:70:7)
      common.npm.spread (test/tap/spec-local-specifiers.js:725:7)
    source: >
      t.fail(message, {found: 'exists', wanted: 'not exists', compare: 'fs.stat(' +
      file + ')'})

and

test/tap/unit-deps-removeObsoleteDep.js ............... 2/3
  removeObsoleteDep
  not ok Cannot read property 'isTop' of undefined
    stack: |
      topHasNoPjson (lib/install/is-extraneous.js:11:14)
      isNotExtraneous (lib/install/is-extraneous.js:20:12)
      lib/install/is-extraneous.js:24:14
      Array.some (<anonymous>)
      isNotExtraneous (lib/install/is-extraneous.js:23:47)
      isExtraneous (lib/install/is-extraneous.js:5:17)
      lib/install/deps.js:178:9
      Array.forEach (<anonymous>)
      removeObsoleteDep (lib/install/deps.js:174:12)
      Test.<anonymous> (test/tap/unit-deps-removeObsoleteDep.js:41:3)
    at:
      line: 11
      column: 14
      file: lib/install/is-extraneous.js
      function: topHasNoPjson
    type: TypeError
    test: removeObsoleteDep
    source: |
      while (!top.isTop) top = top.parent

@zkat zkat mentioned this pull request Jul 11, 2017
Merged
@zkat
Copy link
Contributor Author

zkat commented Jul 12, 2017

@Fishrock123 we'll be rolling a new npm with fixes for this stuff like... soon? maybe tomorrow? later this week? Anyway, we've got the fixes 👍

@chorrell chorrell mentioned this pull request Jul 13, 2017
Closed
@addaleax
Copy link
Member

addaleax commented Jul 14, 2017

@zkat Do you think you could update this today? :)

@zkat
Copy link
Contributor Author

zkat commented Jul 14, 2017

@addaleax sure.

@zkat zkat mentioned this pull request Jul 14, 2017
Closed
4 tasks
@zkat
Copy link
Contributor Author

zkat commented Jul 14, 2017

Closing in favor of #14235

@zkat zkat closed this Jul 14, 2017
@zkat zkat deleted the npm-5.2.0 branch July 14, 2017 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@refack refack refack approved these changes

@MylesBorins MylesBorins MylesBorins left review comments

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

7 participants

@zkat @refack @MylesBorins @Fishrock123 @gibfahn @addaleax @nodejs-github-bot

Comments