Make install order more consistent for directly dependencies #9274

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@iarna
Member
iarna commented Aug 14, 2015

The install order is determined by:

  1. The location of the thing to be installed relative to the root module.
  2. Dependency order, such that a -> b -> c will install modules as: c, b, a

1 is deterministic, regardless of what's being installed.

But 2 can change the order of things higher in the dep tree. Eg, b, or a
might get sorted earlier if c requires them. This mostly doesn't matter,
but it does mean that if you have two modules with conflicting bins, they
can get installed in different orders. This changes sorts all of the top
level modules to be LAST, in location order (1), and then sorts all the rest
per (2). This ensures that top level modules have a deterministic install
order. (Non top level modules can't have bin conflicts as that's treated
the same as a version conflict and the conflicting module would be hoisted.)

Fixes #8995

  • Needs a test
@iarna iarna added the in progress label Aug 14, 2015
@iarna iarna added this to the 3.x-next-next milestone Aug 14, 2015
@iarna iarna changed the title from diff-trees: Make install order more consistent for directly dependencies to Make install order more consistent for directly dependencies Aug 19, 2015
@iarna iarna commented on an outdated diff Aug 19, 2015
lib/install/diff-trees.js
@@ -68,7 +68,21 @@ var sortActions = module.exports.sortActions = function (differences) {
var sorted = []
var added = {}
- Object.keys(actions).sort(sortByLocation).forEach(function (location) {
+
+ var sortedlocs = Object.keys(actions).sort(sortByLocation)
+
+ // Do top level deps first, this stops the sorting by required order from
+ // unsorting these deps.
+ var toplocs = sortedlocs.filter(function (location) {
+ var mod = actions[location][1]
+ if (!mod.package._requiredBy) return true
+ var deepDeps = mod.package._requiredBy.filter(function (req) {
+ return req !== '/' && req[0] !== '#'
@iarna
iarna Aug 19, 2015 Member

put in function

@othiym23
Contributor

Reviewed by me over ScreenHero, with one small suggestion to improve comprehensibility. 🐑

@iarna iarna added a commit that referenced this pull request Aug 20, 2015
@iarna iarna diff-trees: Make install order more consistent for directly dependencies
The install order is determined by:

1) The location of the thing to be installed relative to the root module.
2) Dependency order, such that a -> b -> c will install modules as: c, b, a

1 is deterministic, regardless of what's being installed.

But 2 can change the order of things higher in the dep tree.  Eg, b, or a
might get sorted earlier if c requires them.  This mostly doesn't matter,
but it does mean that if you have two modules with conflicting bins, they
_can_ get installed in different orders.  This changes sorts all of the top
level modules to be LAST, in location order (1), and then sorts all the rest
per (2).  This ensures that top level modules have a deterministic install
order. (Non top level modules can't have bin conflicts as that's treated
the same as a version conflict and the conflicting module would be hoisted.)

PR-URL: #9274
Fixes: #8995
ac33031
@iarna iarna added a commit that referenced this pull request Aug 21, 2015
@iarna iarna diff-trees: Make install order more consistent for directly dependencies
The install order is determined by:

1) The location of the thing to be installed relative to the root module.
2) Dependency order, such that a -> b -> c will install modules as: c, b, a

1 is deterministic, regardless of what's being installed.

But 2 can change the order of things higher in the dep tree.  Eg, b, or a
might get sorted earlier if c requires them.  This mostly doesn't matter,
but it does mean that if you have two modules with conflicting bins, they
_can_ get installed in different orders.  This changes sorts all of the top
level modules to be LAST, in location order (1), and then sorts all the rest
per (2).  This ensures that top level modules have a deterministic install
order. (Non top level modules can't have bin conflicts as that's treated
the same as a version conflict and the conflicting module would be hoisted.)

PR-URL: #9274
Fixes: #8995
f277c71
@iarna iarna added ready and removed in progress labels Aug 21, 2015
@iarna iarna commented on an outdated diff Aug 21, 2015
lib/install/diff-trees.js
}
function sortByDeps (action) {
- var cmd = action[1]
- if (added[cmd.package._location]) return
- added[cmd.package._location] = action
- cmd.package._requiredBy.forEach(function (location) {
+ var mod = action[1]
+ if (added[mod.package._location]) return
+ added[mod.package._location] = action
+ mod.package._requiredBy.forEach(function (location) {
@iarna
iarna Aug 21, 2015 Member

sort _requiredBy

@iarna iarna diff-trees: Make install order more consistent for directly dependencies
The install order is determined by:

1) The location of the thing to be installed relative to the root module.
2) Dependency order, such that a -> b -> c will install modules as: c, b, a

1 is deterministic, regardless of what's being installed.

But 2 can change the order of things higher in the dep tree.  Eg, b, or a
might get sorted earlier if c requires them.  This mostly doesn't matter,
but it does mean that if you have two modules with conflicting bins, they
_can_ get installed in different orders.  This changes sorts all of the top
level modules to be LAST, in location order (1), and then sorts all the rest
per (2).  This ensures that top level modules have a deterministic install
order. (Non top level modules can't have bin conflicts as that's treated
the same as a version conflict and the conflicting module would be hoisted.)
774df20
@iarna iarna added a commit that referenced this pull request Aug 21, 2015
@iarna iarna diff-trees: Make install order more consistent for directly dependencies
The install order is determined by:

1) The location of the thing to be installed relative to the root module.
2) Dependency order, such that a -> b -> c will install modules as: c, b, a

1 is deterministic, regardless of what's being installed.

But 2 can change the order of things higher in the dep tree.  Eg, b, or a
might get sorted earlier if c requires them.  This mostly doesn't matter,
but it does mean that if you have two modules with conflicting bins, they
_can_ get installed in different orders.  This changes sorts all of the top
level modules to be LAST, in location order (1), and then sorts all the rest
per (2).  This ensures that top level modules have a deterministic install
order. (Non top level modules can't have bin conflicts as that's treated
the same as a version conflict and the conflicting module would be hoisted.)

PR-URL: #9274
Fixes: #8995
d395a6b
@othiym23
Contributor

Landed as d395a6b. Consistency is one of our top-level goals, so this is a great thing to get in. Thanks once more, Rebecca!

@othiym23 othiym23 closed this Aug 28, 2015
@iarna iarna deleted the iarna/make-install-order-more-consistent branch Sep 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment