-
-
Couldn't load subscription status.
- Fork 39
Bumped child process packages and open up Windows support again #64
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
Bumped child process packages and open up Windows support again #64
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WyriHaximus Thanks for looking into this! Can you provide some instructions on how to best reproduce this on Windows? Is this covered by just running the test suite on Windows? I couldn't find any references in the updated dependencies, that's why I'm wondering what changes where required upstream.
@clue Whoops this was supposed to be a draft PR. Wanted to do one last test run before opening it. In short all communication on the underlying dependencies goes over sockets rather then STD* and they all support react/child-process v0.6 by passing an list of overwritten file descriptors.
165e223 to
68b50a5
Compare
ghost
commented
Jul 9, 2019
@WyriHaximus @clue 🏓 What's the current state on this PR?
I agree that supporting the latest ChildProcess component makes perfect sense, but I don't see how this currently supports Windows? Perhaps split this into a separate follow-up PR?
For the reference, in case anybody's interested, here's an example how one could use socket I/O to communicate with a child process on Windows: clue/reactphp-sqlite#13
ghost
commented
Oct 5, 2019
@WyriHaximus status?
@CharlotteDunois right!
@clue the messenger pool switched to fully using sockets in that release
@WyriHaximus That's great! Let's make this actionable, what makes this PR "WIP"?
@clue I can't remember 🤐 , will have a check on my windows box tomorrow
@clue Checked earlier today and it works on windows now. What do you think about adding an allowed to fail windows build on travis?
@WyriHaximus That would be fantastic! See reactphp/child-process#71 for possible Travis CI config.
@clue added it to this PR 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you may want to also update or overwrite the install and/or script instructions for Windows 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I missed that, will fix it later tonight 👍
871cf7d to
b06b908
Compare
ghost
commented
Oct 7, 2019
You'll probably want to add this utility method to the tests for cross compatibility paths. https://github.com/reactphp/filesystem/pull/69/files#diff-d4c8c6dc8769324fc27cfdac19f05cafR122-R131
@CharlotteDunois yup, I'll fix all the windows build issues in this PR 🤣
ghost
commented
Apr 29, 2020
@WyriHaximus status?
b06b908 to
0a8e671
Compare
@CharlotteDunois just rebased and pushed it, will have a better look tomorrow
With the latest child process related packages adding support for windows again we can also open support for it again in this package