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

lib/install/diff-trees.js

@@ -59,6 +59,12 @@ function requiredByAllLinked (node) {
   return node.requiredBy.filter(isLink).length === node.requiredBy.length
 }
 
+function isNotReqByTop (req) {
+  return req !== '/' &&     // '/' is the top level itself
+         req !== '#USER' && // #USER
+         req !== '#EXTRANEOUS'
+}
+
 var sortActions = module.exports.sortActions = function (differences) {
   var actions = {}
   differences.forEach(function (action) {
@@ -68,22 +74,36 @@ 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
+    // If the module is required by ANY non-top level package
+    // then we don't want to include this.
+    return !mod.package._requiredBy.some(isNotReqByTop)
+  })
+
+  toplocs.concat(sortedlocs).forEach(function (location) {
     sortByDeps(actions[location])
   })
 
   function sortByLocation (aa, bb) {
-    return aa.length - bb.length || bb.localeCompare(aa)
+    return bb.localeCompare(aa)
   }
   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.sort().forEach(function (location) {
       if (actions[location]) sortByDeps(actions[location])
     })
     sorted.unshift(action)
   }
+
   return sorted
 }
 

test/tap/dedupe-scoped.js

@@ -12,10 +12,10 @@ var modules = join(pkg, 'node_modules')
 var EXEC_OPTS = { cwd: pkg }
 
 var body = function () {/*
-- @scope/shared@2.1.6 node_modules/second/node_modules/@scope/shared
 @scope/shared@2.1.6 node_modules/first/node_modules/@scope/shared -> node_modules/@scope/shared
-secondUnique@1.2.0 node_modules/second/node_modules/secondUnique -> node_modules/secondUnique
 firstUnique@0.6.0 node_modules/first/node_modules/firstUnique -> node_modules/firstUnique
+secondUnique@1.2.0 node_modules/second/node_modules/secondUnique -> node_modules/secondUnique
+- @scope/shared@2.1.6 node_modules/second/node_modules/@scope/shared
 */}.toString().split('\n').slice(1, -1)
 
 var deduper = {

test/tap/install-order.js

@@ -0,0 +1,37 @@
+'use strict'
+var test = require('tap').test
+var sortActions = require('../../lib/install/diff-trees.js').sortActions
+
+var a = {
+  package: {_location: '/a', _requiredBy: []}
+}
+var b = {
+  package: {_location: '/b', _requiredBy: []}
+}
+var c = {
+  package: {_location: '/c', _requiredBy: ['/a', '/b']}
+}
+
+test('install-order when installing deps', function (t) {
+  var plain = [
+    ['add', a],
+    ['add', b],
+    ['add', c]]
+  var sorted = [
+    ['add', c],
+    ['add', a],
+    ['add', b]]
+  t.isDeeply(sortActions(plain), sorted)
+  t.end()
+})
+
+test('install-order when not installing deps', function (t) {
+  var plain = [
+    ['add', a],
+    ['add', b]]
+  var sorted = [
+    ['add', a],
+    ['add', b]]
+  t.isDeeply(sortActions(plain), sorted)
+  t.end()
+})