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.

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

deps: update npm to 5.2.0 #14163

Closed
wants to merge 2 commits into from
Closed

deps: update npm to 5.2.0 #14163

wants to merge 2 commits into from

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
@@ -95,6 +95,8 @@
Description="!(loc.npm_Description)">
<ComponentRef Id="NpmCmdScript"/>
<ComponentRef Id="NpmBashScript"/>
<ComponentRef Id="NpxCmdScript"/>
Copy link
Contributor

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
@@ -3,3 +3,4 @@
# 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

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

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

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

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

@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

@refack refack refack approved these changes

@MylesBorins MylesBorins MylesBorins left review comments

@Fishrock123 Fishrock123 Fishrock123 requested changes

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