Skip to content

Fix --cache-dir with Docker #145

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

Merged
merged 10 commits into from
Mar 20, 2018
Merged

Fix --cache-dir with Docker #145

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c0be594
Fix --cache-dir with Docker
kichik Feb 28, 2018
fb15f0a
Detect docker-machine uid
kichik Feb 28, 2018
f4a5442
Add test to verify
kichik Mar 1, 2018
a7be23a
Remove dependency
kichik Mar 2, 2018
ba6ec09
Try to support OS X too
kichik Mar 5, 2018
a49b3b7
Merge remote-tracking branch 'origin/master' into fix_docker_cache
kichik Mar 13, 2018
38dd31f
Merge branch 'master' into fix_docker_cache
Mar 19, 2018
d856a85
fix cache test for macs. stupid bsd sed again
dschep Mar 19, 2018
2646bfb
need locale stuff for ci
dschep Mar 20, 2018
ca40274
oops. negate -z test
dschep Mar 20, 2018
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
13 changes: 5 additions & 8 deletions lib/pip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ const path = require('path');
const get = require('lodash.get');
const set = require('lodash.set');
const {spawnSync} = require('child_process');
const {quote} = require('shell-quote');
const values = require('lodash.values');
const {buildImage, getBindPath} = require('./docker');

Expand Down Expand Up @@ -87,19 +86,17 @@ function installRequirements(requirementsPath, targetFolder, serverless, service
cmdOptions.push('-e', 'SSH_AUTH_SOCK=/tmp/ssh_sock');
}
if (process.platform === 'linux') {
// Set the ownership of the .serverless/requirements folder to current user
pipCmd = quote(pipCmd);
const chownCmd = quote([
'chown', '-R', `${process.getuid()}:${process.getgid()}`,
targetRequirementsFolder,
]);
pipCmd = ['/bin/bash', '-c', '"' + pipCmd + ' && ' + chownCmd + '"'];
// Use same user so requirements folder is not root and so --cache-dir works
cmdOptions.push('-u', `${process.getuid()}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

@cgrimal, AFAICT you added the chown stuff. Could you test this alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW tests passed on Linux with all Docker tests enabled. There was no problem deleting the .serverless folder.

// const stripCmd = quote([
// 'find', targetRequirementsFolder,
// '-name', '"*.so"',
// '-exec', 'strip', '{}', '\;',
// ]);
// pipCmd = ['/bin/bash', '-c', '"' + pipCmd + ' && ' + stripCmd + ' && ' + chownCmd + '"'];
} else if (process.platform === 'win32') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to an empty else (or add macOS/darwin)? and I'll have some one with a mac test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is Circle.CI configured to test on Mac OS X too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, AFAIK circle doesn't offer free mac os builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you be open to moving to Travis.CI? Mac builds are free there and I think they have a working Docker setup too.

// Use same user so --cache-dir works
cmdOptions.push('-u', '1000'); // TODO is this always the case?
}
cmdOptions.push(dockerImage);
cmdOptions.push(...pipCmd);
Expand Down