-
Notifications
You must be signed in to change notification settings - Fork 442
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
Vendor dependency os_pipe
#822
Conversation
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Could you provide motivation for this change in the PR description? |
Its motivation was explained by @thomcc and @joshtriplett on zulip |
Jotting out a quick resummary is still good. |
Basically the reason is that the existing crate is not particularly portable, and this fixes those issues (it's not actually a straight vendoring). My stance is that this optimization is not really worth this complexity and non-portability (this PR even has to explicitly compile_error on non-windows/non-unix, which is something we would support prior to #780 so long as std::process/std::fs worked...), but we've already landed it, and this at least reduces the number of things people have to PR in order to get their targets working. |
@thomcc I just looked into rust's stdlib and I've found that process spawning is only supported on windows and unix. In the stdlib's
And I found that other than Unix and Windows, none support process spawning:
FYI, here is the link to @thomcc So no, my PR does not introduce any regression since |
@thomcc I also discovered a pattern in rust's stdlib: OS that does not support pipe creation also don't support process spawning. P.S. I was just wondering how many OSes support process spawning but does not support pipe creation to see how much regression/damage my PR introduced and whether @thomcc is right that it should be reverted, turns out there's none in rust stdlib. |
I'm wondering if we can simplify this substantially. The That then eliminates the need for I also don't think it's worth including the test-support binaries in |
Eliminate the separate PipeReader and PipeWriter types, and just use File. Remove unused windows-sys features. Remove added test infrastructure. Revert unrelated changes.
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
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.
This is a lot more reasonable. Thanks to both you and @joshtriplett for all the changes.
Signed-off-by: Jiahao XU Jiahao_XU@outlook.com