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

`npm shrinkwrap` should be idempotent #5779

Closed
othiym23 opened this Issue Jul 23, 2014 · 14 comments

Comments

Projects
None yet
6 participants
@othiym23
Contributor

othiym23 commented Jul 23, 2014

Reason: noisy diffs, potential nondeterminism.

Key order within the dependencies and other properties should have a stable ordering (i.e. be sorted). A useful implementation strategy would also involve centralizing all the calls to JSON.stringify so that we have a standard way of marshalling objects for external use.

@othiym23 othiym23 added this to the multi-stage install milestone Jul 23, 2014

@grncdr

This comment has been minimized.

Show comment
Hide comment
@grncdr

grncdr Jul 25, 2014

Contributor

If you're not already aware of it, Uber's npm-shrinkwrap already does this. We've switched to using it at our company and it's made working with shrinkwrap files much less painful.

Contributor

grncdr commented Jul 25, 2014

If you're not already aware of it, Uber's npm-shrinkwrap already does this. We've switched to using it at our company and it's made working with shrinkwrap files much less painful.

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Jul 26, 2014

Contributor

I'm super aware of npm-shrinkwrap (pretty sure there's no way @Raynos would not let us be aware of it), and I'll probably be stealing bits and pieces from it as I try to turn shrinkwrap into the feature it's supposed to be. Thanks for the pointer!

Contributor

othiym23 commented Jul 26, 2014

I'm super aware of npm-shrinkwrap (pretty sure there's no way @Raynos would not let us be aware of it), and I'll probably be stealing bits and pieces from it as I try to turn shrinkwrap into the feature it's supposed to be. Thanks for the pointer!

@iarna iarna referenced this issue Dec 15, 2014

Closed

Multi-stage installer --save* support #6933

6 of 6 tasks complete
@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Dec 15, 2014

Member

I'm closing this as this will be addressed by #6933. Further discussion should happen there.

Member

iarna commented Dec 15, 2014

I'm closing this as this will be addressed by #6933. Further discussion should happen there.

@iarna iarna closed this Dec 15, 2014

@iarna iarna reopened this Apr 8, 2015

@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Apr 8, 2015

Member

Reopening because this was not addressed by #6933

Member

iarna commented Apr 8, 2015

Reopening because this was not addressed by #6933

@iarna iarna removed this from the multi-stage install milestone Apr 8, 2015

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Apr 8, 2015

From a project without an existing node_modules or shrinkwrap:

npm install
npm shrinkwrap
rm -rf node_modules
npm install
npm shrinkwrap

The shrinkwrap file changes.

wagenet commented Apr 8, 2015

From a project without an existing node_modules or shrinkwrap:

npm install
npm shrinkwrap
rm -rf node_modules
npm install
npm shrinkwrap

The shrinkwrap file changes.

@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Apr 8, 2015

Member

Do check out the somewhat confusingly named https://www.npmjs.com/package/npm-shrinkwrap module in the interim.

Member

iarna commented Apr 8, 2015

Do check out the somewhat confusingly named https://www.npmjs.com/package/npm-shrinkwrap module in the interim.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Apr 8, 2015

Also related to #7292.

We're looking into dropping the from field or not rewriting it in 2.x. Very happy to work with y'all to get something upstream. If the from field is only used for logging context and not logic, we should be able to figure something out.

mixonic commented Apr 8, 2015

Also related to #7292.

We're looking into dropping the from field or not rewriting it in 2.x. Very happy to work with y'all to get something upstream. If the from field is only used for logging context and not logic, we should be able to figure something out.

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Apr 8, 2015

Contributor

The original version of the shrinkwrap algorithm saved only versions:

commit d54ce3154dfe5283fcfeffc13d4e003bbade6370
Author: Dave Pacheco <dap@joyent.com>
Date:   Tue Feb 21 15:32:16 2012 -0800

    add "npm shrinkwrap"

diff --git a/lib/install.js b/lib/install.js
--- a/lib/install.js
+++ b/lib/install.js
@@ -138,1 +173,4 @@
-      fn(args, where, family, ancestors, true, data, cb)
+      rv["dependencies"] = {}
+      for (key in wrap)
+        rv["dependencies"][key] = wrap[key]["version"]
+      log.verbose([rv["dependencies"]], "readDependencies: returned deps")

A later version added support for from to handle dependencies installed from URLs better (which also eventually added support for installing from GitHub repo branches):

