feat(util/exec): use spawn instead of exec #16414
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Please dont mind the changes in |
|
Test run output using the driver function: Expand logsPost Run ps |
- Use child_process.spawn instead of child_process.exec
Gabriel-Ladzaretti
force-pushed
the
16197-handle-zombies-with-spawn
branch
from
746affd
to
9e86048
Compare
Gabriel-Ladzaretti
changed the title
This PR contains 1+2. above output includes all steps as a POC. |
- Use child_process.spawn instead of child_process.exec
- Use child_process.spawn instead of child_process.exec
- Use child_process.spawn instead of child_process.exec
- init spawn-util
- spawn-util
- init index-spawn.spec.ts
- fixed various tests
- fix all artifacts.spec.ts
- fix all artifacts.spec.ts
- fix npm post update imports
- revert renaming to minimize PR diff
- revert renaming to minimize PR diff
- revert renaming to minimize PR diff
- revert renaming to minimize PR diff
- revert renaming to minimize PR diff - destroy stdio when terminating child process
- delete and revert dev related changes
- fix support for windows
- handle SIGSTOP and such - add test coverage
|
This now behaves the same as before (using exec). works both on ubuntu and windows, the later must be run with ive added test coverage and tested this using a driver function but yet to run e2e tests. once these are done ill mark this as ready for review. |
- now converts to strings when resolving/rejecting
- logs improvements - force shell (exec like) - fix tests
viceice
reviewed
Jul 19, 2022
Gabriel-Ladzaretti
force-pushed
the
16197-handle-zombies-with-spawn
branch
from
241b2aa
to
3fa4f25
Compare
viceice
reviewed
Jul 22, 2022
# Conflicts: # lib/util/exec/index.spec.ts # test/exec-util.ts
|
Can you please do some real runs again with latest changes? |
Sure |
That should be enough |
|
This docker command was used to run all tests -
Relevant logs
Relevant logs
Relevant logs
Relevant logsBug reproduction
Relevant logs |
Relevant logs with the same Artifact error with branch main checked out |
viceice
changed the title
|
Classified as feature, so we get a single release. Hopefully we don't need to rollback this 🙃 |
|
would updating the branch dismiss approval? |
no if you are using the github button with merge commit . 🤗 |
|
🎉 This PR is included in version 32.124.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Use
child_process.spawninstead ofchild_process.execwhile preserving function arguments (i.e, encoding & maxBuffer).Context
Related to #16197
Documentation (please check one with an [x])
How I've tested my work (please tick one)
I have verified these changes via: