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 killing of child process on Windows #21

Closed
wants to merge 2 commits into from

Conversation

knight-bubble
Copy link

Since windows doesn't accept signals as unix does, the child processes should be killed in a different way. Also changed so it could be passed on windows. The 'exit 1' command returns 0 on windows.

@kimmobrunfeldt
Copy link
Contributor

Sorry this branch has now a few conflicts, I have been too late for merging this in. Could you rebase this branch with the new master?

@Spike2050
Copy link

+1

@filipesilva
Copy link
Collaborator

@kimmobrunfeldt any chance of getting this merged in? I'm really missing the windows support for a couple of projects I'm working in. Cheers!

@filipesilva
Copy link
Collaborator

@hirtie-maxim I tried your PR on Windows 10 and it works great.

A question though... is enforcing 'detached': true for the processes necessary? That way it's not possible to read through the output in logs, which is a bit problematic for automation.

Perhaps it could be made an option? I'm happy enough to do that after merge, if you don't have time. I could really use this PR in.

@knight-bubble
Copy link
Author

knight-bubble commented May 4, 2016

@filipesilva unfortunately I couldn't find a solution to fix that. But if you can find a way, then great!

@filipesilva
Copy link
Collaborator

I tried just setting detached:false and it worked well. What was the case where detached needed to be set to true?

@knight-bubble
Copy link
Author

Did you check it on linux? I pretty sure that there was a issue with that.

@filipesilva
Copy link
Collaborator

I did not, no. But I thought concurrently was already working correctly on Linux, and that this PR only fixed the windows issues.

@kimmobrunfeldt
Copy link
Contributor

All these issues should be fixed in master now.

@kimmobrunfeldt
Copy link
Contributor

These issues should be now fixed in 3.0.0-rc1. You can test by installing it: npm i -g concurrently@3.0.0-rc1. Please open a new issue to concurrently repo if this is not the case. I'll release the 3.0.0 soonish.

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.

None yet

4 participants