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.

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

Conversation

kichik
Copy link
Contributor

@kichik kichik commented Feb 28, 2018

Without supplying a user Docker defaulted to root
That caused pip to refuse to use any specified --cache-dir because it's owned by the user
Run Docker as the user so it has proper access to any --cache-dir

Without supplying a user Docker defaulted to root
That caused pip to refuse to use any specified --cache-dir because it's owned by the user
Run Docker as the user so it has proper access to any -cache-dir
Copy link
Contributor

@dschep dschep left a comment

Choose a reason for hiding this comment

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

Yeah, I'm only comfortable adding the -u 1000 if that's actually always the case. I imagine it depends on the docker image you use?

@kichik
Copy link
Contributor Author

kichik commented Feb 28, 2018

It depends on the virtual machine running Docker and I've never seen it not be 1000. The selected image should never affect this, because it's a property of the mounted files. I have added code to detect it.

I'm using alpine for consistency of the test. I want to be sure the used image has the right tools. This image is tiny. It's only 2mb, so it should be really good for this test.

@dschep
Copy link
Contributor

dschep commented Feb 28, 2018

Cool, that makes sense. Do you think this technique would work on macOS too?

@kichik
Copy link
Contributor Author

kichik commented Feb 28, 2018

I think it should. Regardless of the mapping the host OS has for file permissions, what's important is the how Docker, and therefore pip, sees it.

@kichik
Copy link
Contributor Author

kichik commented Mar 1, 2018

The failure seems unrelated. Some random failure to install serverless. Can you trigger a rebuild please?

@dschep
Copy link
Contributor

dschep commented Mar 1, 2018

Yup. Done.

lib/pip.js Outdated
// 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.

]);
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants