diff --git a/workspaces/arborist/lib/arborist/isolated-reifier.js b/workspaces/arborist/lib/arborist/isolated-reifier.js index 80c907aa49b6d..122bafb55edc1 100644 --- a/workspaces/arborist/lib/arborist/isolated-reifier.js +++ b/workspaces/arborist/lib/arborist/isolated-reifier.js @@ -139,6 +139,8 @@ module.exports = cls => class IsolatedReifier extends cls { result.localLocation = node.location result.localPath = node.path result.isWorkspace = true + // isWorkspace above marks every local proxy, file: deps included; this is only the project's workspaces. + result.isProjectWorkspace = node.isWorkspace result.resolved = node.resolved await this.#assignCommonProperties(node, result) return result @@ -316,6 +318,7 @@ module.exports = cls => class IsolatedReifier extends cls { package: c.package, path: c.localPath, resolved: c.resolved, + isWorkspace: c.isProjectWorkspace, }) root.fsChildren.add(workspace) root.inventory.set(workspace.location, workspace) @@ -441,7 +444,8 @@ module.exports = cls => class IsolatedReifier extends cls { version: dep.package.version, } const link = new IsolatedLink({ - isStoreLink: true, + // Links to local targets (workspaces, file: deps) must take the links build pass so their prepare scripts run. + isStoreLink: external, location: join(nmFolder, dep.name), name: toKey, optional, diff --git a/workspaces/arborist/lib/arborist/rebuild.js b/workspaces/arborist/lib/arborist/rebuild.js index 994cac843acac..970cdc6319383 100644 --- a/workspaces/arborist/lib/arborist/rebuild.js +++ b/workspaces/arborist/lib/arborist/rebuild.js @@ -25,6 +25,7 @@ const _trashList = Symbol.for('trashList') module.exports = cls => class Builder extends cls { #doHandleOptionalFailure + #sharedLinks = new WeakMap() #oldMeta = null #queues = { preinstall: [], @@ -194,6 +195,18 @@ module.exports = cls => class Builder extends cls { // there's no particular reason for doing it in this order rather // than another, but sorting *somehow* makes it consistent. const queue = [...set].sort(sortNodes) + // Links in this pass sharing a target run its scripts once, through a required link if any so a failure is not swallowed as optional. + const linksByTarget = new Map() + for (const node of queue.filter(n => n.isLink)) { + const links = linksByTarget.get(node.target) ?? [] + linksByTarget.set(node.target, [...links, node]) + } + const scriptLinks = new Set() + for (const links of linksByTarget.values()) { + const picked = links.find(l => !l.optional) ?? links[0] + scriptLinks.add(picked) + this.#sharedLinks.set(picked, links) + } for (const node of queue) { const { package: { bin, scripts = {} } } = node.target @@ -211,13 +224,14 @@ module.exports = cls => class Builder extends cls { // For non-links node.target === node, so registry deps are unaffected. const scriptsAllowed = this.options.dangerouslyAllowAllScripts || - node.isWorkspace || + node.target.isWorkspace || isScriptAllowed(node.target, this.options.allowScripts) === true + const runScripts = scriptsAllowed && (!node.isLink || scriptLinks.has(node)) for (const [key, has] of Object.entries(tests)) { if (!has) { continue } - if (key !== 'bin' && !scriptsAllowed) { + if (key !== 'bin' && !runScripts) { continue } this.#queues[key].push(node) @@ -366,8 +380,10 @@ module.exports = cls => class Builder extends cls { log.info('run', pkg._id, event, { code, signal }) }) + // A failure applies to every link that skipped the script in favor of this one. + const links = this.#sharedLinks.get(node) ?? [node] await (this.#doHandleOptionalFailure - ? this[_handleOptionalFailure](node, p) + ? Promise.all(links.map(link => this[_handleOptionalFailure](link, p))) : p) timeEndLocation() diff --git a/workspaces/arborist/lib/isolated-classes.js b/workspaces/arborist/lib/isolated-classes.js index c4894e3da4437..cf0159f42f095 100644 --- a/workspaces/arborist/lib/isolated-classes.js +++ b/workspaces/arborist/lib/isolated-classes.js @@ -22,6 +22,7 @@ class IsolatedNode { inBundle = false isRegistryDependency = false isRootDependency = false + isWorkspace = false linksIn = new Set() meta = { loadedFromDisk: false } optional = false @@ -58,6 +59,9 @@ class IsolatedNode { if (options.isRootDependency) { this.isRootDependency = true } + if (options.isWorkspace) { + this.isWorkspace = true + } if (options.optional) { this.optional = true } diff --git a/workspaces/arborist/test/arborist/rebuild.js b/workspaces/arborist/test/arborist/rebuild.js index ab433f2f97174..a356e0aaeff81 100644 --- a/workspaces/arborist/test/arborist/rebuild.js +++ b/workspaces/arborist/test/arborist/rebuild.js @@ -242,6 +242,106 @@ t.test('workspaces bypass the allowScripts gate (owner-managed)', async t => { ) }) +t.test('links sharing a target run its scripts once through a required link', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'root', + version: '1.0.0', + optionalDependencies: { 'a-opt': 'file:./shared' }, + dependencies: { 'b-req': 'file:./shared', 'c-req': 'file:./shared' }, + }), + node_modules: { + 'a-opt': t.fixture('symlink', '../shared'), + 'b-req': t.fixture('symlink', '../shared'), + 'c-req': t.fixture('symlink', '../shared'), + }, + shared: { + 'package.json': JSON.stringify({ + name: 'shared', + version: '1.0.0', + scripts: { postinstall: 'exit 1' }, + }), + }, + }) + const runs = [] + const Arborist = t.mock('../../lib/arborist/index.js', { + '@npmcli/run-script': async ({ event }) => { + runs.push(event) + throw Object.assign(new Error('script failed'), { code: 1 }) + }, + }) + const arb = new Arborist({ path, dangerouslyAllowAllScripts: true }) + const tree = await arb.loadActual() + const nodes = ['a-opt', 'b-req', 'c-req'].map(name => tree.children.get(name)) + t.equal(nodes[0].optional, true, 'first sorted link is optional') + await t.rejects(arb.rebuild({ nodes, handleOptionalFailure: true }), { message: 'script failed' }) + t.same(runs, ['postinstall'], 'script ran once, and its failure was not swallowed') +}) + +t.test('failed script of a target shared by optional links removes every link', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'root', + version: '1.0.0', + optionalDependencies: { 'a-opt': 'file:./shared', 'b-opt': 'file:./shared' }, + }), + node_modules: { + 'a-opt': t.fixture('symlink', '../shared'), + 'b-opt': t.fixture('symlink', '../shared'), + }, + shared: { + 'package.json': JSON.stringify({ + name: 'shared', + version: '1.0.0', + scripts: { postinstall: 'exit 1' }, + }), + }, + }) + const runs = [] + const Arborist = t.mock('../../lib/arborist/index.js', { + '@npmcli/run-script': async ({ event }) => { + runs.push(event) + throw Object.assign(new Error('script failed'), { code: 1 }) + }, + }) + const arb = new Arborist({ path, dangerouslyAllowAllScripts: true }) + const tree = await arb.loadActual() + const nodes = ['a-opt', 'b-opt'].map(name => tree.children.get(name)) + await arb.rebuild({ nodes, handleOptionalFailure: true }) + t.same(runs, ['postinstall'], 'script ran once') + t.ok(arb[_trashList].has(resolve(path, 'node_modules/a-opt')), 'link that ran the script is removed') + t.ok(arb[_trashList].has(resolve(path, 'node_modules/b-opt')), 'link that skipped the script is removed') +}) + +t.test('rebuilding shared links twice with one arborist does not throw', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'root', + version: '1.0.0', + dependencies: { a: 'file:./shared', b: 'file:./shared' }, + }), + node_modules: { + a: t.fixture('symlink', '../shared'), + b: t.fixture('symlink', '../shared'), + }, + shared: { + 'package.json': JSON.stringify({ + name: 'shared', + version: '1.0.0', + scripts: { postinstall: 'exit 0' }, + }), + }, + }) + const Arborist = t.mock('../../lib/arborist/index.js', { + '@npmcli/run-script': async () => ({ code: 0 }), + }) + const arb = new Arborist({ path, dangerouslyAllowAllScripts: true }) + const tree = await arb.loadActual() + const nodes = ['a', 'b'].map(name => tree.children.get(name)) + await arb.rebuild({ nodes, handleOptionalFailure: true }) + await t.resolves(arb.rebuild({ nodes, handleOptionalFailure: true })) +}) + t.test('do nothing if ignoreScripts=true and binLinks=false', async t => { const path = fixture(t, 'testing-rebuild-bundle-reified') const file = resolve(path, 'node_modules/@isaacs/testing-rebuild-bundle-a/node_modules/@isaacs/testing-rebuild-bundle-b/cwd') diff --git a/workspaces/arborist/test/isolated-mode.js b/workspaces/arborist/test/isolated-mode.js index 9d289ebefa974..f9e4e0260ae89 100644 --- a/workspaces/arborist/test/isolated-mode.js +++ b/workspaces/arborist/test/isolated-mode.js @@ -1473,6 +1473,36 @@ tap.test('postinstall scripts run once for store packages', async t => { t.equal(count, 1, 'postinstall ran exactly once') }) +tap.test('workspace lifecycle scripts run once without allowScripts approval', async t => { + // Workspaces are exempt from allowScripts; a workspace linked into several dependents still runs each script once. + const log = event => `node -e "fs.appendFileSync('runs.log', '${event}\\n')"` + const scripts = Object.fromEntries(['preinstall', 'install', 'postinstall', 'prepare'].map(e => [e, log(e)])) + const graph = { + registry: [ + { name: 'which', version: '1.0.0', scripts: { postinstall: log('postinstall') } }, + ], + root: { + name: 'foo', version: '1.2.3', dependencies: { bar: '*', which: '1.0.0' }, + }, + workspaces: [ + { name: 'bar', version: '1.0.0', scripts }, + { name: 'baz', version: '1.0.0', dependencies: { bar: '*' }, scripts: { postinstall: log('postinstall') } }, + { name: 'qux', version: '1.0.0', dependencies: { bar: '*' } }, + ], + } + + const { dir, registry } = await getRepo(graph) + + const cache = fs.mkdtempSync(`${getTempDir()}/test-`) + const arborist = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) + await arborist.reify({ installStrategy: 'linked' }) + + const runs = ws => fs.readFileSync(path.join(dir, 'packages', ws, 'runs.log'), 'utf8') + t.equal(runs('bar'), 'preinstall\nprepare\ninstall\npostinstall\n', 'declared workspace with dependents runs each script once') + t.equal(runs('baz'), 'postinstall\n', 'undeclared workspace runs its script') + t.notOk(pathExists(`${setupRequire(dir)('which')}/runs.log`), 'unapproved registry dependency script is blocked') +}) + tap.test('workspace-filtered install with linked strategy', async t => { // Two workspaces sharing the same dependency must not crash when installing with --workspace + --install-strategy=linked. const graph = {