Clean up how the commands are built and run, and make sure strip is called correctly.

- Make sure we're consistently quoting arguments.
- Add mergeCommands function to construct a script for docker to run when needed.
- Add getStripMode to run strip correctly for the platform and docker.

Author: bsamuel-ui

lib/pip.js

@@ -6,16 +6,42 @@ const set = require('lodash.set');
 const { spawnSync } = require('child_process');
 const { quote } = require('shell-quote');
 const { buildImage, getBindPath, getDockerUid } = require('./docker');
-const { getStripCommand, deleteFiles } = require('./slim');
+const { getStripCommand, getStripMode, deleteFiles } = require('./slim');
 const {
   checkForAndDeleteMaxCacheVersions,
   md5Path,
   getRequirementsWorkingPath,
   getUserCachePath
 } = require('./shared');
 
-function quote_single(quoteme) {
-  return quote([quoteme]);
+/**
+ * Omit empty commands.
+ * In this context, a "command" is a list of arguments. An empty list or falsy value is ommitted.
+ * @param {string[][]} many commands to merge.
+ * @return {string[][]} a list of valid commands.
+ */
+function filterCommands(commands) {
+  return commands.filter((cmd) => Boolean(cmd) && cmd.length > 0);
+}
+
+/**
+ * Render zero or more commands as a single command for a Unix environment.
+ * In this context, a "command" is a list of arguments. An empty list or falsy value is ommitted.
+ *
+ * @param {string[][]} many commands to merge.
+ * @return {string[]} a single list of words.
+ */
+function mergeCommands(commands) {
+  const cmds = filterCommands(commands);
+  if (cmds.length === 0) {
+    throw new Error('Expected at least one non-empty command')
+  } else if (cmds.length === 1) {
+    return cmds[0];
+  } else {
+    // Quote the arguments in each command and join them all using &&.
+    const script = cmds.map(quote).join(' && ');
+    return ["/bin/sh", "-c", script];
+  }
 }
 
 /**
@@ -51,6 +77,25 @@ function installRequirementsFile(
   }
 }
 
+function pipAcceptsSystem(pythonBin) {
+  // Check if pip has Debian's --system option and set it if so
+  const pipTestRes = spawnSync(pythonBin, [
+    '-m',
+    'pip',
+    'help',
+    'install'
+  ]);
+  if (pipTestRes.error) {
+    if (pipTestRes.error.code === 'ENOENT') {
+      throw new Error(
+        `${pythonBin} not found! Try the pythonBin option.`
+      );
+    }
+    throw pipTestRes.error;
+  }
+  return pipTestRes.stdout.toString().indexOf('--system') >= 0;
+}
+
 /**
  * Install requirements described from requirements in the targetFolder into that same targetFolder
  * @param {string} targetFolder
@@ -65,15 +110,16 @@ function installRequirements(targetFolder, serverless, options) {
     `Installing requirements from ${targetRequirementsTxt} ...`
   );
 
-  let cmd;
-  let cmdOptions;
-  let pipCmd = [
+  const dockerCmd = [];
+  const pipCmd = [
     options.pythonBin,
     '-m',
     'pip',
     'install',
     ...options.pipCmdExtraArgs
   ];
+  const pipCmds = [pipCmd];
+  const postCmds = [];
   // Check if we're using the legacy --cache-dir command...
   if (options.pipCmdExtraArgs.indexOf('--cache-dir') > -1) {
     if (options.dockerizePip) {
@@ -94,8 +140,8 @@ function installRequirements(targetFolder, serverless, options) {
 
   if (!options.dockerizePip) {
     // Push our local OS-specific paths for requirements and target directory
-    pipCmd.push('-t', dockerPathForWin(options, targetFolder));
-    pipCmd.push('-r', dockerPathForWin(options, targetRequirementsTxt));
+    pipCmd.push('-t', dockerPathForWin(targetFolder),
+                '-r', dockerPathForWin(targetRequirementsTxt));
     // If we want a download cache...
     if (options.useDownloadCache) {
       const downloadCacheDir = path.join(
@@ -104,35 +150,17 @@ function installRequirements(targetFolder, serverless, options) {
       );
       serverless.cli.log(`Using download cache directory ${downloadCacheDir}`);
       fse.ensureDirSync(downloadCacheDir);
-      pipCmd.push('--cache-dir', quote_single(downloadCacheDir));
+      pipCmd.push('--cache-dir', downloadCacheDir);
     }
 
-    // Check if pip has Debian's --system option and set it if so
-    const pipTestRes = spawnSync(options.pythonBin, [
-      '-m',
-      'pip',
-      'help',
-      'install'
-    ]);
-    if (pipTestRes.error) {
-      if (pipTestRes.error.code === 'ENOENT') {
-        throw new Error(
-          `${options.pythonBin} not found! ` + 'Try the pythonBin option.'
-        );
-      }
-      throw pipTestRes.error;
-    }
-    if (pipTestRes.stdout.toString().indexOf('--system') >= 0) {
+    if (pipAcceptsSystem(options.pythonBin)) {
       pipCmd.push('--system');
     }
   }
   // If we are dockerizing pip
   if (options.dockerizePip) {
-    cmd = 'docker';
-
     // Push docker-specific paths for requirements and target directory
-    pipCmd.push('-t', '/var/task/');
-    pipCmd.push('-r', '/var/task/requirements.txt');
+    pipCmd.push('-t', '/var/task/', '-r', '/var/task/requirements.txt');
 
     // Build docker image if required
     let dockerImage;
@@ -148,28 +176,18 @@ function installRequirements(targetFolder, serverless, options) {
 
     // Prepare bind path depending on os platform
     const bindPath = dockerPathForWin(
-      options,
       getBindPath(serverless, targetFolder)
     );
 
-    cmdOptions = ['run', '--rm', '-v', `${bindPath}:/var/task:z`];
+    dockerCmd.push('docker', 'run', '--rm', '-v', `${bindPath}:/var/task:z`);
     if (options.dockerSsh) {
       // Mount necessary ssh files to work with private repos
-      cmdOptions.push(
-        '-v',
-        quote_single(`${process.env.HOME}/.ssh/id_rsa:/root/.ssh/id_rsa:z`)
-      );
-      cmdOptions.push(
-        '-v',
-        quote_single(
-          `${process.env.HOME}/.ssh/known_hosts:/root/.ssh/known_hosts:z`
-        )
-      );
-      cmdOptions.push(
-        '-v',
-        quote_single(`${process.env.SSH_AUTH_SOCK}:/tmp/ssh_sock:z`)
+      dockerCmd.push(
+        '-v', `${process.env.HOME}/.ssh/id_rsa:/root/.ssh/id_rsa:z`,
+        '-v', `${process.env.HOME}/.ssh/known_hosts:/root/.ssh/known_hosts:z`,
+        '-v', `${process.env.SSH_AUTH_SOCK}:/tmp/ssh_sock:z`,
+        '-e', 'SSH_AUTH_SOCK=/tmp/ssh_sock'
       );
-      cmdOptions.push('-e', 'SSH_AUTH_SOCK=/tmp/ssh_sock');
     }
 
     // If we want a download cache...
@@ -189,104 +207,100 @@ function installRequirements(targetFolder, serverless, options) {
       );
       const windowsized = getBindPath(serverless, downloadCacheDir);
       // And now push it to a volume mount and to pip...
-      cmdOptions.push(
+      dockerCmd.push(
         '-v',
-        quote_single(`${windowsized}:${dockerDownloadCacheDir}:z`)
+        `${windowsized}:${dockerDownloadCacheDir}:z`
       );
-      pipCmd.push('--cache-dir', quote_single(dockerDownloadCacheDir));
+      pipCmd.push('--cache-dir', dockerDownloadCacheDir);
     }
 
     if (options.dockerEnv) {
       // Add environment variables to docker run cmd
       options.dockerEnv.forEach(function(item) {
-        cmdOptions.push('-e', item);
+        dockerCmd.push('-e', item);
       });
     }
 
     if (process.platform === 'linux') {
       // Use same user so requirements folder is not root and so --cache-dir works
-      var commands = [];
       if (options.useDownloadCache) {
         // Set the ownership of the download cache dir to root
-        commands.push(quote(['chown', '-R', '0:0', dockerDownloadCacheDir]));
+        pipCmds.unshift(['chown', '-R', '0:0', dockerDownloadCacheDir]);
       }
       // Install requirements with pip
-      commands.push(pipCmd.join(' '));
       // Set the ownership of the current folder to user
-      commands.push(
-        quote([
-          'chown',
-          '-R',
-          `${process.getuid()}:${process.getgid()}`,
-          '/var/task'
-        ])
-      );
+      pipCmds.push(['chown', '-R', `${process.getuid()}:${process.getgid()}`, '/var/task']);
       if (options.useDownloadCache) {
         // Set the ownership of the download cache dir back to user
-        commands.push(
-          quote([
+        pipCmds.push(
+          [
             'chown',
             '-R',
             `${process.getuid()}:${process.getgid()}`,
             dockerDownloadCacheDir
-          ])
+          ]
         );
       }
-      pipCmd = ['/bin/bash', '-c', '"' + commands.join(' && ') + '"'];
     } else {
       // Use same user so --cache-dir works
-      cmdOptions.push('-u', quote_single(getDockerUid(bindPath)));
+      dockerCmd.push('-u', getDockerUid(bindPath));
     }
-    cmdOptions.push(dockerImage);
-    cmdOptions.push(...pipCmd);
-  } else {
-    cmd = pipCmd[0];
-    cmdOptions = pipCmd.slice(1);
+    dockerCmd.push(dockerImage);
   }
 
   // If enabled slimming, strip so files
-  if (options.slim === true || options.slim === 'true') {
-    const preparedPath = dockerPathForWin(options, targetFolder);
-    cmdOptions.push(getStripCommand(options, preparedPath));
+  switch (getStripMode(options)) {
+    case 'docker':
+      pipCmds.push(getStripCommand(options, '/var/task'));
+    case 'direct':
+      postCmds.push(getStripCommand(options, dockerPathForWin(targetFolder)));
   }
+
   let spawnArgs = { shell: true };
   if (process.env.SLS_DEBUG) {
     spawnArgs.stdio = 'inherit';
   }
-  const res = spawnSync(cmd, cmdOptions, spawnArgs);
-  if (res.error) {
-    if (res.error.code === 'ENOENT') {
-      if (options.dockerizePip) {
-        throw new Error('docker not found! Please install it.');
-      }
-      throw new Error(
-        `${options.pythonBin} not found! Try the pythonBin option.`
-      );
-    }
-    throw res.error;
-  }
-  if (res.status !== 0) {
-    throw new Error(res.stderr);
+  let mainCmds = [];
+  if (dockerCmd.length) {
+    dockerCmd.push(...mergeCommands(pipCmds));
+    mainCmds = [dockerCmd];
+  } else {
+    mainCmds = pipCmds;
   }
+  mainCmds.push(...postCmds);
+
+  serverless.cli.log(`Running ${quote(dockerCmd)}...`);
+
+  filterCommands(mainCmds).forEach(([cmd, ...args]) => {
+      const res = spawnSync(cmd, args);
+      if (res.error) {
+        if (res.error.code === 'ENOENT') {
+          const advice = cmd.indexOf('python') > -1 ? 'Try the pythonBin option' : 'Please install it';
+          throw new Error(`${cmd} not found! ${advice}`);
+        }
+        throw res.error;
+      }
+      if (res.status !== 0) {
+        throw new Error(res.stderr);
+      }
+  });
   // If enabled slimming, delete files in slimPatterns
   if (options.slim === true || options.slim === 'true') {
     deleteFiles(options, targetFolder);
   }
 }
 
 /**
- * convert path from Windows style to Linux style, if needed
- * @param {Object} options
+ * Convert path from Windows style to Linux style, if needed.
  * @param {string} path
  * @return {string}
  */
-function dockerPathForWin(options, path) {
+function dockerPathForWin(path) {
   if (process.platform === 'win32') {
-    return `"${path.replace(/\\/g, '/')}"`;
-  } else if (process.platform === 'win32' && !options.dockerizePip) {
+    return path.replace(/\\/g, '/');
+  } else {
     return path;
   }
-  return quote_single(path);
 }
 
 /** create a filtered requirements.txt without anything from noDeploy

lib/slim.js

@@ -2,10 +2,19 @@ const isWsl = require('is-wsl');
 const glob = require('glob-all');
 const fse = require('fs-extra');
 
-const getStripCommand = (options, folderPath) =>
-  process.platform !== 'win32' || isWsl || options.dockerizePip
-    ? ` && find ${folderPath} -name "*.so" -exec strip {} ';'`
-    : '';
+const getStripMode = (options) => {
+  if (options.slim === false || options.slim === 'false') {
+    return 'skip';
+  } else if (options.dockerizePip) {
+    return 'docker';
+  } else if (!isWsl && process.platform === 'win32' || process.platform === 'darwin') {
+    return 'skip';
+  } else {
+    return 'direct';
+  }
+}
+
+const getStripCommand = (options, folderPath) => (['find', folderPath, '-name', '*.so', '-exec', 'strip', '{}', '+']);
 
 const deleteFiles = (options, folderPath) => {
   let patterns = ['**/*.py[c|o]', '**/__pycache__*', '**/*.dist-info*'];
@@ -27,6 +36,7 @@ const deleteFiles = (options, folderPath) => {
 };
 
 module.exports = {
+  getStripMode,
   getStripCommand,
   deleteFiles
 };

test.js

@@ -3,6 +3,7 @@ const deasync = require('deasync-promise');
 const glob = require('glob-all');
 const JSZip = require('jszip');
 const tape = require('tape');
+const { quote } = require('shell-quote');
 const { removeSync, readFileSync, copySync } = require('fs-extra');
 const { sep } = require('path');
 
@@ -25,11 +26,14 @@ const mkCommand = cmd => (args, options = {}) => {
       options
     )
   );
-  if (error) throw error;
+  if (error) {
+    console.error(`Error running: ${quote([cmd, ...args])}`);
+    throw error;
+  }
   if (status) {
-    console.error(stdout.toString()); // eslint-disable-line no-console
-    console.error(stderr.toString()); // eslint-disable-line no-console
-    throw new Error(`${cmd} failed with status code ${status}`);
+    console.error('STDOUT: ', stdout.toString()); // eslint-disable-line no-console
+    console.error('STDERR: ', stderr.toString()); // eslint-disable-line no-console
+    throw new Error(`${quote([cmd, ...args])} failed with status code ${status}`);
   }
   return stdout && stdout.toString().trim();
 };