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.

Sign up for GitHub

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

Jump to bottom

Windows: CommandExt::async_pipes #97149

Merged
merged 1 commit into from
Jun 20, 2022
Merged

Windows: CommandExt::async_pipes #97149

merged 1 commit into from
Jun 20, 2022

Conversation

ChrisDenton
Copy link
Member

Discussed in tokio-rs/tokio#4670 was the need for third party crates to be able to force process::Command::spawn to create pipes as async.

This implements the suggestion for a async_pipes method that gives third party crates that option.

Example:

use std::process::{Command, Stdio};

Command::new("cmd")
    .async_pipes(true)
    .stdin(Stdio::piped())
    .stdout(Stdio::piped())
    .stderr(Stdio::piped())
    .spawn()
    .unwrap();

@ChrisDenton ChrisDenton added O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-process Area: `std::process` and `std::env` labels May 18, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 18, 2022
@ChrisDenton
Copy link
Member Author

r? rust-lang/libs-api

@rust-highfive rust-highfive assigned dtolnay and unassigned kennytm May 18, 2022
@ChrisDenton ChrisDenton mentioned this pull request May 18, 2022
Merged
@dtolnay dtolnay mentioned this pull request Jun 17, 2022
Closed
@dtolnay
Copy link
Member

dtolnay commented Jun 17, 2022

Seems okay to me. @m-ou-se could you take a look since you requested this API in tokio-rs/tokio#4670 (comment)?

r? m-ou-se

@dtolnay dtolnay assigned m-ou-se and unassigned dtolnay Jun 17, 2022
m-ou-se
m-ou-se approved these changes Jun 20, 2022
View reviewed changes
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. r=me after minor changes.

@ChrisDenton
Copy link
Member Author

@bors r=m-ou-se rollup

@bors
Copy link
Contributor

bors commented Jun 20, 2022

📌 Commit f1f614cd5ce8364d1fc5da103553a9c262c1d60f has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2022
@ChrisDenton
Copy link
Member Author

@bors r-

Oops, forgot to post the tracking issue for this one. I really shouldn't try to do two things at once. 🙂

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 20, 2022
@ChrisDenton ChrisDenton mentioned this pull request Jun 20, 2022
Open
3 tasks
@ChrisDenton
Copy link
Member Author

@bors r=m-ou-se rollup

@bors
Copy link
Contributor

bors commented Jun 20, 2022

📌 Commit 740a54c has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 20, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 20, 2022
@Dylan-DPC
Rollup merge of rust-lang#97150 - ChrisDenton:stdio-create_pipe, r=m-…
2807f28 
…ou-se

`Stdio::makes_pipe`

Wrappers around `std::process::Command` may want to be able to override pipe creation. However, [`std::process::Stdio`](https://doc.rust-lang.org/std/process/struct.Stdio.html) is opaque so there's no way to tell if `Command` was told to create new pipes or not.

This is in some ways a more generic (and cross-platform) alternative to rust-lang#97149. However, unlike that feature, this comes with the price of the user needing to actually create their own pipes rather than reusing the std one. So I think it stands (or not) on its own.

# Example

```rust
#![feature(stdio_makes_pipe)]
use std::process::Stdio;

let io = Stdio::piped();
assert_eq!(io.makes_pipe(), true);
```
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 20, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2022
@bors
Auto merge of rust-lang#98292 - Dylan-DPC:rollup-hueb8tm, r=Dylan-DPC
b12708f 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#93080 (Implement `core::slice::IterMut::as_mut_slice` and `impl<T> AsMut<[T]> for IterMut<'_, T>`)
 - rust-lang#94855 (Panic when advance_slices()'ing too far and update docs.)
 - rust-lang#96609 (Add `{Arc, Rc}::downcast_unchecked`)
 - rust-lang#96719 (Fix the generator example for `pin!()`)
 - rust-lang#97149 (Windows: `CommandExt::async_pipes`)
 - rust-lang#97150 (`Stdio::makes_pipe`)
 - rust-lang#97837 (Document Rust's stance on `/proc/self/mem`)
 - rust-lang#98159 (Include ForeignItem when visiting types for WF check)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 85f1de2 into rust-lang:master Jun 20, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-ou-se m-ou-se m-ou-se approved these changes

Assignees

@m-ou-se m-ou-se

Labels
A-process Area: `std::process` and `std::env` O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.
7 participants
@ChrisDenton @rust-highfive @dtolnay @bors @m-ou-se @kennytm @rustbot