From 1cfbf88f7c62d66a6c686757ae5e405126d6768e Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Thu, 18 Feb 2021 01:26:16 -0500
Subject: [PATCH] run_cmd: Enhance with ability to return stdout as string

---
 src/node/utils/run_cmd.js         | 158 +++++++++++++++++++++---------
 src/static/js/pluginfw/plugins.js |  34 ++-----
 2 files changed, 119 insertions(+), 73 deletions(-)

diff --git a/src/node/utils/run_cmd.js b/src/node/utils/run_cmd.js
index 2af5381a..f4bc59bd 100644
--- a/src/node/utils/run_cmd.js
+++ b/src/node/utils/run_cmd.js
@@ -28,70 +28,130 @@ const logLines = (readable, logLineFn) => {
 };
 
 /**
- * Similar to `util.promisify(child_rocess.exec)`, except:
- *   - `cwd` defaults to the Etherpad root directory.
- *   - PATH is prefixed with src/node_modules/.bin so that utilities from installed dependencies
- *     (e.g., npm) are preferred over system utilities.
- *   - Output is passed to logger callback functions by default. See below for details.
+ * Runs a command, logging its output to Etherpad's logs by default.
+ *
+ * Examples:
+ *
+ *   Just run a command, logging stdout and stder to Etherpad's logs:
+ *     await runCmd(['ls', '-l']);
+ *
+ *   Capture just stdout as a string:
+ *     const stdout = await runCmd(['ls', '-l'], {stdio: [null, 'string']});
+ *
+ *   Capture both stdout and stderr as strings:
+ *     const p = runCmd(['ls', '-l'], {stdio: 'string'});
+ *     const stdout = await p; // Or: await p.stdout;
+ *     const stderr = await p.stderr;
+ *
+ *   Call a callback with each line of stdout:
+ *     await runCmd(['ls', '-l'], {stdio: [null, (line) => console.log(line)]});
  *
  * @param args Array of command-line arguments, where `args[0]` is the command to run.
- * @param opts Optional options that will be passed to `child_process.spawn()` with two extensions:
- *   - `stdoutLogger`: Callback that is called each time a line of text is written to stdout (utf8
- *     is assumed). The line (without trailing newline) is passed as the only argument. If null,
- *     stdout is not logged. If unset, defaults to logging to the Etherpad logs at log level INFO.
- *     Ignored if stdout is not a pipe.
- *   - `stderrLogger`: Like `stdoutLogger` but for stderr and log level ERROR.
+ * @param opts As with `child_process.spawn()`, except:
+ *   - `cwd` defaults to the Etherpad root directory.
+ *   - `env.PATH` is prefixed with `src/node_modules/.bin:node_modules/.bin` so that utilities from
+ *     installed dependencies (e.g., npm) are preferred over system utilities.
+ *   - By default stdout and stderr are logged to the Etherpad log at log levels INFO and ERROR.
+ *     To pipe without logging you must explicitly use 'pipe' for opts.stdio.
+ *   - opts.stdio[1] and opts.stdio[2] can be functions that will be called each time a line (utf8)
+ *     is written to stdout or stderr. The line (without its trailing newline, if present) will be
+ *     passed as the only argument, and the return value is ignored. opts.stdio = fn is equivalent
+ *     to opts.stdio = [null, fn, fn].
+ *   - opts.stdio[1] and opts.stdio[2] can be 'string', which will cause output to be collected,
+ *     decoded as utf8, and returned (see below). opts.stdio = 'string' is equivalent to
+ *     opts.stdio = [null, 'string', 'string'].
  *
- * @returns A Promise with `stdout`, `stderr`, and `child` properties containing the stdout stream,
- *     stderr stream, and ChildProcess objects, respectively.
+ * @returns A Promise that resolves when the command exits. The Promise resolves to the complete
+ * stdout if opts.stdio[1] is 'string', otherwise it resolves to undefined. The returned Promise is
+ * augmented with these additional properties:
+ *   - `stdout`: If opts.stdio[1] is 'pipe', the stdout stream object. If opts.stdio[1] is 'string',
+ *     a Promise that will resolve to the complete stdout (utf8 decoded) when the command exits.
+ *   - `stderr`: Similar to `stdout` but for stderr.
+ *   - `child`: The ChildProcess object.
  */
 module.exports = exports = (args, opts = {}) => {
   logger.debug(`Executing command: ${args.join(' ')}`);
 
-  const cmdLogger = log4js.getLogger(`runCmd|${args[0]}`);
-  const {
-    stdoutLogger = (line) => cmdLogger.info(line),
-    stderrLogger = (line) => cmdLogger.error(line),
-  } = opts;
-  // Avoid confusing child_process.spawn() with our extensions.
-  opts = {...opts}; // Make a copy to avoid mutating the caller's copy.
-  delete opts.stdoutLogger;
-  delete opts.stderrLogger;
+  opts = {cwd: settings.root, ...opts};
+  logger.debug(`cwd: ${opts.cwd}`);
 
+  // Log stdout and stderr by default.
+  const stdio =
+      Array.isArray(opts.stdio) ? opts.stdio.slice() // Copy to avoid mutating the caller's array.
+      : typeof opts.stdio === 'function' ? [null, opts.stdio, opts.stdio]
+      : opts.stdio === 'string' ? [null, 'string', 'string']
+      : Array(3).fill(opts.stdio);
+  const cmdLogger = log4js.getLogger(`runCmd|${args[0]}`);
+  if (stdio[1] == null) stdio[1] = (line) => cmdLogger.info(line);
+  if (stdio[2] == null) stdio[2] = (line) => cmdLogger.error(line);
+  const stdioLoggers = [];
+  const stdioSaveString = [];
+  for (const fd of [1, 2]) {
+    if (typeof stdio[fd] === 'function') {
+      stdioLoggers[fd] = stdio[fd];
+      stdio[fd] = 'pipe';
+    } else if (stdio[fd] === 'string') {
+      stdioSaveString[fd] = true;
+      stdio[fd] = 'pipe';
+    }
+  }
+  opts.stdio = stdio;
+
+  // On Windows the PATH environment var might be spelled "Path".
+  const pathVarName =
+      Object.keys(process.env).filter((k) => k.toUpperCase() === 'PATH')[0] || 'PATH';
   // Set PATH so that utilities from installed dependencies (e.g., npm) are preferred over system
   // (global) utilities.
-  let {env = process.env} = opts;
-  env = {...env}; // Copy to avoid modifying process.env.
-  // On Windows the PATH environment var might be spelled "Path".
-  const pathVarName = Object.keys(env).filter((k) => k.toUpperCase() === 'PATH')[0] || 'PATH';
-  env[pathVarName] = [
-    path.join(settings.root, 'src', 'node_modules', '.bin'),
-    path.join(settings.root, 'node_modules', '.bin'),
-    ...(env[pathVarName] ? env[pathVarName].split(path.delimiter) : []),
-  ].join(path.delimiter);
+  const {env = process.env} = opts;
+  const {[pathVarName]: PATH} = env;
+  opts.env = {
+    ...env, // Copy env to avoid modifying process.env or the caller's supplied env.
+    [pathVarName]: [
+      path.join(settings.root, 'src', 'node_modules', '.bin'),
+      path.join(settings.root, 'node_modules', '.bin'),
+      ...(PATH ? PATH.split(path.delimiter) : []),
+    ].join(path.delimiter),
+  };
   logger.debug(`${pathVarName}=${env[pathVarName]}`);
 
   // Create an error object to use in case the process fails. This is done here rather than in the
   // process's `exit` handler so that we get a useful stack trace.
-  const procFailedErr = new Error(`Command exited non-zero: ${args.join(' ')}`);
+  const procFailedErr = new Error();
 
-  const proc = spawn(args[0], args.slice(1), {cwd: settings.root, ...opts, env});
-  if (proc.stdout != null && stdoutLogger != null) logLines(proc.stdout, stdoutLogger);
-  if (proc.stderr != null && stderrLogger != null) logLines(proc.stderr, stderrLogger);
-  const p = new Promise((resolve, reject) => {
-    proc.on('exit', (code, signal) => {
-      if (code !== 0) {
-        logger.debug(procFailedErr.stack);
-        procFailedErr.code = code;
-        procFailedErr.signal = signal;
-        return reject(procFailedErr);
-      }
-      logger.debug(`Command returned successfully: ${args.join(' ')}`);
-      resolve();
-    });
-  });
-  p.stdout = proc.stdout;
-  p.stderr = proc.stderr;
+  const proc = spawn(args[0], args.slice(1), opts);
+  const streams = [undefined, proc.stdout, proc.stderr];
+
+  let px;
+  const p = new Promise((resolve, reject) => { px = {resolve, reject}; });
+  [, p.stdout, p.stderr] = streams;
   p.child = proc;
+
+  const stdioStringPromises = [undefined, Promise.resolve(), Promise.resolve()];
+  for (const fd of [1, 2]) {
+    if (streams[fd] == null) continue;
+    if (stdioLoggers[fd] != null) {
+      logLines(streams[fd], stdioLoggers[fd]);
+    } else if (stdioSaveString[fd]) {
+      p[[null, 'stdout', 'stderr'][fd]] = stdioStringPromises[fd] = (async () => {
+        const chunks = [];
+        for await (const chunk of streams[fd]) chunks.push(chunk);
+        return Buffer.concat(chunks).toString().replace(/\n+$/g, '');
+      })();
+    }
+  }
+
+  proc.on('exit', async (code, signal) => {
+    const [, stdout] = await Promise.all(stdioStringPromises);
+    if (code !== 0) {
+      procFailedErr.message =
+          `Command exited ${code ? `with code ${code}` : `on signal ${signal}`}: ${args.join(' ')}`;
+      procFailedErr.code = code;
+      procFailedErr.signal = signal;
+      logger.debug(procFailedErr.stack);
+      return px.reject(procFailedErr);
+    }
+    logger.debug(`Command returned successfully: ${args.join(' ')}`);
+    px.resolve(stdout);
+  });
   return p;
 };
