From d3a8128c31c8794fa413c26b329f1f2f8a32cf50 Mon Sep 17 00:00:00 2001 From: Daniel Stockman Date: Tue, 8 May 2018 17:20:14 -0700 Subject: [PATCH] fix(child-process): Prevent duplicate logs when any package-oriented execution fails Previously, avoiding duplicate logging required special handling per use-case and only covered non-streaming variants. --- commands/exec/__tests__/exec-command.test.js | 9 +++ commands/exec/index.js | 61 ++++++++----------- commands/run/index.js | 19 ++---- .../__tests__/child-process.test.js | 13 ++++ core/child-process/index.js | 36 ++++++++--- integration/lerna-run.test.js | 2 +- utils/get-npm-exec-opts/get-npm-exec-opts.js | 1 + .../__tests__/npm-dist-tag.test.js | 6 ++ .../npm-install/__tests__/npm-install.test.js | 12 +++- .../npm-publish/__tests__/npm-publish.test.js | 5 ++ .../__tests__/npm-run-script.test.js | 5 ++ 11 files changed, 106 insertions(+), 63 deletions(-) diff --git a/commands/exec/__tests__/exec-command.test.js b/commands/exec/__tests__/exec-command.test.js index 6180d0d17d..a697c67185 100644 --- a/commands/exec/__tests__/exec-command.test.js +++ b/commands/exec/__tests__/exec-command.test.js @@ -89,9 +89,14 @@ describe("ExecCommand", () => { expect(ChildProcessUtilities.spawn).toHaveBeenCalledTimes(1); expect(ChildProcessUtilities.spawn).lastCalledWith("ls", [], { cwd: path.join(testDir, "packages/package-2"), + pkg: expect.objectContaining({ + name: "package-2", + }), env: expect.objectContaining({ LERNA_PACKAGE_NAME: "package-2", + LERNA_ROOT_PATH: testDir, }), + extendEnv: false, reject: true, shell: true, }); @@ -120,9 +125,13 @@ describe("ExecCommand", () => { expect(ChildProcessUtilities.spawn).toHaveBeenCalledTimes(1); expect(ChildProcessUtilities.spawn).lastCalledWith("ls", [], { cwd: path.join(testDir, "packages/package-1"), + pkg: expect.objectContaining({ + name: "package-1", + }), env: expect.objectContaining({ LERNA_PACKAGE_NAME: "package-1", }), + extendEnv: false, reject: true, shell: true, }); diff --git a/commands/exec/index.js b/commands/exec/index.js index ff9a8e06f0..670e773747 100644 --- a/commands/exec/index.js +++ b/commands/exec/index.js @@ -27,20 +27,21 @@ class ExecCommand extends Command { initialize() { const dashedArgs = this.options["--"] || []; - const { cmd, args } = this.options; - this.command = cmd || dashedArgs.shift(); - this.args = (args || []).concat(dashedArgs); + this.command = this.options.cmd || dashedArgs.shift(); + this.args = (this.options.args || []).concat(dashedArgs); if (!this.command) { throw new ValidationError("ENOCOMMAND", "A command to execute is required"); } - const { filteredPackages } = this; + // accessing properties of process.env can be expensive, + // so cache it here to reduce churn during tighter loops + this.env = Object.assign({}, process.env); this.batchedPackages = this.toposort - ? batchPackages(filteredPackages, this.options.rejectCycles) - : [filteredPackages]; + ? batchPackages(this.filteredPackages, this.options.rejectCycles) + : [this.filteredPackages]; } execute() { @@ -48,29 +49,24 @@ class ExecCommand extends Command { return this.runCommandInPackagesParallel(); } - return runParallelBatches(this.batchedPackages, this.concurrency, pkg => - this.runCommandInPackage(pkg).catch(err => { - this.logger.error("exec", `'${err.cmd}' errored in '${pkg.name}'`); + const runner = this.options.stream + ? pkg => this.runCommandInPackageStreaming(pkg) + : pkg => this.runCommandInPackageCapturing(pkg); - if (err.code) { - // log non-lerna error cleanly - err.pkg = pkg; - } - - throw err; - }) - ); + return runParallelBatches(this.batchedPackages, this.concurrency, runner); } getOpts(pkg) { return { cwd: pkg.location, shell: true, - env: Object.assign({}, process.env, { + extendEnv: false, + env: Object.assign({}, this.env, { LERNA_PACKAGE_NAME: pkg.name, LERNA_ROOT_PATH: this.project.rootPath, }), reject: this.options.bail, + pkg, }; } @@ -82,28 +78,19 @@ class ExecCommand extends Command { [this.command].concat(this.args).join(" ") ); - return Promise.all( - this.filteredPackages.map(pkg => - ChildProcessUtilities.spawnStreaming( - this.command, - this.args, - this.getOpts(pkg), - this.options.prefix && pkg.name - ) - ) - ); + return Promise.all(this.filteredPackages.map(pkg => this.runCommandInPackageStreaming(pkg))); } - runCommandInPackage(pkg) { - if (this.options.stream) { - return ChildProcessUtilities.spawnStreaming( - this.command, - this.args, - this.getOpts(pkg), - this.options.prefix && pkg.name - ); - } + runCommandInPackageStreaming(pkg) { + return ChildProcessUtilities.spawnStreaming( + this.command, + this.args, + this.getOpts(pkg), + this.options.prefix && pkg.name + ); + } + runCommandInPackageCapturing(pkg) { return ChildProcessUtilities.spawn(this.command, this.args, this.getOpts(pkg)); } } diff --git a/commands/run/index.js b/commands/run/index.js index 89209a5c03..50db574afb 100644 --- a/commands/run/index.js +++ b/commands/run/index.js @@ -88,7 +88,7 @@ class RunCommand extends Command { ? pkg => this.runScriptInPackageStreaming(pkg) : pkg => this.runScriptInPackageCapturing(pkg); - return runParallelBatches(this.batchedPackages, this.concurrency, pkg => runner(pkg)); + return runParallelBatches(this.batchedPackages, this.concurrency, runner); } runScriptInPackagesParallel() { @@ -107,20 +107,9 @@ class RunCommand extends Command { } runScriptInPackageCapturing(pkg) { - return npmRunScript(this.script, this.getOpts(pkg)) - .then(result => { - output(result.stdout); - }) - .catch(err => { - this.logger.error("run", `'${this.script}' errored in '${pkg.name}'`); - - if (err.code) { - // log non-lerna error cleanly - err.pkg = pkg; - } - - throw err; - }); + return npmRunScript(this.script, this.getOpts(pkg)).then(result => { + output(result.stdout); + }); } } diff --git a/core/child-process/__tests__/child-process.test.js b/core/child-process/__tests__/child-process.test.js index f6a0676bf2..cd56cccde5 100644 --- a/core/child-process/__tests__/child-process.test.js +++ b/core/child-process/__tests__/child-process.test.js @@ -42,6 +42,19 @@ describe("ChildProcessUtilities", () => { expect(one.stdout).toBe("one"); expect(two.stdout).toBe("two"); }); + + it("decorates opts.pkg on error if caught", async () => { + try { + await ChildProcessUtilities.exec( + "theVeneratedVirginianVeteranWhoseMenAreAll", + ["liningUpToPutMeUpOnAPedestal"], + { pkg: { name: "hamilton" } } + ); + } catch (err) { + expect(err.code).toBe("ENOENT"); + expect(err.pkg).toEqual({ name: "hamilton" }); + } + }); }); describe(".spawn()", () => { diff --git a/core/child-process/index.js b/core/child-process/index.js index 453795de91..8290abf568 100644 --- a/core/child-process/index.js +++ b/core/child-process/index.js @@ -13,8 +13,9 @@ const NUM_COLORS = colorWheel.length; function exec(command, args, opts) { const options = Object.assign({ stdio: "pipe" }, opts); + const spawned = spawnProcess(command, args, options); - return _spawn(command, args, options); + return wrapError(spawned); } function execSync(command, args, opts) { @@ -22,10 +23,10 @@ function execSync(command, args, opts) { } function spawn(command, args, opts) { - const options = Object.assign({}, opts); - options.stdio = "inherit"; + const options = Object.assign({}, opts, { stdio: "inherit" }); + const spawned = spawnProcess(command, args, options); - return _spawn(command, args, options); + return wrapError(spawned); } // istanbul ignore next @@ -35,7 +36,7 @@ function spawnStreaming(command, args, opts, prefix) { const colorName = colorWheel[children % NUM_COLORS]; const color = chalk[colorName]; - const spawned = _spawn(command, args, options); + const spawned = spawnProcess(command, args, options); const stdoutOpts = {}; const stderrOpts = {}; // mergeMultiline causes escaped newlines :P @@ -54,15 +55,14 @@ function spawnStreaming(command, args, opts, prefix) { spawned.stdout.pipe(logTransformer(stdoutOpts)).pipe(process.stdout); spawned.stderr.pipe(logTransformer(stderrOpts)).pipe(process.stderr); - return spawned; + return wrapError(spawned); } function getChildProcessCount() { return children; } -// eslint-disable-next-line no-underscore-dangle -function _spawn(command, args, opts) { +function spawnProcess(command, args, opts) { children += 1; const child = execa(command, args, opts); @@ -78,9 +78,29 @@ function _spawn(command, args, opts) { child.once("exit", drain); child.once("error", drain); + if (opts.pkg) { + child.pkg = opts.pkg; + } + return child; } +function wrapError(spawned) { + if (spawned.pkg) { + return spawned.catch(err => { + // istanbul ignore else + if (err.code) { + // log non-lerna error cleanly + err.pkg = spawned.pkg; + } + + throw err; + }); + } + + return spawned; +} + exports.exec = exec; exports.execSync = execSync; exports.spawn = spawn; diff --git a/integration/lerna-run.test.js b/integration/lerna-run.test.js index 440b2a8294..49701def1f 100644 --- a/integration/lerna-run.test.js +++ b/integration/lerna-run.test.js @@ -15,7 +15,7 @@ describe("lerna run", () => { try { await cliRunner(cwd)(...args); } catch (err) { - expect(err.message).toMatch("run 'fail' errored in 'package-3'"); + expect(err.message).toMatch("npm run fail --silent exited 1 in 'package-3'"); } }); diff --git a/utils/get-npm-exec-opts/get-npm-exec-opts.js b/utils/get-npm-exec-opts/get-npm-exec-opts.js index 2fc1798743..8efd913d91 100644 --- a/utils/get-npm-exec-opts/get-npm-exec-opts.js +++ b/utils/get-npm-exec-opts/get-npm-exec-opts.js @@ -7,6 +7,7 @@ module.exports = getExecOpts; function getExecOpts(pkg, registry) { const opts = { cwd: pkg.location, + pkg, }; if (registry) { diff --git a/utils/npm-dist-tag/__tests__/npm-dist-tag.test.js b/utils/npm-dist-tag/__tests__/npm-dist-tag.test.js index 193f77ca59..11a66d24a3 100644 --- a/utils/npm-dist-tag/__tests__/npm-dist-tag.test.js +++ b/utils/npm-dist-tag/__tests__/npm-dist-tag.test.js @@ -27,6 +27,7 @@ describe("dist-tag", () => { expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["dist-tag", "add", "foo-pkg@1.0.0", tag], { cwd: pkg.location, + pkg, }); }); @@ -39,6 +40,7 @@ describe("dist-tag", () => { npm_config_registry: registry, }), extendEnv: false, + pkg, }); }); }); @@ -56,6 +58,7 @@ describe("dist-tag", () => { expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["dist-tag", "rm", pkg.name, tag], { cwd: pkg.location, + pkg, }); }); @@ -68,6 +71,7 @@ describe("dist-tag", () => { npm_config_registry: registry, }), extendEnv: false, + pkg, }); }); }); @@ -88,6 +92,7 @@ describe("dist-tag", () => { expect(ChildProcessUtilities.execSync).lastCalledWith("npm", ["dist-tag", "ls", pkg.name], { cwd: pkg.location, + pkg, }); }); @@ -102,6 +107,7 @@ describe("dist-tag", () => { npm_config_registry: registry, }), extendEnv: false, + pkg, }); }); }); diff --git a/utils/npm-install/__tests__/npm-install.test.js b/utils/npm-install/__tests__/npm-install.test.js index b4902c90f5..e71cb45dee 100644 --- a/utils/npm-install/__tests__/npm-install.test.js +++ b/utils/npm-install/__tests__/npm-install.test.js @@ -42,7 +42,7 @@ describe("npm-install", () => { expect(ChildProcessUtilities.exec).lastCalledWith( "yarn", ["install", "--mutex", "file:foo", "--non-interactive", "--no-optional"], - { cwd: pkg.location, stdio: "pipe" } + { cwd: pkg.location, pkg, stdio: "pipe" } ); }); @@ -62,6 +62,7 @@ describe("npm-install", () => { expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["install"], { cwd: pkg.location, + pkg, stdio: "inherit", }); }); @@ -87,6 +88,7 @@ describe("npm-install", () => { expect(ChildProcessUtilities.exec).lastCalledWith("yarn", ["install", "--non-interactive"], { cwd: pkg.location, + pkg, stdio: "pipe", }); } @@ -139,6 +141,7 @@ describe("npm-install", () => { }); expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["install"], { cwd: pkg.location, + pkg, stdio: "pipe", }); }); @@ -182,6 +185,7 @@ describe("npm-install", () => { npm_config_registry: config.registry, }), extendEnv: false, + pkg, stdio: "pipe", }); }); @@ -220,6 +224,7 @@ describe("npm-install", () => { }); expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["install", "--global-style"], { cwd: pkg.location, + pkg, stdio: "pipe", }); }); @@ -254,7 +259,7 @@ describe("npm-install", () => { expect(ChildProcessUtilities.exec).lastCalledWith( "yarn", ["install", "--mutex", "network:12345", "--non-interactive"], - { cwd: pkg.location, stdio: "pipe" } + { cwd: pkg.location, pkg, stdio: "pipe" } ); }); @@ -292,6 +297,7 @@ describe("npm-install", () => { }); expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["install", "--production", "--no-optional"], { cwd: pkg.location, + pkg, stdio: "pipe", }); }); @@ -332,6 +338,7 @@ describe("npm-install", () => { }); expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["install", "--global-style"], { cwd: pkg.location, + pkg, stdio: "pipe", }); }); @@ -370,6 +377,7 @@ describe("npm-install", () => { }); expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["ci"], { cwd: pkg.location, + pkg, stdio: "pipe", }); }); diff --git a/utils/npm-publish/__tests__/npm-publish.test.js b/utils/npm-publish/__tests__/npm-publish.test.js index d63e79799d..748955e325 100644 --- a/utils/npm-publish/__tests__/npm-publish.test.js +++ b/utils/npm-publish/__tests__/npm-publish.test.js @@ -25,6 +25,7 @@ describe("npm-publish", () => { ["publish", "--ignore-scripts", "--tag", "published-tag"], { cwd: pkg.location, + pkg, } ); }); @@ -34,6 +35,7 @@ describe("npm-publish", () => { expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["publish", "--ignore-scripts"], { cwd: pkg.location, + pkg, }); }); @@ -45,6 +47,7 @@ describe("npm-publish", () => { ["publish", "--ignore-scripts", "--tag", "trailing-tag"], { cwd: pkg.location, + pkg, } ); }); @@ -63,6 +66,7 @@ describe("npm-publish", () => { npm_config_registry: registry, }), extendEnv: false, + pkg, } ); }); @@ -84,6 +88,7 @@ describe("npm-publish", () => { ], { cwd: pkg.location, + pkg, } ); }); diff --git a/utils/npm-run-script/__tests__/npm-run-script.test.js b/utils/npm-run-script/__tests__/npm-run-script.test.js index 7ec78a0f82..ed7f8a5402 100644 --- a/utils/npm-run-script/__tests__/npm-run-script.test.js +++ b/utils/npm-run-script/__tests__/npm-run-script.test.js @@ -27,6 +27,7 @@ describe("npm-run-script", () => { expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["run", script, "--bar", "baz"], { cwd: config.pkg.location, + pkg: config.pkg, reject: true, }); }); @@ -46,6 +47,7 @@ describe("npm-run-script", () => { expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["run", script], { cwd: config.pkg.location, + pkg: config.pkg, reject: false, }); }); @@ -64,6 +66,7 @@ describe("npm-run-script", () => { expect(ChildProcessUtilities.exec).lastCalledWith("yarn", ["run", script, "--bar", "baz"], { cwd: config.pkg.location, + pkg: config.pkg, reject: true, }); }); @@ -89,6 +92,7 @@ describe("npm-run-script", () => { ["run", script, "--bar", "baz"], { cwd: config.pkg.location, + pkg: config.pkg, reject: true, }, config.pkg.name @@ -114,6 +118,7 @@ describe("npm-run-script", () => { ["run", script], { cwd: config.pkg.location, + pkg: config.pkg, reject: false, }, undefined