Show Sentry CLI output when uploading artifacts (#11100)

The function we were using to run shell commands during the
`sentry:publish` script were swallowing the CLI output. We also weren't
correctly detecting the process exit in some cases.

The `run-command` module originally written for `auto-changelog`
(introduced in #10782 and replaced in #10993) has been resurrected for
running commands where we don't care about the output, or where we want
to use the output for something. A second function (`runInShell`) has
been added for running commands with the same STDOUT and STDERR
streams, so that the output is sent directly to the CLI. This ensures
that the console output from the shell script we run gets correctly
output to the CLI.

development/lib/run-command.js

@@ -0,0 +1,134 @@
+const spawn = require('cross-spawn');
+
+/**
+ * Run a command to completion using the system shell.
+ *
+ * This will run a command with the specified arguments, and resolve when the
+ * process has exited. The STDOUT stream is monitored for output, which is
+ * returned after being split into lines. All output is expected to be UTF-8
+ * encoded, and empty lines are removed from the output.
+ *
+ * Anything received on STDERR is assumed to indicate a problem, and is tracked
+ * as an error.
+ *
+ * @param {string} command - The command to run
+ * @param {Array<string>} [args] - The arguments to pass to the command
+ * @returns {Array<string>} Lines of output received via STDOUT
+ */
+async function runCommand(command, args) {
+  const output = [];
+  let mostRecentError;
+  let errorSignal;
+  let errorCode;
+  const internalError = new Error('Internal');
+  try {
+    await new Promise((resolve, reject) => {
+      const childProcess = spawn(command, args, { encoding: 'utf8' });
+      childProcess.stdout.setEncoding('utf8');
+      childProcess.stderr.setEncoding('utf8');
+
+      childProcess.on('error', (error) => {
+        mostRecentError = error;
+      });
+
+      childProcess.stdout.on('data', (message) => {
+        const nonEmptyLines = message.split('\n').filter((line) => line !== '');
+        output.push(...nonEmptyLines);
+      });
+
+      childProcess.stderr.on('data', (message) => {
+        mostRecentError = new Error(message.trim());
+      });
+
+      childProcess.once('exit', (code, signal) => {
+        if (code === 0) {
+          return resolve();
+        }
+        errorCode = code;
+        errorSignal = signal;
+        return reject(internalError);
+      });
+    });
+  } catch (error) {
+    /**
+     * The error is re-thrown here in an `async` context to preserve the stack trace. If this was
+     * was thrown inside the Promise constructor, the stack trace would show a few frames of
+     * Node.js internals then end, without indicating where `runCommand` was called.
+     */
+    if (error === internalError) {
+      let errorMessage;
+      if (errorCode !== null && errorSignal !== null) {
+        errorMessage = `Terminated by signal '${errorSignal}'; exited with code '${errorCode}'`;
+      } else if (errorSignal !== null) {
+        errorMessage = `Terminaled by signal '${errorSignal}'`;
+      } else if (errorCode === null) {
+        errorMessage = 'Exited with no code or signal';
+      } else {
+        errorMessage = `Exited with code '${errorCode}'`;
+      }
+      const improvedError = new Error(errorMessage);
+      if (mostRecentError) {
+        improvedError.cause = mostRecentError;
+      }
+      throw improvedError;
+    }
+  }
+  return output;
+}
+
+/**
+ * Run a command to using the system shell.
+ *
+ * This will run a command with the specified arguments, and resolve when the
+ * process has exited. The STDIN, STDOUT and STDERR streams are inherited,
+ * letting the command take over completely until it completes. The success or
+ * failure of the process is determined entirely by the exit code; STDERR
+ * output is not used to indicate failure.
+ *
+ * @param {string} command - The command to run
+ * @param {Array<string>} [args] - The arguments to pass to the command
+ */
+async function runInShell(command, args) {
+  let errorSignal;
+  let errorCode;
+  const internalError = new Error('Internal');
+  try {
+    await new Promise((resolve, reject) => {
+      const childProcess = spawn(command, args, {
+        encoding: 'utf8',
+        stdio: 'inherit',
+      });
+
+      childProcess.once('exit', (code, signal) => {
+        if (code === 0) {
+          return resolve();
+        }
+        errorCode = code;
+        errorSignal = signal;
+        return reject(internalError);
+      });
+    });
+  } catch (error) {
+    /**
+     * The error is re-thrown here in an `async` context to preserve the stack trace. If this was
+     * was thrown inside the Promise constructor, the stack trace would show a few frames of
+     * Node.js internals then end, without indicating where `runInShell` was called.
+     */
+    if (error === internalError) {
+      let errorMessage;
+      if (errorCode !== null && errorSignal !== null) {
+        errorMessage = `Terminated by signal '${errorSignal}'; exited with code '${errorCode}'`;
+      } else if (errorSignal !== null) {
+        errorMessage = `Terminaled by signal '${errorSignal}'`;
+      } else if (errorCode === null) {
+        errorMessage = 'Exited with no code or signal';
+      } else {
+        errorMessage = `Exited with code '${errorCode}'`;
+      }
+      const improvedError = new Error(errorMessage);
+      throw improvedError;
+    }
+  }
+}
+
+module.exports = { runCommand, runInShell };

development/sentry-publish.js

@@ -1,9 +1,6 @@
 #!/usr/bin/env node
-const childProcess = require('child_process');
-const pify = require('pify');
-
-const exec = pify(childProcess.exec, { multiArgs: true });
 const VERSION = require('../dist/chrome/manifest.json').version; // eslint-disable-line import/no-unresolved
+const { runCommand, runInShell } = require('./lib/run-command');
 
 start().catch((error) => {
   console.error(error);
@@ -31,11 +28,17 @@ async function start() {
   } else {
     // create sentry release
     console.log(`creating Sentry release for "${VERSION}"...`);
-    await exec(`sentry-cli releases new ${VERSION}`);
+    await runCommand('sentry-cli', ['releases', 'new', VERSION]);
     console.log(
       `removing any existing files from Sentry release "${VERSION}"...`,
     );
-    await exec(`sentry-cli releases files ${VERSION} delete --all`);
+    await runCommand('sentry-cli', [
+      'releases',
+      'files',
+      VERSION,
+      'delete',
+      '--all',
+    ]);
   }
 
   // check if version has artifacts or not
@@ -49,34 +52,43 @@ async function start() {
   }
 
   // upload sentry source and sourcemaps
-  await exec(`./development/sentry-upload-artifacts.sh --release ${VERSION}`);
+  await runInShell('./development/sentry-upload-artifacts.sh', [
+    '--release',
+    VERSION,
+  ]);
 }
 
 async function checkIfAuthWorks() {
-  const itWorked = await doesNotFail(async () => {
-    await exec(`sentry-cli releases list`);
-  });
-  return itWorked;
+  return await doesNotFail(() =>
+    runCommand('sentry-cli', ['releases', 'list']),
+  );
 }
 
 async function checkIfVersionExists() {
-  const versionAlreadyExists = await doesNotFail(async () => {
-    await exec(`sentry-cli releases info ${VERSION}`);
-  });
-  return versionAlreadyExists;
+  return await doesNotFail(() =>
+    runCommand('sentry-cli', ['releases', 'info', VERSION]),
+  );
 }
 
 async function checkIfVersionHasArtifacts() {
-  const artifacts = await exec(`sentry-cli releases files ${VERSION} list`);
+  const [artifact] = await runCommand('sentry-cli', [
+    'releases',
+    'files',
+    VERSION,
+    'list',
+  ]);
   // When there's no artifacts, we get a response from the shell like this ['', '']
-  return artifacts[0] && artifacts[0].length > 0;
+  return artifact?.length > 0;
 }
 
 async function doesNotFail(asyncFn) {
   try {
     await asyncFn();
     return true;
-  } catch (err) {
-    return false;
+  } catch (error) {
+    if (error.message === `Exited with code '1'`) {
+      return false;
+    }
+    throw error;
   }
 }

development/sentry-upload-artifacts.sh

@@ -1,6 +1,5 @@
 #!/usr/bin/env bash
 
-set -x
 set -e
 set -u
 set -o pipefail