Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: Adapt to async version of spawn #648

Merged
merged 1 commit into from
Nov 24, 2021
Merged

refactor: Adapt to async version of spawn #648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 19 additions & 18 deletions lib/docker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const { spawnSync } = require('child_process');
const spawn = require('child-process-ext/spawn');
const isWsl = require('is-wsl');
const fse = require('fs-extra');
const path = require('path');
Expand All @@ -8,18 +8,19 @@ const path = require('path');
* @param {string[]} options
* @return {Object}
*/
function dockerCommand(options) {
async function dockerCommand(options) {
const cmd = 'docker';
const ps = spawnSync(cmd, options, { encoding: 'utf-8' });
if (ps.error) {
if (ps.error.code === 'ENOENT') {
try {
return await spawn(cmd, options, { encoding: 'utf-8' });
} catch (e) {
if (
e.stderrBuffer &&
e.stderrBuffer.toString().includes('command not found')
) {
throw new Error('docker not found! Please install it.');
medikoo marked this conversation as resolved.
Show resolved Hide resolved
}
throw new Error(ps.error);
} else if (ps.status !== 0) {
throw new Error(ps.stderr);
throw e;
}
return ps;
}

/**
Expand All @@ -28,7 +29,7 @@ function dockerCommand(options) {
* @param {string[]} extraArgs
* @return {string} The name of the built docker image.
*/
function buildImage(dockerFile, extraArgs) {
async function buildImage(dockerFile, extraArgs) {
const imageName = 'sls-py-reqs-custom';
const options = ['build', '-f', dockerFile, '-t', imageName];

Expand All @@ -40,7 +41,7 @@ function buildImage(dockerFile, extraArgs) {

options.push('.');

dockerCommand(options);
await dockerCommand(options);
return imageName;
}

Expand Down Expand Up @@ -72,7 +73,7 @@ function findTestFile(servicePath) {
* @param {string} bindPath
* @return {boolean}
*/
function tryBindPath(serverless, bindPath, testFile) {
async function tryBindPath(serverless, bindPath, testFile) {
const debug = process.env.SLS_DEBUG;
const options = [
'run',
Expand All @@ -85,7 +86,7 @@ function tryBindPath(serverless, bindPath, testFile) {
];
try {
if (debug) serverless.cli.log(`Trying bindPath ${bindPath} (${options})`);
const ps = dockerCommand(options);
const ps = await dockerCommand(options);
if (debug) serverless.cli.log(ps.stdout.trim());
return ps.stdout.trim() === `/test/${testFile}`;
} catch (err) {
Expand All @@ -100,14 +101,14 @@ function tryBindPath(serverless, bindPath, testFile) {
* @param {string} servicePath
* @return {string} The bind path.
*/
function getBindPath(serverless, servicePath) {
async function getBindPath(serverless, servicePath) {
// Determine bind path
if (process.platform !== 'win32' && !isWsl) {
return servicePath;
}

// test docker is available
dockerCommand(['version']);
await dockerCommand(['version']);

// find good bind path for Windows
let bindPaths = [];
Expand Down Expand Up @@ -144,7 +145,7 @@ function getBindPath(serverless, servicePath) {

for (let i = 0; i < bindPaths.length; i++) {
const bindPath = bindPaths[i];
if (tryBindPath(serverless, bindPath, testFile)) {
if (await tryBindPath(serverless, bindPath, testFile)) {
return bindPath;
}
}
Expand All @@ -157,7 +158,7 @@ function getBindPath(serverless, servicePath) {
* @param {string} bindPath
* @return {boolean}
*/
function getDockerUid(bindPath) {
async function getDockerUid(bindPath) {
const options = [
'run',
'--rm',
Expand All @@ -169,7 +170,7 @@ function getDockerUid(bindPath) {
'%u',
'/bin/sh',
];
const ps = dockerCommand(options);
const ps = await dockerCommand(options);
return ps.stdout.trim();
}

Expand Down
147 changes: 79 additions & 68 deletions lib/pip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const rimraf = require('rimraf');
const path = require('path');
const get = require('lodash.get');
const set = require('lodash.set');
const { spawnSync } = require('child_process');
const spawn = require('child-process-ext/spawn');
const { quote } = require('shell-quote');
const { buildImage, getBindPath, getDockerUid } = require('./docker');
const { getStripCommand, getStripMode, deleteFiles } = require('./slim');
Expand Down Expand Up @@ -96,16 +96,23 @@ function generateRequirementsFile(
}
}

function pipAcceptsSystem(pythonBin) {
async 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') {
try {
const pipTestRes = await spawn(pythonBin, ['-m', 'pip', 'help', 'install']);
return (
pipTestRes.stdoutBuffer &&
Copy link
Contributor

Choose a reason for hiding this comment

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

stdoutBuffer will be provided unconditionally, so no need to confirm on it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is that the same case with stderrBuffer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but in case of error handling, there could be an edge case, where the command fails because it cannot be invoked for some reason and then the error won't hold any of those properties

pipTestRes.stdoutBuffer.toString().indexOf('--system') >= 0
Copy link
Contributor

Choose a reason for hiding this comment

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

More natural would be includes('--system')

);
} catch (e) {
if (
e.stderrBuffer &&
e.stderrBuffer.toString().includes('command not found')
) {
throw new Error(`${pythonBin} not found! Try the pythonBin option.`);
}
throw pipTestRes.error;
throw e;
}
return pipTestRes.stdout.toString().indexOf('--system') >= 0;
}

/**
Expand All @@ -115,7 +122,7 @@ function pipAcceptsSystem(pythonBin) {
* @param {Object} options
* @return {undefined}
*/
function installRequirements(targetFolder, serverless, options) {
async function installRequirements(targetFolder, serverless, options) {
const targetRequirementsTxt = path.join(targetFolder, 'requirements.txt');

serverless.cli.log(
Expand Down Expand Up @@ -176,7 +183,7 @@ function installRequirements(targetFolder, serverless, options) {
pipCmd.push('--cache-dir', downloadCacheDir);
}

if (pipAcceptsSystem(options.pythonBin)) {
if (await pipAcceptsSystem(options.pythonBin)) {
pipCmd.push('--system');
}
}
Expand All @@ -191,7 +198,7 @@ function installRequirements(targetFolder, serverless, options) {
serverless.cli.log(
`Building custom docker image from ${options.dockerFile}...`
);
dockerImage = buildImage(
dockerImage = await buildImage(
options.dockerFile,
options.dockerBuildCmdExtraArgs
);
Expand All @@ -201,7 +208,9 @@ function installRequirements(targetFolder, serverless, options) {
serverless.cli.log(`Docker Image: ${dockerImage}`);

// Prepare bind path depending on os platform
const bindPath = dockerPathForWin(getBindPath(serverless, targetFolder));
const bindPath = dockerPathForWin(
await getBindPath(serverless, targetFolder)
);

dockerCmd.push('docker', 'run', '--rm', '-v', `${bindPath}:/var/task:z`);
if (options.dockerSsh) {
Expand Down Expand Up @@ -233,7 +242,7 @@ function installRequirements(targetFolder, serverless, options) {
fse.closeSync(
fse.openSync(path.join(downloadCacheDir, 'requirements.txt'), 'w')
);
const windowsized = getBindPath(serverless, downloadCacheDir);
const windowsized = await getBindPath(serverless, downloadCacheDir);
// And now push it to a volume mount and to pip...
dockerCmd.push('-v', `${windowsized}:${dockerDownloadCacheDir}:z`);
pipCmd.push('--cache-dir', dockerDownloadCacheDir);
Expand Down Expand Up @@ -262,7 +271,7 @@ function installRequirements(targetFolder, serverless, options) {
]);
} else {
// Use same user so --cache-dir works
dockerCmd.push('-u', getDockerUid(bindPath));
dockerCmd.push('-u', await getDockerUid(bindPath));
}

for (let path of options.dockerExtraFiles) {
Expand Down Expand Up @@ -315,22 +324,23 @@ function installRequirements(targetFolder, serverless, options) {

serverless.cli.log(`Running ${quote(dockerCmd)}...`);

filterCommands(mainCmds).forEach(([cmd, ...args]) => {
const res = spawnSync(cmd, args);
if (res.error) {
if (res.error.code === 'ENOENT') {
for (const [cmd, ...args] of mainCmds) {
try {
await spawn(cmd, args);
} catch (e) {
if (
e.stderrBuffer &&
e.stderrBuffer.toString().includes('command not found')
) {
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(`STDOUT: ${res.stdout}\n\nSTDERR: ${res.stderr}`);
throw e;
}
});
}
// If enabled slimming, delete files in slimPatterns
if (options.slim === true || options.slim === 'true') {
deleteFiles(options, targetFolder);
Expand Down Expand Up @@ -489,7 +499,7 @@ function requirementsFileExists(servicePath, options, fileName) {
* @param {Object} serverless
* @return {string}
*/
function installRequirementsIfNeeded(
async function installRequirementsIfNeeded(
servicePath,
modulePath,
options,
Expand Down Expand Up @@ -573,7 +583,7 @@ function installRequirementsIfNeeded(
fse.copySync(slsReqsTxt, path.join(workingReqsFolder, 'requirements.txt'));

// Then install our requirements from this folder
installRequirements(workingReqsFolder, serverless, options);
await installRequirements(workingReqsFolder, serverless, options);

// Copy vendor libraries to requirements folder
if (options.vendor) {
Expand All @@ -596,63 +606,64 @@ function installRequirementsIfNeeded(
* pip install the requirements to the requirements directory
* @return {undefined}
*/
function installAllRequirements() {
async function installAllRequirements() {
// fse.ensureDirSync(path.join(this.servicePath, '.serverless'));
// First, check and delete cache versions, if enabled
checkForAndDeleteMaxCacheVersions(this.options, this.serverless);

// Then if we're going to package functions individually...
if (this.serverless.service.package.individually) {
let doneModules = [];
this.targetFuncs
.filter((func) =>
(func.runtime || this.serverless.service.provider.runtime).match(
/^python.*/
)
const filteredFuncs = this.targetFuncs.filter((func) =>
(func.runtime || this.serverless.service.provider.runtime).match(
/^python.*/
)
.map((f) => {
if (!get(f, 'module')) {
set(f, ['module'], '.');
}
// If we didn't already process a module (functions can re-use modules)
if (!doneModules.includes(f.module)) {
const reqsInstalledAt = installRequirementsIfNeeded(
this.servicePath,
f.module,
this.options,
f,
this.serverless
);
// Add modulePath into .serverless for each module so it's easier for injecting and for users to see where reqs are
let modulePath = path.join(
this.servicePath,
'.serverless',
`${f.module}`,
'requirements'
);
// Only do if we didn't already do it
if (
reqsInstalledAt &&
!fse.existsSync(modulePath) &&
reqsInstalledAt != modulePath
) {
if (this.options.useStaticCache) {
// Windows can't symlink so we have to copy on Windows,
// it's not as fast, but at least it works
if (process.platform == 'win32') {
fse.copySync(reqsInstalledAt, modulePath);
} else {
fse.symlink(reqsInstalledAt, modulePath);
}
);

for (const f of filteredFuncs) {
if (!get(f, 'module')) {
set(f, ['module'], '.');
}

// If we didn't already process a module (functions can re-use modules)
if (!doneModules.includes(f.module)) {
const reqsInstalledAt = await installRequirementsIfNeeded(
this.servicePath,
f.module,
this.options,
f,
this.serverless
);
// Add modulePath into .serverless for each module so it's easier for injecting and for users to see where reqs are
let modulePath = path.join(
this.servicePath,
'.serverless',
`${f.module}`,
'requirements'
);
// Only do if we didn't already do it
if (
reqsInstalledAt &&
!fse.existsSync(modulePath) &&
reqsInstalledAt != modulePath
) {
if (this.options.useStaticCache) {
// Windows can't symlink so we have to copy on Windows,
// it's not as fast, but at least it works
if (process.platform == 'win32') {
fse.copySync(reqsInstalledAt, modulePath);
} else {
fse.rename(reqsInstalledAt, modulePath);
fse.symlink(reqsInstalledAt, modulePath);
}
} else {
fse.rename(reqsInstalledAt, modulePath);
}
doneModules.push(f.module);
}
});
doneModules.push(f.module);
}
}
} else {
const reqsInstalledAt = installRequirementsIfNeeded(
const reqsInstalledAt = await installRequirementsIfNeeded(
this.servicePath,
'',
this.options,
Expand Down
Loading