feat(python): streaming bin script output #3794
Conversation
|
Note, the kernel recording tests needed to be disabled for the |
jmalins
force-pushed
the
streaming-bin-script-support
branch
from
1bfc2eb to
2a5b889
Compare
|
@RomainMuller, are there any changes you'd recommend to this PR? |
| } | ||
|
|
||
| return { | ||
| command: path.join(packageDir, scriptPath), |
There was a problem hiding this comment.
On Windows, this may only work if the caller spawns that commend via a shell, which is a relatively easy way to inadvertently make your code not work on Windows...
There would be a .cmd file that might be preferable to use as an entry point when on Windows if present...
There was a problem hiding this comment.
I don't disagree. I was taking a "do no harm" approach with this PR by just carrying forward the existing logic, which would also not work on Windows.
I don't have easy access to a Windows host to test and understand the auto-generated .cmd file mentioned here: https://docs.npmjs.com/cli/v9/configuring-npm/package-json#bin
Is it essential that Windows support be included in the PR, since this isn't a new regression?
There was a problem hiding this comment.
Is it essential that Windows support be included in the PR, since this isn't a new regression?
Well... no. I have this principle that I never demand contributors leave the place cleaner than they found it 😅
| // Make sure the current NODE_OPTIONS are honored if we shell out to node | ||
| NODE_OPTIONS: process.execArgv.join(' '), |
There was a problem hiding this comment.
Not everything in process.execArgv is guaranteed to be valid to use in NODE_OPTIONS... That'll be randomly problematic in certain environments.
There was a problem hiding this comment.
Same comment as above, this is a direct port of the existing invokeBinScript logic.
I think, in this case, the contents of process.execArgv is predictable because the JSII runtime spawns the node process. Python is currently the only runtime with bin script support and it appears to only pass --max-old-space-size=4069, which shouldn't present a problem:
https://github.com/aws/jsii/blob/main/packages/%40jsii/python-runtime/src/jsii/_kernel/providers/process.py#L252
Accept typing simplication Co-authored-by: Romain Marcadier <[email protected]>
Single error message for missing and/or no scripts defined. Co-authored-by: Romain Marcadier <[email protected]>
|
Hi @RomainMuller, Thanks |
I see you have become acquainted with one of the pains of my life! 🤣 |
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
|
@all-contributors add @jmalins for code |
|
@jmalins already contributed before to code |
|
Merging (with squash)... |
Resolves #3782.
Inverts control on bin script execution by returning the script to execute (as well as arguments and ENV variables), rather than running it within the kernel process. This allows the calling process to pipe the output streams, rather than the kernel buffering the output and returning it at the end of the synchronous call.
Also, adds more complete tests to the python runtime to validate
stderr, exit codes and streaming response.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.