-
Notifications
You must be signed in to change notification settings - Fork 293
[v4.2.0] packaging fails with relative path requirements #245
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
Comments
I've got you, fix incoming... |
Thanks @AndrewFarley. I was just starting on this too. Removing targetFolder as the cwd in the spawn arguments seemed to do the trick for me. Haven't run the full test suite yet, so I'll just let you finish up you fix 😄 |
But I think the fix is only for non-docker. I don't think there's a reasonable way to make a path like that work with docker. |
@dschep Want to have a brief conversation? I already did a fix which is totally different... inside generate requirements I auto-prepended two more cd backwards. And I was going to add a "error" if you do this with docker enabled, thoughts?
|
Ah. yeah, I think I prefer my fix. There's no reason to run with the target as the working directory AFAICT. Tests seem to be passing with my change. This is the change: diff --git a/lib/pip.js b/lib/pip.js
index 044f6c7..39d52ab 100644
--- a/lib/pip.js
+++ b/lib/pip.js
@@ -227,7 +227,7 @@ function installRequirements(targetFolder, serverless, options) {
const preparedPath = dockerPathForWin(options, targetFolder);
cmdOptions.push(getStripCommand(options, preparedPath));
}
- let spawnArgs = { cwd: targetFolder, shell: true };
+ let spawnArgs = { shell: true };
if (process.env.SLS_DEBUG) {
spawnArgs.stdio = 'inherit';
} |
Yeah, I like your change a lot more. Let me try all tests with your change, and then throw a error incase this is present with docker enabled, eh? :) |
Cool, yeah I was thinking we could still incorporate your idea of throwing an error if you find a requirement starting with |
This fixes #245 by making it so that relative paths in dependencies resolve correctly
Hey @dschep @AndrewFarley |
I can merge it anytime, I was waiting for @AndrewFarley to push a few of his changes, but they aren't necessary for it to work. For now you can also just install the plugin from #246
|
I got pulled into work just merge that fix. :)
…On Mon, 17 Sep 2018 at 15:28, Daniel Schep ***@***.***> wrote:
I can merge it anytime, I was waiting for @AndrewFarley
<https://github.com/AndrewFarley> to push a few of his changes, but they
aren't necessary for it to work. For now you can also just install the
plugin from #246
<#246>
npm i github:unitedincome/serverless-python-requirements#rel-dep
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#245 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAcsk6SKY0DHCCo1_PFGMU-jrtlAz_SCks5ub6ONgaJpZM4Wq0IQ>
.
|
This fixes #245 by making it so that relative paths in dependencies resolve correctly
released in v4.2.4 |
Setup
OS: Linux Ubuntu 18.04 LTS
Working directory:
~/playground
serverless.yml:
requirements.txt:
Running
DEBUG_SLS=* serverless package
results in the following log:Let me know if you need any more info.
Thanks!
The text was updated successfully, but these errors were encountered: