Skip to content

clean up slim code using glob-all to make it more cross platform #227

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.

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

Merged
merged 9 commits into from
Aug 28, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class ServerlessPythonRequirements {
const options = Object.assign(
{
slim: false,
slimPatterns: false,
zip: false,
cleanupZipHelper: true,
invalidateCaches: false,
Expand Down
17 changes: 7 additions & 10 deletions lib/pip.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const get = require('lodash.get');
const set = require('lodash.set');
const { spawnSync } = require('child_process');
const { buildImage, getBindPath, getDockerUid } = require('./docker');
const { getSlimPackageCommands } = require('./slim');
const { getStripCommand, deleteFiles } = require('./slim');

/**
* Install requirements described in requirementsPath to targetFolder
Expand Down Expand Up @@ -106,12 +106,6 @@ function installRequirements(
if (process.platform === 'linux') {
// Use same user so requirements folder is not root and so --cache-dir works
cmdOptions.push('-u', `${process.getuid()}`);
// const stripCmd = quote([
// 'find', targetRequirementsFolder,
// '-name', '"*.so"',
// '-exec', 'strip', '{}', '\;',
// ]);
// pipCmd = ['/bin/bash', '-c', '"' + pipCmd + ' && ' + stripCmd + ' && ' + chownCmd + '"'];
} else {
// Use same user so --cache-dir works
cmdOptions.push('-u', getDockerUid(bindPath));
Expand All @@ -123,11 +117,10 @@ function installRequirements(
cmdOptions = pipCmd.slice(1);
}

// If enabled slimming, strip out the caches, tests and dist-infos
// If enabled slimming, strip so files
if (options.slim === true || options.slim === 'true') {
const preparedPath = dockerPathForWin(options, targetRequirementsFolder);
const slimCmd = getSlimPackageCommands(options, preparedPath);
cmdOptions.push(...slimCmd);
cmdOptions.push(getStripCommand(options, preparedPath));
}

const res = spawnSync(cmd, cmdOptions, { cwd: servicePath, shell: true });
Expand All @@ -145,6 +138,10 @@ function installRequirements(
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, targetRequirementsFolder);
}
}

/**
Expand Down
73 changes: 22 additions & 51 deletions lib/slim.js
Original file line number Diff line number Diff line change
@@ -1,59 +1,30 @@
const isWsl = require('is-wsl');
const glob = require('glob-all');
const fse = require('fs-extra');

/**
* Get commands to slim the installed requirements
* only for non-windows platforms:
* works for docker builds and when run on UNIX platforms (wsl included)
* @param {Object} options
* @param {string} folderPath
* @return {Array.<String>}
*/
function getSlimPackageCommands(options, folderPath) {
let stripCmd = [];
const getStripCommand = (options, folderPath) =>
process.platform !== 'win32' || isWsl || options.dockerizePip
? ` && find ${folderPath} -name "*.so" -exec strip {} \\;`
Copy link
Contributor

Choose a reason for hiding this comment

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

I have noticed that again changing
` && find ${folderPath} -name "*.so" -exec strip {} \\; ` to
either ` && find ${folderPath} -name "*.so" -exec strip {} \;`
or ` && find ${folderPath} -name "*.so" -exec strip {} ;`
makes it run as expected on Win10.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. pushing a commit with out the \\ to see how it fares in circle. It might have to be a platform dependent thing. I'd assume WSL should be treated like a *nix tho because it's running in as *nix shell.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dschep
Indeed, I was rather surprised too..
If it fails, could be interesting to also replace ; operator with +.

I did not have a good reason to use a semicolon, but it seems that + should also work and it does not generally need an escape: as mentioned here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion! Works great since strip supports getting multiple arguments.

: '';

// Default stripping is done for non-windows environments
if (process.platform !== 'win32' || isWsl) {
stripCmd = getDefaultSLimOptions(folderPath);

// If specified any custom patterns to remove
if (options.slimPatterns instanceof Array) {
// Add the custom specified patterns to remove to the default commands
const customPatterns = options.slimPatterns.map(pattern => {
return getRemovalCommand(folderPath, pattern);
});
stripCmd = stripCmd.concat(customPatterns);
const deleteFiles = (options, folderPath) => {
let patterns = [
'**/*.so',
Copy link
Contributor

Choose a reason for hiding this comment

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

Also deleteing the .so files might not necessarily be safe as these are extension modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DoH! Good catch. I didn't mean to delete those. that's what strip is for!

'**/*.py[c|o]',
'**/__pycache__*',
'**/*.dist-info*'
];
if (options.slimPatterns) {
patterns = patterns.concat(options.slimPatterns);
}
for (const pattern of patterns) {
for (const file of glob.sync(`${folderPath}/${pattern}`)) {
fse.removeSync(file);
}
}
return stripCmd;
}

/**
* Gets the commands to slim the default (safe) files:
* including removing caches, stripping compiled files, removing dist-infos
* @param {String} folderPath
* @return {Array}
*/
function getDefaultSLimOptions(folderPath) {
return [
`&& find ${folderPath} -name "*.so" -exec strip {} \\;`,
`&& find ${folderPath} -name "*.py[c|o]" -exec rm -rf {} +`,
`&& find ${folderPath} -type d -name "__pycache__*" -exec rm -rf {} +`,
`&& find ${folderPath} -type d -name "*.dist-info*" -exec rm -rf {} +`
];
}

/**
* Get the command created fromt he find and remove template:
* returns a string in form `&& find <folder> -name "<match>" -exec rm -rf {} +`
* @param {String} folderPath
* @param {String} removalMatch
* @return {String}
*/
function getRemovalCommand(folderPath, removalMatch) {
return `&& find ${folderPath} -type d -wholename "${removalMatch}" -exec rm -rf {} +`;
}
};

module.exports = {
getSlimPackageCommands,
getDefaultSLimOptions
getStripCommand,
deleteFiles
};
2 changes: 1 addition & 1 deletion tests/base/_slimPatterns.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
slimPatterns:
- "*.egg-info*"
- "**/*.egg-info*"
2 changes: 1 addition & 1 deletion tests/pipenv/_slimPatterns.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
slimPatterns:
- "*.egg-info*"
- "**/*.egg-info*"