fix: fix stdio and tons of windows issues

[jeanbmar]

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 https://github.com/serverless/serverless-python-requirements/commit/937fa564bf12fcb043678a0b48064bc60cbd2ea2

[pgrzesik]

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

[jeanbmar]

@pgrzesik

We can see in https://github.com/serverless/serverless-python-requirements/commit/937fa564bf12fcb043678a0b48064bc60cbd2ea2 that:

https://github.com/serverless/serverless-python-requirements/blob/f2d73eac40e332e59eec8aa50e5999d9462406af/lib/pip.js#L256

was changed to: https://github.com/serverless/serverless-python-requirements/blob/937fa564bf12fcb043678a0b48064bc60cbd2ea2/lib/pip.js#L275

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: https://github.com/serverless/serverless-python-requirements/issues/663, https://github.com/serverless/serverless-python-requirements/issues/677, https://github.com/serverless/serverless-python-requirements/issues/673, https://github.com/serverless/serverless-python-requirements/pull/664 and many more.

This recent PR https://github.com/serverless/serverless-python-requirements/pull/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 https://github.com/serverless/serverless-python-requirements/issues/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]

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 🙇