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(core): combine serial and parallel execution + forward sigint to child process #13885

Merged
merged 3 commits into from
Mar 17, 2023

Conversation

caioaao
Copy link
Contributor

@caioaao caioaao commented Dec 17, 2022

Attempt at fixing #13766

Combined parallel and sync process spawning since parallel one was already dealing with capturing signals, so it seemed like the easiest way to fix it

@vercel
Copy link

vercel bot commented Dec 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 17, 2023 at 3:00PM (UTC)

@luxaritas
Copy link
Contributor

luxaritas commented Dec 30, 2022

Is this also related to (or even fixes) #11782? Maybe it needs to be expanded to run-script as well?

@caioaao
Copy link
Contributor Author

caioaao commented Jan 3, 2023

@luxaritas this fixes the run-commands executor, but I didn't check how the run-script executor is related

Copy link
Member

@AgentEnder AgentEnder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good change, but there are some weird formatting issues. I'll reformat it and address the minor comment I had, but this should be merged later today :)

@@ -208,6 +216,7 @@ function createProcess(
const processExitListener = () => childProcess.kill();
process.on('exit', processExitListener);
process.on('SIGTERM', processExitListener);
process.on('SIGINT', () => childProcess.kill('SIGINT'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets reuse the childProcessExitListener here

@AgentEnder
Copy link
Member

Huh, on second glance I don't have permission to push up to the branch on your fork. Can you run nx format after addressing the minor comment?

@luxaritas
Copy link
Contributor

Any chance we can get run-script addressed now as well?

@caioaao
Copy link
Contributor Author

caioaao commented Jan 13, 2023

@AgentEnder oh that's weird. I thought contributors had push access to forks. Will do!

@caioaao
Copy link
Contributor Author

caioaao commented Jan 13, 2023

@luxaritas I'm currently traveling, so I don't think I'll have time for going through run-scripts too. I might have time after Jan 23rd though

@caioaao
Copy link
Contributor Author

caioaao commented Mar 17, 2023

@AgentEnder thanks for fixing tests and pushing this over the finish line! Let me know if there are any pending changes that you need help with. I'm now back so I can spend some time on it

@FrozenPandaz FrozenPandaz merged commit fd11334 into nrwl:master Mar 17, 2023
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants