From 5a50762faa37ae5964ae6f12595b20b367056c0a Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Tue, 10 May 2022 14:19:04 -0400 Subject: [PATCH] fix(arborist): link deps lifecycle scripts (#4875) - Fixes running proper lifecycle scripts for linked deps and workspaces. - Added test to validate lifecycle scripts don't run twice for linked deps - Tweaked "reify workspaces bin files" test to also validate proper lifecycle scripts ran before check for linked bins. - Tweaked reify test running lifecycle scripts of unchanged link nodes to also validate that the install lifecycle scripts are also called. Fixes: https://github.com/npm/cli/issues/4277 Fixes: https://github.com/npm/cli/issues/4552 Fixes: https://github.com/npm/statusboard/issues/439 Relates to: https://github.com/npm/cli/issues/2905 --- workspaces/arborist/lib/arborist/rebuild.js | 117 +++++++++++------- workspaces/arborist/lib/arborist/reify.js | 3 +- workspaces/arborist/test/arborist/rebuild.js | 45 +++++++ workspaces/arborist/test/arborist/reify.js | 2 + .../link-dep-lifecycle-scripts/a/package.json | 3 +- .../reify-cases/link-dep-lifecycle-scripts.js | 3 +- .../reify-cases/workspaces-link-bin.js | 5 +- .../packages/b/create-file.js | 1 + .../workspaces-link-bin/packages/b/file.js | 0 .../packages/b/package.json | 3 + 10 files changed, 136 insertions(+), 46 deletions(-) create mode 100644 workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/create-file.js delete mode 100644 workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/file.js diff --git a/workspaces/arborist/lib/arborist/rebuild.js b/workspaces/arborist/lib/arborist/rebuild.js index 8b47904004a96..e9b79031ef427 100644 --- a/workspaces/arborist/lib/arborist/rebuild.js +++ b/workspaces/arborist/lib/arborist/rebuild.js @@ -21,6 +21,8 @@ const sortNodes = (a, b) => const _workspaces = Symbol.for('workspaces') const _build = Symbol('build') +const _loadDefaultNodes = Symbol('loadDefaultNodes') +const _retrieveNodesByType = Symbol('retrieveNodesByType') const _resetQueues = Symbol('resetQueues') const _rebuildBundle = Symbol('rebuildBundle') const _ignoreScripts = Symbol('ignoreScripts') @@ -39,6 +41,7 @@ const _includeWorkspaceRoot = Symbol.for('includeWorkspaceRoot') const _workspacesEnabled = Symbol.for('workspacesEnabled') const _force = Symbol.for('force') +const _global = Symbol.for('global') // defined by reify mixin const _handleOptionalFailure = Symbol.for('handleOptionalFailure') @@ -75,36 +78,60 @@ module.exports = cls => class Builder extends cls { // running JUST a rebuild, we treat optional failures as real fails this[_doHandleOptionalFailure] = handleOptionalFailure - // if we don't have a set of nodes, then just rebuild - // the actual tree on disk. if (!nodes) { - const tree = await this.loadActual() - let filterSet - if (!this[_workspacesEnabled]) { - filterSet = this.excludeWorkspacesDependencySet(tree) - nodes = tree.inventory.filter(node => - filterSet.has(node) || node.isProjectRoot - ) - } else if (this[_workspaces] && this[_workspaces].length) { - filterSet = this.workspaceDependencySet( - tree, - this[_workspaces], - this[_includeWorkspaceRoot] - ) - nodes = tree.inventory.filter(node => filterSet.has(node)) - } else { - nodes = tree.inventory.values() - } + nodes = await this[_loadDefaultNodes]() } // separates links nodes so that it can run // prepare scripts and link bins in the expected order process.emit('time', 'build') + + const { + depNodes, + linkNodes, + } = this[_retrieveNodesByType](nodes) + + // build regular deps + await this[_build](depNodes, {}) + + // build link deps + if (linkNodes.size) { + this[_resetQueues]() + await this[_build](linkNodes, { type: 'links' }) + } + + process.emit('timeEnd', 'build') + } + + // if we don't have a set of nodes, then just rebuild + // the actual tree on disk. + async [_loadDefaultNodes] () { + let nodes + const tree = await this.loadActual() + let filterSet + if (!this[_workspacesEnabled]) { + filterSet = this.excludeWorkspacesDependencySet(tree) + nodes = tree.inventory.filter(node => + filterSet.has(node) || node.isProjectRoot + ) + } else if (this[_workspaces] && this[_workspaces].length) { + filterSet = this.workspaceDependencySet( + tree, + this[_workspaces], + this[_includeWorkspaceRoot] + ) + nodes = tree.inventory.filter(node => filterSet.has(node)) + } else { + nodes = tree.inventory.values() + } + return nodes + } + + [_retrieveNodesByType] (nodes) { const depNodes = new Set() const linkNodes = new Set() + for (const node of nodes) { - // we skip the target nodes to that workspace in order to make sure - // we only run lifecycle scripts / place bin links once per workspace if (node.isLink) { linkNodes.add(node) } else { @@ -112,14 +139,22 @@ module.exports = cls => class Builder extends cls { } } - await this[_build](depNodes, {}) - - if (linkNodes.size) { - this[_resetQueues]() - await this[_build](linkNodes, { type: 'links' }) + // deduplicates link nodes and their targets, avoids + // calling lifecycle scripts twice when running `npm rebuild` + // ref: https://github.com/npm/cli/issues/2905 + // + // we avoid doing so if global=true since `bin-links` relies + // on having the target nodes available in global mode. + if (!this[_global]) { + for (const node of linkNodes) { + depNodes.delete(node.target) + } } - process.emit('timeEnd', 'build') + return { + depNodes, + linkNodes, + } } [_resetQueues] () { @@ -136,24 +171,22 @@ module.exports = cls => class Builder extends cls { process.emit('time', `build:${type}`) await this[_buildQueues](nodes) + + if (!this[_ignoreScripts]) { + await this[_runScripts]('preinstall') + } + // links should run prepare scripts and only link bins after that - if (type !== 'links') { - if (!this[_ignoreScripts]) { - await this[_runScripts]('preinstall') - } - if (this[_binLinks]) { - await this[_linkAllBins]() - } - if (!this[_ignoreScripts]) { - await this[_runScripts]('install') - await this[_runScripts]('postinstall') - } - } else { + if (type === 'links') { await this[_runScripts]('prepare') + } + if (this[_binLinks]) { + await this[_linkAllBins]() + } - if (this[_binLinks]) { - await this[_linkAllBins]() - } + if (!this[_ignoreScripts]) { + await this[_runScripts]('install') + await this[_runScripts]('postinstall') } process.emit('timeEnd', `build:${type}`) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 7fd0ca7f60740..4932c17d03667 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -1105,7 +1105,8 @@ module.exports = cls => class Reifier extends cls { // skip links that only live within node_modules as they are most // likely managed by packages we installed, we only want to rebuild // unchanged links we directly manage - if (node.isLink && node.target.fsTop === tree) { + const linkedFromRoot = node.parent === tree || node.target.fsTop === tree + if (node.isLink && linkedFromRoot) { nodes.push(node) } } diff --git a/workspaces/arborist/test/arborist/rebuild.js b/workspaces/arborist/test/arborist/rebuild.js index e75895628c289..37551c7483386 100644 --- a/workspaces/arborist/test/arborist/rebuild.js +++ b/workspaces/arborist/test/arborist/rebuild.js @@ -459,6 +459,51 @@ t.test('do not rebuild node-gyp dependencies with gypfile:false', async t => { await arb.rebuild() }) +// ref: https://github.com/npm/cli/issues/2905 +t.test('do not run lifecycle scripts of linked deps twice', async t => { + const testdir = t.testdir({ + project: { + 'package.json': JSON.stringify({ + name: 'my-project', + version: '1.0.0', + dependencies: { + foo: 'file:../foo', + }, + }), + node_modules: { + foo: t.fixture('symlink', '../../foo'), + }, + }, + foo: { + 'package.json': JSON.stringify({ + name: 'foo', + version: '1.0.0', + scripts: { + postinstall: 'echo "ok"', + }, + }), + }, + }) + + const path = resolve(testdir, 'project') + const RUNS = [] + const Arborist = t.mock('../../lib/arborist/index.js', { + '@npmcli/run-script': opts => { + RUNS.push(opts) + return require('@npmcli/run-script')(opts) + }, + }) + const arb = new Arborist({ path, registry }) + await arb.rebuild() + t.equal(RUNS.length, 1, 'should run postinstall script only once') + t.match(RUNS, [ + { + event: 'postinstall', + pkg: { name: 'foo' }, + }, + ]) +}) + t.test('workspaces', async t => { const path = t.testdir({ 'package.json': JSON.stringify({ diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index 5097004041174..a77b0fbb2c499 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -1770,6 +1770,8 @@ t.test('running lifecycle scripts of unchanged link nodes on reify', async t => t.ok(fs.lstatSync(resolve(path, 'a/a-prepare')).isFile(), 'should run prepare lifecycle scripts for links directly linked to the tree') + t.ok(fs.lstatSync(resolve(path, 'a/a-post-install')).isFile(), + 'should run postinstall lifecycle scripts for links directly linked to the tree') }) t.test('save-prod, with optional', async t => { diff --git a/workspaces/arborist/test/fixtures/link-dep-lifecycle-scripts/a/package.json b/workspaces/arborist/test/fixtures/link-dep-lifecycle-scripts/a/package.json index e545ad6869296..f239bf31d476d 100644 --- a/workspaces/arborist/test/fixtures/link-dep-lifecycle-scripts/a/package.json +++ b/workspaces/arborist/test/fixtures/link-dep-lifecycle-scripts/a/package.json @@ -2,6 +2,7 @@ "name": "a", "version": "1.0.0", "scripts": { - "prepare": "node -e \"require('fs').writeFileSync(require('path').resolve('a-prepare'), '')\"" + "prepare": "node -e \"require('fs').writeFileSync('a-prepare', '')\"", + "postinstall": "node -e \"require('fs').writeFileSync('a-post-install', '')\"" } } diff --git a/workspaces/arborist/test/fixtures/reify-cases/link-dep-lifecycle-scripts.js b/workspaces/arborist/test/fixtures/reify-cases/link-dep-lifecycle-scripts.js index 651761d5aa5f4..5f75cf33f0633 100644 --- a/workspaces/arborist/test/fixtures/reify-cases/link-dep-lifecycle-scripts.js +++ b/workspaces/arborist/test/fixtures/reify-cases/link-dep-lifecycle-scripts.js @@ -6,7 +6,8 @@ module.exports = t => { "name": "a", "version": "1.0.0", "scripts": { - "prepare": "node -e \"require('fs').writeFileSync(require('path').resolve('a-prepare'), '')\"" + "prepare": "node -e \"require('fs').writeFileSync('a-prepare', '')\"", + "postinstall": "node -e \"require('fs').writeFileSync('a-post-install', '')\"" } }) }, diff --git a/workspaces/arborist/test/fixtures/reify-cases/workspaces-link-bin.js b/workspaces/arborist/test/fixtures/reify-cases/workspaces-link-bin.js index ceca15eeb0569..59120e2ed93b9 100644 --- a/workspaces/arborist/test/fixtures/reify-cases/workspaces-link-bin.js +++ b/workspaces/arborist/test/fixtures/reify-cases/workspaces-link-bin.js @@ -25,13 +25,16 @@ module.exports = t => { }) }, "b": { - "file.js": "", + "create-file.js": "require('fs').writeFileSync('file.js', '')\n", "package.json": JSON.stringify({ "name": "b", "version": "1.0.0", "files": [ "file.js" ], + "scripts": { + "preinstall": "node create-file.js" + }, "bin": "file.js" }) } diff --git a/workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/create-file.js b/workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/create-file.js new file mode 100644 index 0000000000000..937f47da3fdf9 --- /dev/null +++ b/workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/create-file.js @@ -0,0 +1 @@ +require('fs').writeFileSync('file.js', '') diff --git a/workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/file.js b/workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/file.js deleted file mode 100644 index e69de29bb2d1d..0000000000000 diff --git a/workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/package.json b/workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/package.json index 52a65b6fa342c..3d0b8d0b83168 100644 --- a/workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/package.json +++ b/workspaces/arborist/test/fixtures/workspaces-link-bin/packages/b/package.json @@ -2,5 +2,8 @@ "name": "b", "version": "1.0.0", "files": ["file.js"], + "scripts": { + "preinstall": "node create-file.js" + }, "bin": "file.js" }