From 5dfd0a17021786225772f0812c5373a7c8806961 Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 21 Jan 2021 12:27:24 -0800 Subject: [PATCH] properly buildIdealTree when root is a symlink PR-URL: https://github.com/npm/arborist/pull/210 Credit: @nlf Close: #210 Reviewed-by: @isaacs --- .gitignore | 1 + lib/arborist/build-ideal-tree.js | 56 ++++-- lib/arborist/reify.js | 5 +- ...t-arborist-build-ideal-tree.js-TAP.test.js | 162 ++++++++++++++++++ test/arborist/build-ideal-tree.js | 10 ++ test/fixtures/index.js | 1 + 6 files changed, 219 insertions(+), 16 deletions(-) diff --git a/.gitignore b/.gitignore index 5858f28ef..a1fa3f3ce 100644 --- a/.gitignore +++ b/.gitignore @@ -56,6 +56,7 @@ scripts/benchmark/*/ /test/fixtures/selflink/node_modules/foo/node_modules/selflink /test/fixtures/symlinked-node-modules/example/node_modules /test/fixtures/symlinked-node-modules/linked-node-modules/bar +/test/fixtures/testing-peer-deps-link /test/fixtures/workspace/node_modules/a /test/fixtures/workspace/node_modules/b /test/fixtures/workspace/node_modules/c diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js index c1f18af7e..ac35209e5 100644 --- a/lib/arborist/build-ideal-tree.js +++ b/lib/arborist/build-ideal-tree.js @@ -332,20 +332,28 @@ module.exports = cls => class IdealTreeBuilder extends cls { }) } - [_globalRootNode] () { - const root = this[_rootNodeFromPackage]({ dependencies: {} }) + async [_globalRootNode] () { + const root = await this[_rootNodeFromPackage]({ dependencies: {} }) // this is a gross kludge to handle the fact that we don't save // metadata on the root node in global installs, because the "root" // node is something like /usr/local/lib. const meta = new Shrinkwrap({ path: this.path }) meta.reset() root.meta = meta - return Promise.resolve(root) + return root } - [_rootNodeFromPackage] (pkg) { - return new Node({ + async [_rootNodeFromPackage] (pkg) { + // if the path doesn't exist, then we explode at this point. Note that + // this is not a problem for reify(), since it creates the root path + // before ever loading trees. + // TODO: make buildIdealTree() and loadActual handle a missing root path, + // or a symlink to a missing target, and let reify() create it as needed. + const real = await realpath(this.path, this[_rpcache], this[_stcache]) + const Cls = real === this.path ? Node : Link + const root = new Cls({ path: this.path, + realpath: real, pkg, extraneous: false, dev: false, @@ -355,12 +363,29 @@ module.exports = cls => class IdealTreeBuilder extends cls { global: this[_global], legacyPeerDeps: this.legacyPeerDeps, }) + if (root.isLink) { + root.target = new Node({ + path: real, + realpath: real, + pkg, + extraneous: false, + dev: false, + devOptional: false, + peer: false, + optional: false, + global: this[_global], + legacyPeerDeps: this.legacyPeerDeps, + root, + }) + } + return root } // process the add/rm requests by modifying the root node, and the // update.names request by queueing nodes dependent on those named. async [_applyUserRequests] (options) { process.emit('time', 'idealTree:userRequests') + const tree = this.idealTree.target || this.idealTree // If we have a list of package names to update, and we know it's // going to update them wherever they are, add any paths into those // named nodes to the buildIdealTree queue. @@ -373,7 +398,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { const nm = resolve(this.path, 'node_modules') for (const name of await readdir(nm)) { if (this[_updateAll] || this[_updateNames].includes(name)) - this.idealTree.package.dependencies[name] = '*' + tree.package.dependencies[name] = '*' } } @@ -381,7 +406,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { this[_queueVulnDependents](options) if (options.rm && options.rm.length) { - addRmPkgDeps.rm(this.idealTree.package, options.rm) + addRmPkgDeps.rm(tree.package, options.rm) for (const name of options.rm) this[_explicitRequests].add(name) } @@ -391,7 +416,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { // triggers a refresh of all edgesOut if (options.add && options.add.length || options.rm && options.rm.length) - this.idealTree.package = this.idealTree.package + tree.package = tree.package process.emit('timeEnd', 'idealTree:userRequests') } @@ -410,8 +435,9 @@ module.exports = cls => class IdealTreeBuilder extends cls { this[_resolvedAdd] = add // now add is a list of spec objects with names. // find a home for each of them! + const tree = this.idealTree.target || this.idealTree addRmPkgDeps.add({ - pkg: this.idealTree.package, + pkg: tree.package, add, saveBundle, saveType, @@ -514,7 +540,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { fixAvailable, } = topVuln for (const node of topNodes) { - if (node !== this.idealTree) { + if (node !== this.idealTree && node !== this.idealTree.target) { // not something we're going to fix, sorry. have to cd into // that directory and fix it yourself. this.log.warn('audit', 'Manual fix required in linked project ' + @@ -646,11 +672,12 @@ This is a one-time fix-up, please be patient... // at this point we have a virtual tree with the actual root node's // package deps, which may be partly or entirely incomplete, invalid // or extraneous. - [_buildDeps] (node) { + [_buildDeps] () { process.emit('time', 'idealTree:buildDeps') - this[_depsQueue].push(this.idealTree) + const tree = this.idealTree.target || this.idealTree + this[_depsQueue].push(tree) this.log.silly('idealTree', 'buildDeps') - this.addTracker('idealTree', this.idealTree.name, '') + this.addTracker('idealTree', tree.name, '') return this[_buildDepStep]() .then(() => process.emit('timeEnd', 'idealTree:buildDeps')) } @@ -1137,7 +1164,8 @@ This is a one-time fix-up, please be patient... // when installing globally, or just in global style, we never place // deps above the first level. - if (this[_globalStyle] && check.resolveParent === this.idealTree) + const tree = this.idealTree && this.idealTree.target || this.idealTree + if (this[_globalStyle] && check.resolveParent === tree) break } diff --git a/lib/arborist/reify.js b/lib/arborist/reify.js index d916b49c2..79ae390d3 100644 --- a/lib/arborist/reify.js +++ b/lib/arborist/reify.js @@ -183,8 +183,9 @@ module.exports = cls => class Reifier extends cls { const actualOpt = this[_global] ? { ignoreMissing: true, global: true, - filter: (node, kid) => !node.isRoot ? true - : (node.edgesOut.has(kid) || this[_explicitRequests].has(kid)), + filter: (node, kid) => !node.isRoot && node !== node.root.target + ? true + : (node.edgesOut.has(kid) || this[_explicitRequests].has(kid)), } : { ignoreMissing: true } if (!this[_global]) { diff --git a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js index cb3c96ebb..f917662ec 100644 --- a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js +++ b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js @@ -123635,6 +123635,168 @@ ArboristNode { } ` +exports[`test/arborist/build-ideal-tree.js TAP testing-peer-deps package with symlinked root > build ideal tree with peer deps 1`] = ` +ArboristLink { + "location": "../testing-peer-deps-link", + "name": "testing-peer-deps-link", + "packageName": "@isaacs/testing-peer-deps", + "path": "{CWD}/test/fixtures/testing-peer-deps-link", + "realpath": "{CWD}/test/fixtures/testing-peer-deps", + "resolved": "file:testing-peer-deps", + "target": ArboristNode { + "children": Map { + "@isaacs/testing-peer-deps-b" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "@isaacs/testing-peer-deps-b", + "spec": "1", + "type": "prod", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-deps-c" => EdgeOut { + "name": "@isaacs/testing-peer-deps-c", + "spec": "1", + "to": "node_modules/@isaacs/testing-peer-deps-c", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-deps-b", + "name": "@isaacs/testing-peer-deps-b", + "path": "{CWD}/test/fixtures/testing-peer-deps/node_modules/@isaacs/testing-peer-deps-b", + "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-b/-/testing-peer-deps-b-1.2.4.tgz", + "version": "1.2.4", + }, + "@isaacs/testing-peer-deps-c" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "node_modules/@isaacs/testing-peer-deps-b", + "name": "@isaacs/testing-peer-deps-c", + "spec": "1", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-deps-c", + "name": "@isaacs/testing-peer-deps-c", + "path": "{CWD}/test/fixtures/testing-peer-deps/node_modules/@isaacs/testing-peer-deps-c", + "peer": true, + "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-c/-/testing-peer-deps-c-1.2.3.tgz", + "version": "1.2.3", + }, + "@isaacs/testing-peer-deps-d" => ArboristNode { + "children": Map { + "@isaacs/testing-peer-deps-a" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "node_modules/@isaacs/testing-peer-deps-d", + "name": "@isaacs/testing-peer-deps-a", + "spec": "2", + "type": "prod", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-deps-b" => EdgeOut { + "name": "@isaacs/testing-peer-deps-b", + "spec": "2", + "to": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-b", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-a", + "name": "@isaacs/testing-peer-deps-a", + "path": "{CWD}/test/fixtures/testing-peer-deps/node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-a", + "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-a/-/testing-peer-deps-a-2.0.0.tgz", + "version": "2.0.0", + }, + "@isaacs/testing-peer-deps-b" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-a", + "name": "@isaacs/testing-peer-deps-b", + "spec": "2", + "type": "peer", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-deps-c" => EdgeOut { + "name": "@isaacs/testing-peer-deps-c", + "spec": "2", + "to": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-c", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-b", + "name": "@isaacs/testing-peer-deps-b", + "path": "{CWD}/test/fixtures/testing-peer-deps/node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-b", + "peer": true, + "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-b/-/testing-peer-deps-b-2.0.1.tgz", + "version": "2.0.1", + }, + "@isaacs/testing-peer-deps-c" => ArboristNode { + "edgesIn": Set { + EdgeIn { + "from": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-b", + "name": "@isaacs/testing-peer-deps-c", + "spec": "2", + "type": "peer", + }, + }, + "location": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-c", + "name": "@isaacs/testing-peer-deps-c", + "path": "{CWD}/test/fixtures/testing-peer-deps/node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-c", + "peer": true, + "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-c/-/testing-peer-deps-c-2.0.0.tgz", + "version": "2.0.0", + }, + }, + "edgesIn": Set { + EdgeIn { + "from": "", + "name": "@isaacs/testing-peer-deps-d", + "spec": "2", + "type": "prod", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-deps-a" => EdgeOut { + "name": "@isaacs/testing-peer-deps-a", + "spec": "2", + "to": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-a", + "type": "prod", + }, + }, + "location": "node_modules/@isaacs/testing-peer-deps-d", + "name": "@isaacs/testing-peer-deps-d", + "path": "{CWD}/test/fixtures/testing-peer-deps/node_modules/@isaacs/testing-peer-deps-d", + "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-d/-/testing-peer-deps-d-2.0.0.tgz", + "version": "2.0.0", + }, + }, + "edgesOut": Map { + "@isaacs/testing-peer-deps-b" => EdgeOut { + "name": "@isaacs/testing-peer-deps-b", + "spec": "1", + "to": "node_modules/@isaacs/testing-peer-deps-b", + "type": "prod", + }, + "@isaacs/testing-peer-deps-d" => EdgeOut { + "name": "@isaacs/testing-peer-deps-d", + "spec": "2", + "to": "node_modules/@isaacs/testing-peer-deps-d", + "type": "prod", + }, + }, + "location": "", + "name": "testing-peer-deps", + "packageName": "@isaacs/testing-peer-deps", + "path": "{CWD}/test/fixtures/testing-peer-deps", + "version": "2.0.0", + }, + "version": "2.0.0", +} +` + exports[`test/arborist/build-ideal-tree.js TAP update flow outdated > update everything 1`] = ` ArboristNode { "children": Map { diff --git a/test/arborist/build-ideal-tree.js b/test/arborist/build-ideal-tree.js index c3ab8bd27..568b22ff6 100644 --- a/test/arborist/build-ideal-tree.js +++ b/test/arborist/build-ideal-tree.js @@ -152,6 +152,16 @@ t.test('testing-peer-deps package', t => { .then(() => t.matchSnapshot(printTree(idealTree), 'build ideal tree with peer deps'))) }) +t.test('testing-peer-deps package with symlinked root', t => { + const path = resolve(fixtures, 'testing-peer-deps-link') + return buildIdeal(path).then(idealTree => { + t.ok(idealTree.isLink, 'ideal tree is rooted on a Link') + return new Arborist({path, idealTree, ...OPT}) + .buildIdealTree().then(tree2 => t.equal(tree2, idealTree)) + .then(() => t.matchSnapshot(printTree(idealTree), 'build ideal tree with peer deps')) + }) +}) + t.test('testing-peer-deps nested', t => { const path = resolve(fixtures, 'testing-peer-deps-nested') return t.resolveMatchSnapshot(printIdeal(path), 'build ideal tree') diff --git a/test/fixtures/index.js b/test/fixtures/index.js index 9dcb688fe..d2b7accaa 100644 --- a/test/fixtures/index.js +++ b/test/fixtures/index.js @@ -47,6 +47,7 @@ const symlinks = { 'deeproot': 'deep', 'badlink/node_modules/foo': 'foo', 'badlink/node_modules/bar': 'baz', + 'testing-peer-deps-link': 'testing-peer-deps', 'workspace/node_modules/a': '../packages/a', 'workspace/node_modules/b': '../packages/b',