commit 4bb884d2f8a30701c05ed81f7ace62be53147cae
Author: isaacs <i@izs.me>
Date:   Tue Feb 28 17:25:18 2012 -0800

    shrinkwrap: Behave properly with url-installed deps

diff --git a/lib/install.js b/lib/install.js
--- a/lib/install.js
+++ b/lib/install.js
@@ -186,2 +187,3 @@
-        rv.dependencies[key] = wrap[key].version
+        var w = wrap[key]
+        rv.dependencies[key] = w.from || w.version
       })

Later, to make shrinkwrap installs idempotent across machines, support for resolved was added:

commit cfce70eae008e39f5ad00b4167fb5bd81857c377
Author: isaacs <i@izs.me>
Date:   Mon Jan 14 21:43:51 2013 -0800

    Use stashed git sha in shrinkwrap install

    Fix #3056

diff --git a/lib/install.js b/lib/install.js
--- a/lib/install.js
+++ b/lib/install.js
@@ -210,3 +210,2 @@
-        var w = wrap[key]
-        rv.dependencies[key] = w.from || w.version
+        rv.dependencies[key] = readWrap(wrap[key])
       })

where readWrap is

function readWrap (w) {
  return (w.resolved) ? w.resolved
       : (w.from && url.parse(w.from).protocol) ? w.from
       : w.version
}

When we get back to the long-delayed cache rewrite, we're going to add support for shasum into shrinkwrap files, both to reduce (tremendously) the amount of bandwidth a shrinkwrapped install uses and further increase idempotency across shrinkwrapped installs.

At some point there were versions of npm that depended on all of those fields, and there are various third-party tools like npm-shrinkwrap that rely on the current behavior to do their job, so simply removing from is a breaking change, and one that makes me pretty uncomfortable. At one point, the value of from was stable from run to run, but something changed in the caching logic over the last year, and now it's much more likely to change from run to run of shrinkwrap. I would be much more in favor of figuring out how to make from a consistent value than just removing it outright.

Contributor

othiym23 commented Apr 8, 2015

The original version of the shrinkwrap algorithm saved only versions:

commit d54ce3154dfe5283fcfeffc13d4e003bbade6370
Author: Dave Pacheco <dap@joyent.com>
Date:   Tue Feb 21 15:32:16 2012 -0800

    add "npm shrinkwrap"

diff --git a/lib/install.js b/lib/install.js
--- a/lib/install.js
+++ b/lib/install.js
@@ -138,1 +173,4 @@
-      fn(args, where, family, ancestors, true, data, cb)
+      rv["dependencies"] = {}
+      for (key in wrap)
+        rv["dependencies"][key] = wrap[key]["version"]
+      log.verbose([rv["dependencies"]], "readDependencies: returned deps")

A later version added support for from to handle dependencies installed from URLs better (which also eventually added support for installing from GitHub repo branches):

commit 4bb884d2f8a30701c05ed81f7ace62be53147cae
Author: isaacs <i@izs.me>
Date:   Tue Feb 28 17:25:18 2012 -0800

    shrinkwrap: Behave properly with url-installed deps

diff --git a/lib/install.js b/lib/install.js
--- a/lib/install.js
+++ b/lib/install.js
@@ -186,2 +187,3 @@
-        rv.dependencies[key] = wrap[key].version
+        var w = wrap[key]
+        rv.dependencies[key] = w.from || w.version
       })

Later, to make shrinkwrap installs idempotent across machines, support for resolved was added:

commit cfce70eae008e39f5ad00b4167fb5bd81857c377
Author: isaacs <i@izs.me>
Date:   Mon Jan 14 21:43:51 2013 -0800

    Use stashed git sha in shrinkwrap install

    Fix #3056

diff --git a/lib/install.js b/lib/install.js
--- a/lib/install.js
+++ b/lib/install.js
@@ -210,3 +210,2 @@
-        var w = wrap[key]
-        rv.dependencies[key] = w.from || w.version
+        rv.dependencies[key] = readWrap(wrap[key])
       })

where readWrap is

function readWrap (w) {
  return (w.resolved) ? w.resolved
       : (w.from && url.parse(w.from).protocol) ? w.from
       : w.version
}

When we get back to the long-delayed cache rewrite, we're going to add support for shasum into shrinkwrap files, both to reduce (tremendously) the amount of bandwidth a shrinkwrapped install uses and further increase idempotency across shrinkwrapped installs.

