Skip to content
This repository has been archived by the owner. It is now read-only.

npm install ignores nested shrinkwrapped dependencies #12372

Closed
misterbyrne opened this issue Apr 15, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@misterbyrne
Copy link
Contributor

commented Apr 15, 2016

Related issue? #6928

During an npm install, the shrinkwrap is inflated, but only one level deep. As a result the versions of these nested sub-dependencies are resolved the non-shrinkwrapped way.

This only seems to occur during multi-stage installs (i.e. rm -rf node_modules && npm install will result in the correct versions being installed). When it does happen it can result in different versions of sub-dependencies being installed than specified in the shrinkwrap file. This is probably more likely to happen where people rely on multi-stage installs (i.e. restoring a 'cached' node_modules directory from a previous build followed by npm install - pretty common in hosted CI - e.g. CodeShip / CircleCI).

Reproduction steps

(there's a similar test here that fails on the master branch)

  1. npm init

  2. npm install repeating@1.0.0 --save

  3. npm install redent@1.0.0 --save

  4. npm shrinkwrap - notice that there's a sub-dependency nested within the shrinkwrap

    image

  5. Edit the shrinkwrap file to change the repeating@2.0.1 sub-dependency to repeating@2.0.0, e.g.

    sed -i.bak "s/2.0.1/2.0.0/g" npm-shrinkwrap.json

    I know manually editing a shrinkwrap file is kinda crazy, but this is just to get a quick repro in place - in practise this happens frequently whenever the sub-dependency has a patch release version bump [albeit from a lower version number to a higher one].

  6. npm install

Expected outcome

The repeating@2.0.1 sub-dependency is downgraded to 2.0.0

Actual outcome

Nothing happens - the 'ideal' tree and the current tree are identical.

Incidentally, this was made apparent because ember cli runs a dependency checker before running and flags any version mismatches between what's in npm-shrinkwrap.json and node_modules.

@iarna

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2016

Yeah, this is awkward because we currently allow partial shrinkwraps (which is necessary to install hapi), which means that you're allowed to have modules in your tree that aren't in the shrinkwrap as long as they match a package.json entry, and in this case it does. =/

I'm not sure if there's any solution here short of nixing the partial shrinkwrap feature. (Which would please me greatly.)

@misterbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2016

Ah. Okay. Are there tests for the partial shrinkwrap feature? I'm wondering because my PR passes CI. I'll try have a dig round soon either way!

@misterbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2016

I've tried the following:

Comparing installing hapi

mkdir swtest
cd swtest
# install using system npm@latest
npm init
npm install hapi --save
# use git to see is there any difference between my branch and the latest release
git init
git add .
git commit -m"installed using npm@latest"
# install using npm@ my branch
node ~/src/npm/bin/npm-cli.js install hapi --save
# this shows 'nothing to commit' - no changes
git status

I tried a similar approach in the hapi repo and it worked similarly.
I'm probably missing something though :/

@iarna

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2016

Ok, I have had time to sit down and go through your repro and I understand the implications of your fix better now and it looks right to me. (My earlier concern around hapi was due to a misreading of the issue. Your change shouldn't cause problems.)

@misterbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2016

Aw cool, that's great news! Thanks so much for looking at this.

@iarna iarna closed this in b894413 May 13, 2016

@zkat zkat referenced this issue May 27, 2016

Closed

deps: upgrade npm to 3.9.3 #7030

4 of 4 tasks complete

lukesampson added a commit to lukesampson/scoop that referenced this issue Jun 4, 2016

Update NodeJs to 6.2.1 (security update) (#891)
# Notable changes

## Notable changes

* **buffer**: Ignore negative lengths in calls to `Buffer()` and `Buffer.allocUnsafe()`. This fixes a possible security concern (reported by Feross Aboukhadijeh) where user input is passed unchecked to the Buffer constructor or `allocUnsafe()` as it can expose parts of the memory slab used by other Buffers in the application. Note that negative lengths are not supported by the Buffer API and user input to the constructor should always be sanitised and type-checked. (Anna Henningsen) [#7051](nodejs/node#7051)
* **npm**: Upgrade npm to 3.9.3 (Kat Marchán) [#7030](nodejs/node#7030)
  - [`npm/npm@42d71be`](npm/npm@42d71be) [npm/npm#12685](npm/npm#12685) When using `npm ls <pkg>` without a semver specifier, `npm ls` would skip any packages in your tree that matched by name, but had a prerelease version in their `package.json`. ([@zkat](https://github.com/zkat))
  - [`npm/npm@f04e05`](npm/npm@df04e05) [npm/npm#10013](npm/npm#10013) `read-package-tree@5.1.4`: Fixes an issue where `npm install` would fail if your `node_modules` was symlinked. ([@iarna](https://github.com/iarna))
  - [`b894413`](npm/npm@b894413) [#12372](npm/npm#12372) Changing a nested dependency in an `npm-shrinkwrap.json` and then running `npm install` would not get up the updated package. This corrects that. ([@misterbyrne](https://github.com/misterbyrne))
  - This release includes `npm@3.9.0`, which is the result of our Windows testing push -- the test suite (should) pass on Windows now. We're working on getting AppVeyor to a place where we can just rely on it like Travis.
* **tty**: Default to blocking mode for stdio on OS X. A bug fix in libuv 1.9.0, introduced in Node.js v6.0.0, exposed problems with Node's use of non-blocking stdio, particularly on OS X which has a small output buffer. This change should fix CLI applications that have been having problems with output since Node.js v6.0.0 on OS X. The core team is continuing to address stdio concerns that exist across supported platforms and progress can be tracked at <nodejs/node#6980>. (Jeremiah Senkpiel) [#6895](nodejs/node#6895)
* **V8**: Upgrade to V8 5.0.71.52. This includes a fix that addresses problems experienced by users of node-inspector since Node.js v6.0.0, see <node-inspector/node-inspector#864> for details. (Michaël Zasso) [#6928](nodejs/node#6928)

misterbyrne added a commit to misterbyrne/ember-cli-dependency-checker that referenced this issue Jul 6, 2016

Detect npm >=3.9.2 and adjust the install command accordingly
npm v3.9.1 included a fix for the bug which makes the `rm -rf
node_modules` necessary ( npm/npm#12372 ), but
also exposed a different bug which was fixed in npm v3.9.2
( npm/npm#12724 )
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.