From 2fc15258e6ffdefed8a9fd30816e8a10e61c1638 Mon Sep 17 00:00:00 2001
From: isaacs <i@izs.me>
Date: Mon, 1 Feb 2021 10:20:04 -0800
Subject: [PATCH] Install bins properly when global root is a link

Also fixes up a few other places where we were checking isRoot and
should have been checking isProjectRoot.

Fix: https://github.com/nodejs/build/issues/2525

PR-URL: https://github.com/npm/arborist/pull/217
Credit: @isaacs
Close: #217
Reviewed-by: @nlf
---
 lib/arborist/reify.js    |  4 ++--
 lib/node.js              | 21 ++++++++++++---------
 test/arborist/rebuild.js | 31 +++++++++++++++++++++++++++++++
 test/node.js             | 18 ++++++++++++++++++
 4 files changed, 63 insertions(+), 11 deletions(-)

diff --git a/lib/arborist/reify.js b/lib/arborist/reify.js
index 34c1dbb18..6cc129a7c 100644
--- a/lib/arborist/reify.js
+++ b/lib/arborist/reify.js
@@ -605,7 +605,7 @@ module.exports = cls => class Reifier extends cls {
       tree: this.diff,
       visit: diff => {
         const node = diff.ideal
-        if (node && !node.isRoot && node.package.bundleDependencies &&
+        if (node && !node.isProjectRoot && node.package.bundleDependencies &&
             node.package.bundleDependencies.length) {
           maxBundleDepth = Math.max(maxBundleDepth, node.depth)
           if (!bundlesByDepth.has(node.depth))
@@ -811,7 +811,7 @@ module.exports = cls => class Reifier extends cls {
     dfwalk({
       tree: this.diff,
       leave: diff => {
-        if (!diff.ideal.isRoot)
+        if (!diff.ideal.isProjectRoot)
           nodes.push(diff.ideal)
       },
       // process adds before changes, ignore removals
diff --git a/lib/node.js b/lib/node.js
index 13c89c143..9a6b86e40 100644
--- a/lib/node.js
+++ b/lib/node.js
@@ -247,7 +247,7 @@ class Node {
 
   // true for packages installed directly in the global node_modules folder
   get globalTop () {
-    return this.global && this.parent.isRoot
+    return this.global && this.parent.isProjectRoot
   }
 
   get workspaces () {
@@ -294,8 +294,11 @@ class Node {
     const { name = '', version = '' } = this.package
     // root package will prefer package name over folder name,
     // and never be called an alias.
-    const myname = this.isRoot ? name || this.name : this.name
-    const alias = !this.isRoot && name && myname !== name ? `npm:${name}@` : ''
+    const { isProjectRoot } = this
+    const myname = isProjectRoot ? name || this.name
+      : this.name
+    const alias = !isProjectRoot && name && myname !== name ? `npm:${name}@`
+      : ''
     return `${myname}@${alias}${version}`
   }
 
@@ -339,14 +342,14 @@ class Node {
   }
 
   [_explain] (edge, seen) {
-    if (this.isRoot && !this.sourceReference) {
+    if (this.isProjectRoot && !this.sourceReference) {
       return {
         location: this.path,
       }
     }
 
     const why = {
-      name: this.isRoot ? this.package.name : this.name,
+      name: this.isProjectRoot ? this.package.name : this.name,
       version: this.package.version,
     }
     if (this.errors.length || !this.package.name || !this.package.version) {
@@ -384,7 +387,7 @@ class Node {
       // and are not keeping it held in this spot anyway.
       const edges = []
       for (const edge of this.edgesIn) {
-        if (!edge.valid && !edge.from.isRoot)
+        if (!edge.valid && !edge.from.isProjectRoot)
           continue
 
         edges.push(edge)
@@ -453,7 +456,7 @@ class Node {
   }
 
   get isWorkspace () {
-    if (this.isRoot)
+    if (this.isProjectRoot)
       return false
     const { root } = this
     const { type, to } = root.edgesOut.get(this.package.name) || {}
@@ -904,8 +907,8 @@ class Node {
     if (this.isLink)
       return node.isLink && this.target.matches(node.target)
 
-    // if they're two root nodes, they're different if the paths differ
-    if (this.isRoot && node.isRoot)
+    // if they're two project root nodes, they're different if the paths differ
+    if (this.isProjectRoot && node.isProjectRoot)
       return this.path === node.path
 
     // if the integrity matches, then they're the same.
diff --git a/test/arborist/rebuild.js b/test/arborist/rebuild.js
index 423148c4c..5ec42d9dc 100644
--- a/test/arborist/rebuild.js
+++ b/test/arborist/rebuild.js
@@ -610,3 +610,34 @@ t.test('workspaces', async t => {
     )
   })
 })
+
+t.test('put bins in the right place for linked-global top pkgs', async t => {
+  const path = t.testdir({
+    lib: t.fixture('symlink', 'target'),
+    target: {
+      node_modules: {
+        foo: {
+          'package.json': JSON.stringify({
+            name: 'foo',
+            version: '1.2.3',
+            bin: 'foo',
+          }),
+          foo: 'the bin script',
+        },
+      },
+    },
+  })
+  const binpath = resolve(path, isWindows ? 'lib' : 'bin')
+  const arb = new Arborist({ path: path + '/lib', registry, global: true })
+  await arb.rebuild()
+  const expect = isWindows ? [
+    'foo',
+    'foo.cmd',
+    'foo.ps1',
+  ] : ['foo']
+  const test = isWindows ? 'isFile' : 'isSymbolicLink'
+  for (const f of expect) {
+    const p = resolve(binpath, f)
+    t.equal(fs.lstatSync(p)[test](), true, `${test} ${binpath}/${f}`)
+  }
+})
diff --git a/test/node.js b/test/node.js
index 4f5f8e34d..69edab7f2 100644
--- a/test/node.js
+++ b/test/node.js
@@ -2163,3 +2163,21 @@ t.test('virtual references to root node has devDep edges', async t => {
   })
   t.equal(virtualRoot.edgesOut.get('a').type, 'dev')
 })
+
+t.test('globaTop set for children of global link root target', async t => {
+  const root = new Link({
+    path: '/usr/local/lib',
+    realpath: '/data/lib',
+    global: true,
+  })
+  root.target = new Node({
+    path: '/data/lib',
+    global: true,
+    root,
+  })
+  const gtop = new Node({
+    parent: root.target,
+    pkg: { name: 'foo', version: '1.2.3' },
+  })
+  t.equal(gtop.globalTop, true)
+})