At some point there were versions of npm that depended on all of those fields, and there are various third-party tools like npm-shrinkwrap that rely on the current behavior to do their job, so simply removing from is a breaking change, and one that makes me pretty uncomfortable. At one point, the value of from was stable from run to run, but something changed in the caching logic over the last year, and now it's much more likely to change from run to run of shrinkwrap. I would be much more in favor of figuring out how to make from a consistent value than just removing it outright.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Apr 9, 2015

@othiym23 thanks for this context. Great background. Agreed that the best solution seems to be making from a constant value. We got up to speed on the codebase today and wrote a test for this behavior. /cc @bantic

I've pinged @Raynos to discuss how npm-shrinkwrap uses the from field.

I'd really like to encourage Ember-CLI users to use shrinkwrap. Along with quaertym/ember-cli-dependency-checker#26 it will provide a decent path to re-producable builds. This issue is the last major blocker for me getting there.

mixonic commented Apr 9, 2015

@othiym23 thanks for this context. Great background. Agreed that the best solution seems to be making from a constant value. We got up to speed on the codebase today and wrote a test for this behavior. /cc @bantic

I've pinged @Raynos to discuss how npm-shrinkwrap uses the from field.

I'd really like to encourage Ember-CLI users to use shrinkwrap. Along with quaertym/ember-cli-dependency-checker#26 it will provide a decent path to re-producable builds. This issue is the last major blocker for me getting there.

@Raynos

This comment has been minimized.

Show comment
Hide comment
@Raynos

Raynos Apr 9, 2015

Contributor

The most important thing about the from field is that I use it to compute resolved iff resolved is missing ( https://github.com/uber/npm-shrinkwrap/blob/d589a48ffddc40769ef550e568cf661bbe26bf55/set-resolved.js#L74-L76 ).

Just make sure you always set resolved.

Also note that npm-shrinkwrap is mostly for old npm.

Contributor

Raynos commented Apr 9, 2015

The most important thing about the from field is that I use it to compute resolved iff resolved is missing ( https://github.com/uber/npm-shrinkwrap/blob/d589a48ffddc40769ef550e568cf661bbe26bf55/set-resolved.js#L74-L76 ).

Just make sure you always set resolved.

Also note that npm-shrinkwrap is mostly for old npm.

@iarna iarna added this to the multi-stage install milestone Apr 18, 2015

@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Apr 18, 2015

Member

I'm taking this over to multi-stage because it turns it out it was super easy to add a constant from field there. So yay! 😁 See 78a8b99 for the _from part.

Member

iarna commented Apr 18, 2015

I'm taking this over to multi-stage because it turns it out it was super easy to add a constant from field there. So yay! 😁 See 78a8b99 for the _from part.

@iarna iarna modified the milestones: cache rewrite, multi-stage install Apr 18, 2015

iarna added a commit that referenced this issue Apr 18, 2015

@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Apr 18, 2015

Member

And fixed the rest of the way with a781cb7 in npm/npm#multi-stage

Member

iarna commented Apr 18, 2015

And fixed the rest of the way with a781cb7 in npm/npm#multi-stage

@iarna iarna closed this Apr 18, 2015

iarna added a commit that referenced this issue Apr 18, 2015

iarna added a commit that referenced this issue Apr 18, 2015

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Apr 18, 2015

Thanks @iarna! looking forward to when 3 drops.

mixonic commented Apr 18, 2015

Thanks @iarna! looking forward to when 3 drops.

iarna added a commit that referenced this issue Apr 20, 2015

@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Apr 20, 2015

Member

Ended up decoupling shrinkwrap from npm ls in 9f01098, as npm ls now shows you the "logical tree" and not what's literally on disk.

Member

iarna commented Apr 20, 2015

Ended up decoupling shrinkwrap from npm ls in 9f01098, as npm ls now shows you the "logical tree" and not what's literally on disk.

iarna added a commit that referenced this issue Apr 23, 2015

iarna added a commit that referenced this issue Apr 27, 2015

iarna added a commit that referenced this issue Apr 27, 2015

iarna added a commit that referenced this issue May 16, 2015

iarna added a commit that referenced this issue May 22, 2015

iarna added a commit that referenced this issue Jun 1, 2015

iarna added a commit that referenced this issue Jun 10, 2015

iarna added a commit that referenced this issue Jun 12, 2015

iarna added a commit that referenced this issue Jun 21, 2015

@npm npm locked and limited conversation to collaborators Jun 24, 2015

iarna added a commit that referenced this issue Jun 26, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.