-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Implement subprocess using _posixsubprocess and _winapi #1962
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
Conversation
ae89e71
to
511ee56
Compare
@@ -101,7 +102,6 @@ uname = "0.1.1" | |||
crc32fast = "1.2.0" | |||
adler32 = "1.0.3" | |||
gethostname = "0.2.0" | |||
subprocess = "0.2.2" |
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.
I like the subprocess
library. What are we missing in the library? The maintainer is very responsive so we should consider just asking him to add what we need.
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.
maybe due to platform compatibility like _posixsubprocess?
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, it's generally more difficult to implement an API with a large surface area than it is to implement the low-level primitives it's based on. This also guarantees parity in the implementation, since there could be a subtle disparity between cpython's _posixsubprocess or windows subprocess implementation and the subprocess crate that causes some weird bug. This way we know that as long as the fork_exec
or CreateProcess
code (which are pretty small) are equivalent, then so should the subprocess
module.
non-subprocess stuff from #1962
129366c
to
04f3033
Compare
56157cf
to
e06641d
Compare
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 good. Sorry for the late review
No description provided.