diff --git a/src/static/js/pluginfw/plugins.js b/src/static/js/pluginfw/plugins.js
index 0860a65b..b705fb73 100644
--- a/src/static/js/pluginfw/plugins.js
+++ b/src/static/js/pluginfw/plugins.js
@@ -12,22 +12,15 @@ const defs = require('./plugin_defs');
 const logger = log4js.getLogger('plugins');
 
 // Log the version of npm at startup.
-let loggedVersion = false;
 (async () => {
-  if (loggedVersion) return;
-  loggedVersion = true;
-  const p = runCmd(['npm', '--version'], {stdoutLogger: null});
-  const chunks = [];
-  await Promise.all([
-    (async () => { for await (const chunk of p.stdout) chunks.push(chunk); })(),
-    p, // Await in parallel to avoid unhandled rejection if p rejects during chunk read.
-  ]);
-  const version = Buffer.concat(chunks).toString().replace(/\n+$/g, '');
-  logger.info(`npm --version: ${version}`);
-})().catch((err) => {
-  logger.error(`Failed to get npm version: ${err.stack || err}`);
-  // This isn't a fatal error so don't re-throw.
-});
+  try {
+    const version = await runCmd(['npm', '--version'], {stdio: [null, 'string']});
+    logger.info(`npm --version: ${version}`);
+  } catch (err) {
+    logger.error(`Failed to get npm version: ${err.stack || err}`);
+    // This isn't a fatal error so don't re-throw.
+  }
+})();
 
 exports.prefix = 'ep_';
 
@@ -96,15 +89,8 @@ exports.getPackages = async () => {
   //   * The `--no-production` flag is required (or the `NODE_ENV` environment variable must be
   //     unset or set to `development`) because otherwise `npm ls` will not mention any packages
   //     that are not included in `package.json` (which is expected to not exist).
-  const p = runCmd(['npm', 'ls', '--long', '--json', '--depth=0', '--no-production'], {
-    stdoutLogger: null, // We want to capture stdout, so don't attempt to log it.
-  });
-  const chunks = [];
-  await Promise.all([
-    (async () => { for await (const chunk of p.stdout) chunks.push(chunk); })(),
-    p, // Await in parallel to avoid unhandled rejection if p rejects during chunk read.
-  ]);
-  const {dependencies = {}} = JSON.parse(Buffer.concat(chunks).toString());
+  const cmd = ['npm', 'ls', '--long', '--json', '--depth=0', '--no-production'];
+  const {dependencies = {}} = JSON.parse(await runCmd(cmd, {stdio: [null, 'string']}));
   await Promise.all(Object.entries(dependencies).map(async ([pkg, info]) => {
     if (!pkg.startsWith(exports.prefix)) {
       delete dependencies[pkg];