Skip to content

Commit

Permalink
Clean up how the commands are built and run, and make sure strip is c…
Browse files Browse the repository at this point in the history 
…alled 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.
  • Loading branch information
@bsamuel-ui
bsamuel-ui committed Nov 27, 2018
1 parent 337cd46 commit 937fa56
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 101 deletions.
200 changes: 107 additions & 93 deletions lib/pip.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
}

/**
Expand Down Expand Up @@ -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
Expand All @@ -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) {
Expand All @@ -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(
Expand All @@ -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;
Expand All @@ -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...
Expand All @@ -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
Expand Down
18 changes: 14 additions & 4 deletions lib/slim.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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*'];
Expand All @@ -27,6 +36,7 @@ const deleteFiles = (options, folderPath) => {
};

module.exports = {
getStripMode,
getStripCommand,
deleteFiles
};
12 changes: 8 additions & 4 deletions test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand All @@ -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();
};
Expand Down

2 comments on commit 937fa56

@jeanbmar
Copy link

@jeanbmar jeanbmar commented on 937fa56 Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsamuel-ui @pgrzesik this PR has severely damaged this repo, eg:

  • It stopped printing stdout and stderr and now people have to PR things that make no sense like refactor: Log child process command output on error #679 (which actually doesn't work for pip install errors)
  • It stopped using a shell when calling spawn which totally breaks this plugin on Windows

@pgrzesik
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @jeanbmar - this is a PR from ~4 years ago - what exactly has been broken? Could you provide some reproduction steps and describe the issues fully in a separate issue?

Please sign in to comment.