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

Implement two features: Creating OS pipes and redirecting stdio of child processes to File handles #24035

Closed
wants to merge 2 commits into from

Conversation

ipetkov
Copy link
Contributor

@ipetkov ipetkov commented Apr 3, 2015

This pull request implements two separate but related features:

  • Safely creating an OS pipe which will create two std::fs::File handles (thus inheriting all their benefits). There was a similar struct in the past, however, it exposed raw file descriptors and was rather unsafe. It is feature gated behind fs_pipe.
  • Allow for redirection of stdio of std::process::Commands to open file handles, this allows saving command output or piping output from two different Commands without having to manually buffer the data. This is currently feature gated behind process_redirect.

This allows for safely creating OS in-memory pipes and representing the
reader and writer ends simply as an `std::fs::File`
This allows redirection of stdio for `std::process::Command` to already
open file handles, or to OS pipes created via `std::fs::Pipe`
@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Oh wow, thanks for taking the initiative here! Also thanks for the quite comprehensive tests!

I think that the Windows implementation here may need some more work, however. We're trying to move away from the use of file descriptors as much as possible and currently Stdio::piped is the last holdout before we're able to remove them entirely. The current representation of a File on Windows as simple a libc::HANDLE is actually quite intentional, and it's fairly crucial that we keep it that way.

Would you be interested in investigating removing the usage of file descriptors on Windows here? I'd be willing to help out of course!

@alexcrichton alexcrichton assigned alexcrichton and unassigned brson Apr 4, 2015
@alexcrichton
Copy link
Member

This will also need an RFC to land first as there's quite a few vectors of design here which may want to be discussed in a broader forum than on this PR itself.

@ipetkov
Copy link
Contributor Author

ipetkov commented Apr 6, 2015

@alexcrichton I think it shouldn't be too difficult to refactor the Windows implementation to not rely on any file descriptors, so I'll definitely take a shot at it!

In the mean time I'll draft up an RFC for the proposed API additions (any any existing APIs that may be beneficial to tweak).

@alexcrichton
Copy link
Member

Thanks @ipetkov!

@bors
Copy link
Contributor

bors commented Apr 10, 2015

☔ The latest upstream changes (presumably #24034) made this pull request unmergeable. Please resolve the merge conflicts.

@ipetkov
Copy link
Contributor Author

ipetkov commented Apr 10, 2015

RFC for this feature: rust-lang/rfcs#1055

@alexcrichton
Copy link
Member

I'm gonna close this for now in favor of the RFC opened by @ipetkov.

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.

5 participants