From b4689eb6249cdc2afd96a684a8887f0bcca929f6 Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Wed, 14 Jul 2021 16:45:44 -0400 Subject: [PATCH 1/3] feat(rebuild): add lifecycle scripts dependencies Adds the ability for lifecycle scripts of Link nodes to depend on each other, such that a `prepare` script of linked module A can make use of files that are a result of a `prepare` script of a linked package B. This is important in order to unlock workflows in which a workspace needs to make use of another workspace but they all have transpilation steps (very common in Typescript projects) so tracking the order (and completion) in which the dependencies lifecycle scripts runs becomes necessary. Fixes: https://github.com/npm/cli/issues/3034 --- lib/arborist/rebuild.js | 203 ++++++++++++++++++++++++++++----------- test/arborist/rebuild.js | 109 +++++++++++++++++++++ 2 files changed, 255 insertions(+), 57 deletions(-) diff --git a/lib/arborist/rebuild.js b/lib/arborist/rebuild.js index 8e447bb8f..2031466da 100644 --- a/lib/arborist/rebuild.js +++ b/lib/arborist/rebuild.js @@ -249,73 +249,162 @@ module.exports = cls => class Builder extends cls { if (!queue.length) return + // helper class to manage script execution + class Script { + constructor (opts) { + const { + node, + timer, + ...runScriptOpts + } = opts + this.node = node + this.timer = timer + this.opts = runScriptOpts + this.pkg = opts.pkg + this.path = opts.path + this.env = opts.env + this.ref = this.node.target.path + + if (!Script.runPromises) + Script.runPromises = new Map() + } + + // when the script to be run belongs to a link node, this method + // retrieves a list of all other link nodes that are direct dependencies + // of the current node, their scripts also have to be part of the current + // queue, otherwise it just returns an empty list + // this is necessary in order to enable workspaces lifecycle scripts + // that depend on each other + linkLinkedDeps () { + const res = [] + + if (!this.node.isLink) + return res + + for (const edge of this.node.target.edgesOut.values()) { + if (edge.to.isLink && + queue.some(node => node.isLink && node.target.name === edge.name)) + res.push(edge.to.target.path) + } + + return res + } + + // use the list of depended scripts from linkLinkedDeps() + // and returns a Promise that resolves once all scripts + // this current script depends are resolved + async linkLinkedDepsResolved () { + const resolved = (ref) => { + const p = Script.runPromises.get(ref) + // polling mechanism used in case the depended script is not + // yet set in Script.runPromises, this allow us to keep the + // alphabetical order of execution at first and the dependency + // mechanism for linked deps is only used when necessary + if (!p) { + return new Promise(res => + setTimeout(() => res(resolved(ref)), 10)) + } + return p + } + + return await Promise.all( + this.linkLinkedDeps().map(ref => resolved(ref))) + } + + async run () { + // in case the current script belongs to a link node that also has + // linked nodes dependencies (e.g: workspaces that depend on each other) + // then we await for that dependency script to be resolved before + // executing the current script + await this.linkLinkedDepsResolved() + + // executes the current script using @npmcli/run-script + const p = runScript(this.opts) + + // keeps a pool of executing scripts in order to track + // inter-dependencies between these scripts + Script.runPromises.set(this.ref, p) + return p + } + } + process.emit('time', `build:run:${event}`) const stdio = this.options.foregroundScripts ? 'inherit' : 'pipe' const limit = this.options.foregroundScripts ? 1 : undefined - await promiseCallLimit(queue.map(node => async () => { - const { - path, - integrity, - resolved, - optional, - peer, - dev, - devOptional, - package: pkg, - location, - } = node.target - - // skip any that we know we'll be deleting - if (this[_trashList].has(path)) - return - - const timer = `build:run:${event}:${location}` - process.emit('time', timer) - this.log.info('run', pkg._id, event, location, pkg.scripts[event]) - const env = { - npm_package_resolved: resolved, - npm_package_integrity: integrity, - npm_package_json: resolve(path, 'package.json'), - npm_package_optional: boolEnv(optional), - npm_package_dev: boolEnv(dev), - npm_package_peer: boolEnv(peer), - npm_package_dev_optional: - boolEnv(devOptional && !dev && !optional), - } - const runOpts = { - event, - path, - pkg, - stdioString: true, - stdio, - env, - scriptShell: this[_scriptShell], - } - const p = runScript(runOpts).catch(er => { - const { code, signal } = er - this.log.info('run', pkg._id, event, {code, signal}) - throw er - }).then(({args, code, signal, stdout, stderr}) => { - this.scriptsRun.add({ - pkg, + await promiseCallLimit(queue + .map(node => { + const { path, + integrity, + resolved, + optional, + peer, + dev, + devOptional, + package: pkg, + location, + } = node.target + + // skip any that we know we'll be deleting + if (this[_trashList].has(path)) + return + + const timer = `build:run:${event}:${location}` + process.emit('time', timer) + this.log.info('run', pkg._id, event, location, pkg.scripts[event]) + const env = { + npm_package_resolved: resolved, + npm_package_integrity: integrity, + npm_package_json: resolve(path, 'package.json'), + npm_package_optional: boolEnv(optional), + npm_package_dev: boolEnv(dev), + npm_package_peer: boolEnv(peer), + npm_package_dev_optional: + boolEnv(devOptional && !dev && !optional), + } + return new Script({ + node, + timer, event, - cmd: args && args[args.length - 1], + path, + pkg, + stdioString: true, + stdio, env, - code, - signal, - stdout, - stderr, + scriptShell: this[_scriptShell], }) - this.log.info('run', pkg._id, event, {code, signal}) }) + .filter(Boolean) + .map(script => async () => { + const p = script.run().catch(er => { + const { code, signal } = er + this.log.info('run', script.pkg._id, event, {code, signal}) + throw er + }).then(({args, code, signal, stdout, stderr}) => { + this.scriptsRun.add({ + pkg: script.pkg, + path: script.path, + event, + cmd: args && args[args.length - 1], + env: script.env, + code, + signal, + stdout, + stderr, + }) + this.log.info('run', script.pkg._id, event, {code, signal}) + }) + + await (this[_doHandleOptionalFailure] + ? this[_handleOptionalFailure](script.node, p) + : p) + + process.emit('timeEnd', script.timer) + }), limit) - await (this[_doHandleOptionalFailure] - ? this[_handleOptionalFailure](node, p) - : p) + // release pool refs to scripts promises + delete Script.runPromises - process.emit('timeEnd', timer) - }), limit) process.emit('timeEnd', `build:run:${event}`) } diff --git a/test/arborist/rebuild.js b/test/arborist/rebuild.js index f2a3bb192..77c386239 100644 --- a/test/arborist/rebuild.js +++ b/test/arborist/rebuild.js @@ -713,3 +713,112 @@ t.test('only rebuild for workspace', async t => { t.equal(fs.readFileSync(adepTxt, 'utf8'), 'adep', 'adep rebuilt') t.throws(() => fs.readFileSync(bdepTxt, 'utf8'), { code: 'ENOENT' }, 'bdep not rebuilt') }) + +t.test('scripts dependencies between Link nodes', async t => { + // this scenario is laid out as such: the `prepare` script of each linked + // pkg needs the resulting files of its dependency `prepare` script: + // + // prepare: b -> a -> c + // postinstall: e -> a `prepare` + // + // in order to make sure concurrency is handled properly, the `prepare` + // script of "c" takes at least 20ms to complete, while "a" takes at + // least 10ms and the "b" script expects to run synchronously + + const path = t.testdir({ + 'package.json': JSON.stringify({ + dependencies: { + a: 'file:./packages/a', + b: 'file:./packages/b', + c: 'file:./packages/c', + d: 'file:./packages/d', + }, + }), + node_modules: { + a: t.fixture('symlink', '../packages/a'), + b: t.fixture('symlink', '../packages/b'), + c: t.fixture('symlink', '../packages/c'), + d: t.fixture('symlink', '../packages/d'), + abbrev: { + 'package.json': JSON.stringify({ + name: 'abbrev', + version: '1.1.1', + }), + }, + }, + packages: { + a: { + 'package.json': JSON.stringify({ + name: 'a', + version: '1.0.0', + scripts: { + // on prepare script writes a `index.js` file containing: + // module.exports = require("c") + // this is a slow script though that sleeps for + // at least 10ms prior to writing that file + prepare: "node -e \"setTimeout(() => { require('fs').writeFileSync(require('path').resolve('index.js'), 'module.exports = require(function c(){}.name);') }, 10)\"", + }, + dependencies: { + c: '^1.0.0', + }, + }), + }, + b: { + 'package.json': JSON.stringify({ + name: 'b', + version: '1.0.0', + scripts: { + // on prepare script requires `./node_modules/a/index.js` which + // is a dependency of this workspace but with the caveat that this + // file is only build during the `prepare` script of "a" + prepare: "node -p \"require('a')\"", + }, + dependencies: { + a: '^1.0.0', + abbrev: '^1.0.0', + }, + }), + }, + c: { + 'package.json': JSON.stringify({ + name: 'c', + version: '1.0.0', + scripts: { + // on prepare script writes a `index.js` file containing: + // module.exports = "HELLO" + // this is an even slower slower script that sleeps for + // at least 20ms prior to writing that file + prepare: "node -e \"setTimeout(() => { require('fs').writeFileSync(require('path').resolve('index.js'), 'module.exports = function HELLO() {}.name;') }, 20)\"", + }, + }), + }, + d: { + 'package.json': JSON.stringify({ + name: 'd', + version: '1.0.0', + }), + }, + e: { + 'package.json': JSON.stringify({ + name: 'd', + version: '1.0.0', + scripts: { + // on postinstall script requires `./node_modules/a/index.js` which + // is a dependency of this workspace but with the caveat that this + // file is only build during the `prepare` script of "a" + // although different lifecycle scripts do not hold refs to + // depending on each other, all `prepare` scripts should already + // have been resolved by the time we run `postinstall` scripts + postinstall: "node -p \"require('a')\"", + }, + dependencies: { + a: '^1.0.0', + }, + }), + }, + }, + }) + + const arb = newArb({ path }) + await arb.rebuild() +}) From babac374c68d967d1234a13484d3a04f18386f04 Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Thu, 15 Jul 2021 12:04:16 -0400 Subject: [PATCH 2/3] use single poll system --- lib/arborist/rebuild.js | 69 +++++++++++++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 16 deletions(-) diff --git a/lib/arborist/rebuild.js b/lib/arborist/rebuild.js index 2031466da..cdcf83e74 100644 --- a/lib/arborist/rebuild.js +++ b/lib/arborist/rebuild.js @@ -269,6 +269,56 @@ module.exports = cls => class Builder extends cls { Script.runPromises = new Map() } + // releases any refs used in the pool and polling systems + static end () { + if (Script.runPromises) + Script.runPromises.clear() + + if (Script.pollRefs) + Script.pollRefs.clear() + } + + // make sure there's always just a single, polling system running + // at a given time, this method will return a promise that resolves + // once the original promise tracked in Script.runPromises is available + static waitRunPromise (ref) { + if (!Script.pollRefs) + Script.pollRefs = new Map() + + const pollAgain = () => setTimeout(poll, 10) + const poll = () => { + // check if the run-script promise is available for each entry + // in the polling queue, if so take it out of the queue + // and resolve its pending promise + for (const [pollRef, resolve] of Script.pollRefs.entries()) { + const p = Script.runPromises.get(pollRef) + if (p) { + Script.pollRefs.delete(pollRef) + resolve(p) + } + } + + // only poll again if there are still refs left in the queue + if (Script.pollRefs.size) + pollAgain() + } + + // starts the polling system for the first time, given that this + // part of the code is still synchronous, it's going to only start + // it once the poll refs are empty + if (!Script.pollRefs.size) + pollAgain() + + // creates a new promise that is going to be returned by this fuction + // and also be tracked in the polling system along with its queue ref + let resolve + const p = new Promise((res) => { + resolve = res + }) + Script.pollRefs.set(ref, resolve) + return p + } + // when the script to be run belongs to a link node, this method // retrieves a list of all other link nodes that are direct dependencies // of the current node, their scripts also have to be part of the current @@ -294,21 +344,8 @@ module.exports = cls => class Builder extends cls { // and returns a Promise that resolves once all scripts // this current script depends are resolved async linkLinkedDepsResolved () { - const resolved = (ref) => { - const p = Script.runPromises.get(ref) - // polling mechanism used in case the depended script is not - // yet set in Script.runPromises, this allow us to keep the - // alphabetical order of execution at first and the dependency - // mechanism for linked deps is only used when necessary - if (!p) { - return new Promise(res => - setTimeout(() => res(resolved(ref)), 10)) - } - return p - } - return await Promise.all( - this.linkLinkedDeps().map(ref => resolved(ref))) + this.linkLinkedDeps().map(ref => Script.waitRunPromise(ref))) } async run () { @@ -402,8 +439,8 @@ module.exports = cls => class Builder extends cls { process.emit('timeEnd', script.timer) }), limit) - // release pool refs to scripts promises - delete Script.runPromises + // releases pool refs + Script.end() process.emit('timeEnd', `build:run:${event}`) } From 3a6fa566feb02c46298b0a99b11d4191b5ab12c3 Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Fri, 16 Jul 2021 16:41:33 -0400 Subject: [PATCH 3/3] clean up script vars --- lib/arborist/rebuild.js | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/lib/arborist/rebuild.js b/lib/arborist/rebuild.js index cdcf83e74..655d42bfd 100644 --- a/lib/arborist/rebuild.js +++ b/lib/arborist/rebuild.js @@ -267,6 +267,9 @@ module.exports = cls => class Builder extends cls { if (!Script.runPromises) Script.runPromises = new Map() + + if (!Script.pollRefs) + Script.pollRefs = new Map() } // releases any refs used in the pool and polling systems @@ -276,15 +279,15 @@ module.exports = cls => class Builder extends cls { if (Script.pollRefs) Script.pollRefs.clear() + + delete Script.runPromises + delete Script.pollRefs } // make sure there's always just a single, polling system running // at a given time, this method will return a promise that resolves // once the original promise tracked in Script.runPromises is available static waitRunPromise (ref) { - if (!Script.pollRefs) - Script.pollRefs = new Map() - const pollAgain = () => setTimeout(poll, 10) const poll = () => { // check if the run-script promise is available for each entry @@ -343,9 +346,10 @@ module.exports = cls => class Builder extends cls { // use the list of depended scripts from linkLinkedDeps() // and returns a Promise that resolves once all scripts // this current script depends are resolved - async linkLinkedDepsResolved () { - return await Promise.all( - this.linkLinkedDeps().map(ref => Script.waitRunPromise(ref))) + linkLinkedDepsResolved () { + return Promise.all( + this.linkLinkedDeps() + .map(ref => Script.waitRunPromise(ref))) } async run () { @@ -363,6 +367,16 @@ module.exports = cls => class Builder extends cls { Script.runPromises.set(this.ref, p) return p } + + end () { + delete this.node + delete this.timer + delete this.opts + delete this.pkg + delete this.path + delete this.env + delete this.ref + } } process.emit('time', `build:run:${event}`) @@ -436,6 +450,7 @@ module.exports = cls => class Builder extends cls { ? this[_handleOptionalFailure](script.node, p) : p) + script.end() process.emit('timeEnd', script.timer) }), limit)