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.

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

fix: fix stdio and tons of windows issues #684

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jeanbmar
Copy link

@jeanbmar jeanbmar commented Mar 3, 2022

This PR adds back stdio for output priting and the shell: true to spawn method so it can properly run on Windows.
This was broken in 937fa56

This PR adds back stdio for output priting and the `shell: true` to spawn method so it can properly run in Windows.
This was broken in serverless@937fa56
@pgrzesik
Copy link
Contributor

pgrzesik commented Mar 3, 2022

Hello @jeanbmar - could you share more what exactly was broken? How can I reproduce it to validate the bug?

@jeanbmar
Copy link
Author

jeanbmar commented Mar 3, 2022

@pgrzesik

We can see in 937fa56 that:

const res = spawnSync(cmd, cmdOptions, spawnArgs);

was changed to:

const res = spawnSync(cmd, args);

4 years after the spawnArgs is still present in the code but used nowhere. Which leads me to believe that it should never have been removed in the first place and it wasn't intended.

spawnArgs contains 2 critical options for the spawn method.

stdio: 'inherit' is what allows stdout and stderr from the child process to be printed in the caller's console when SLS_DEBUG environment variable is true.
The PR and more changes that followed (like converting spawnSync to a regular spawn call) basically erased this feature from the module. As a result, people aren't even able to see what's the error when something goes wrong and hacky PR have been submitted to try fixing it. That's why so many people submit issues with Exit code 1 logs.
See: #663, #677, #673, #664 and many more.

This recent PR #679 is dirty because it will print interesting information when an exception is thrown. Except that bad things happening in Docker don't necessary crash the spawn process, and thus don't throw an exception.

To reproduce what I'm saying, add a private package to your requirements.txt and try building your python requirements without adding the SSH key. Your build will fail and you will get no log.
The std must be added back to the spawn call so people can understand what actually happens in Docker.
We should probably output std in every situation and remove the SLS_DEBUG check. As a relevant comparison, node-gyp outputs the build logs by default in npm install.

I come to the second point. As a work around for this lack of std, I ran the docker run of my failing build manually to actually understand what was going on. To my surprise, running the command manually in PowerShell worked properly.

This is because PowerShell uses a shell and the docker run call may contains stuff that needs a shell to run. Thus the shell: true option that was set before the killer PR happened.
For instance, in order to fix the private key issue that makes this module impossible to use on Windows with private packages (hello #488) I had to add:

dockerRunCmdExtraArgs:
      - '-e'
      - 'GIT_SSH_COMMAND="cp ~/.ssh/id_rsa ~/id_rsa && chmod 600 ~/id_rsa && ssh -i ~/id_rsa"'

This won't work if the spawn process isn't a shell because embedded commands can't be evaluated.
This actually kills the interest of options such as dockerRunCmdExtraArgs, dockerRunCmdExtraArgs and pipCmdExtraArgs since the smallest intelligent command will require a shell.

tl;dr
If you can't deploy your lambda, monkey patch pip.js with:

let spawnArgs = { shell: true };
spawnArgs.stdio = 'inherit';
/* ... */
await spawn(cmd, args, spawnArgs);

@pgrzesik
Copy link
Contributor

pgrzesik commented Oct 1, 2022

Hey @jeanbmar - sorry for not taking care of this PR earlier, I missed it, and then didn't have time to dedicate to this project. I'll try to review it more deeply in the coming days/weeks. Thanks for your work 🙇

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