Skip to content

Commit

Permalink
Auto merge of rust-lang#114590 - ijackson:stdio-stdio-2, r=dtolnay
Browse files Browse the repository at this point in the history
Allow redirecting subprocess stdout to our stderr etc. (redux)

This is the code from rust-lang#88561, tidied up, including review suggestions, and with the for-testing-only CI commit removed.  FCP for the API completed in rust-lang#88561.

I have made a new MR to facilitate review.  The discussion there is very cluttered and the branch is full of changes (in many cases as a result of changes to other Rust stdlib APIs since then).  Assuming this MR is approvedl we should close that one.

### Reviewer doing a de novo review

Just code review these four commits..  FCP discussion starts here: rust-lang#88561 (comment)

Portability tests: you can see that this branch works on Windows too by looking at the CI results in rust-lang#88561, which has the same code changes as this branch but with an additional "DO NOT MERGE" commit to make the Windows tests run.

### Reviewer doing an incremental review from some version of rust-lang#88561

Review the new commits since your last review.  I haven't force pushed the branch there.

git diff the two branches (eg `git diff 176886197d6..0842b69c219`).  You'll see that the only difference is in gitlab CI files.  You can also see that *this* MR doesn't touch those files.
  • Loading branch information
bors committed Sep 8, 2023
2 parents feb0673 + 36295fa commit 8f0fd01
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 11 deletions.
60 changes: 60 additions & 0 deletions library/std/src/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1501,6 +1501,66 @@ impl From<fs::File> for Stdio {
}
}

#[stable(feature = "stdio_from_stdio", since = "CURRENT_RUSTC_VERSION")]
impl From<io::Stdout> for Stdio {
/// Redirect command stdout/stderr to our stdout
///
/// # Examples
///
/// ```rust
/// #![feature(exit_status_error)]
/// use std::io;
/// use std::process::Command;
///
/// # fn test() -> Result<(), Box<dyn std::error::Error>> {
/// let output = Command::new("whoami")
// "whoami" is a command which exists on both Unix and Windows,
// and which succeeds, producing some stdout output but no stderr.
/// .stdout(io::stdout())
/// .output()?;
/// output.status.exit_ok()?;
/// assert!(output.stdout.is_empty());
/// # Ok(())
/// # }
/// #
/// # if cfg!(unix) {
/// # test().unwrap();
/// # }
/// ```
fn from(inherit: io::Stdout) -> Stdio {
Stdio::from_inner(inherit.into())
}
}

#[stable(feature = "stdio_from_stdio", since = "CURRENT_RUSTC_VERSION")]
impl From<io::Stderr> for Stdio {
/// Redirect command stdout/stderr to our stderr
///
/// # Examples
///
/// ```rust
/// #![feature(exit_status_error)]
/// use std::io;
/// use std::process::Command;
///
/// # fn test() -> Result<(), Box<dyn std::error::Error>> {
/// let output = Command::new("whoami")
/// .stdout(io::stderr())
/// .output()?;
/// output.status.exit_ok()?;
/// assert!(output.stdout.is_empty());
/// # Ok(())
/// # }
/// #
/// # if cfg!(unix) {
/// # test().unwrap();
/// # }
/// ```
fn from(inherit: io::Stderr) -> Stdio {
Stdio::from_inner(inherit.into())
}
}

/// Describes the result of a process after it has terminated.
///
/// This `struct` is used to represent the exit status or other termination of a child process.
Expand Down
30 changes: 29 additions & 1 deletion library/std/src/sys/unix/process/process_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::sys::fd::FileDesc;
use crate::sys::fs::File;
use crate::sys::pipe::{self, AnonPipe};
use crate::sys_common::process::{CommandEnv, CommandEnvs};
use crate::sys_common::IntoInner;
use crate::sys_common::{FromInner, IntoInner};

#[cfg(not(target_os = "fuchsia"))]
use crate::sys::fs::OpenOptions;
Expand Down Expand Up @@ -150,6 +150,7 @@ pub enum Stdio {
Null,
MakePipe,
Fd(FileDesc),
StaticFd(BorrowedFd<'static>),
}

#[derive(Copy, Clone, Debug, Eq, PartialEq)]
Expand Down Expand Up @@ -463,6 +464,11 @@ impl Stdio {
}
}

Stdio::StaticFd(fd) => {
let fd = FileDesc::from_inner(fd.try_clone_to_owned()?);
Ok((ChildStdio::Owned(fd), None))
}

Stdio::MakePipe => {
let (reader, writer) = pipe::anon_pipe()?;
let (ours, theirs) = if readable { (writer, reader) } else { (reader, writer) };
Expand Down Expand Up @@ -497,6 +503,28 @@ impl From<File> for Stdio {
}
}

impl From<io::Stdout> for Stdio {
fn from(_: io::Stdout) -> Stdio {
// This ought really to be is Stdio::StaticFd(input_argument.as_fd()).
// But AsFd::as_fd takes its argument by reference, and yields
// a bounded lifetime, so it's no use here. There is no AsStaticFd.
//
// Additionally AsFd is only implemented for the *locked* versions.
// We don't want to lock them here. (The implications of not locking
// are the same as those for process::Stdio::inherit().)
//
// Arguably the hypothetical AsStaticFd and AsFd<'static>
// should be implemented for io::Stdout, not just for StdoutLocked.
Stdio::StaticFd(unsafe { BorrowedFd::borrow_raw(libc::STDOUT_FILENO) })
}
}

impl From<io::Stderr> for Stdio {
fn from(_: io::Stderr) -> Stdio {
Stdio::StaticFd(unsafe { BorrowedFd::borrow_raw(libc::STDERR_FILENO) })
}
}

impl ChildStdio {
pub fn fd(&self) -> Option<c_int> {
match *self {
Expand Down
35 changes: 25 additions & 10 deletions library/std/src/sys/windows/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ pub struct Command {

pub enum Stdio {
Inherit,
InheritSpecific { from_stdio_id: c::DWORD },
Null,
MakePipe,
Pipe(AnonPipe),
Expand Down Expand Up @@ -555,17 +556,19 @@ fn program_exists(path: &Path) -> Option<Vec<u16>> {

impl Stdio {
fn to_handle(&self, stdio_id: c::DWORD, pipe: &mut Option<AnonPipe>) -> io::Result<Handle> {
match *self {
Stdio::Inherit => match stdio::get_handle(stdio_id) {
Ok(io) => unsafe {
let io = Handle::from_raw_handle(io);
let ret = io.duplicate(0, true, c::DUPLICATE_SAME_ACCESS);
io.into_raw_handle();
ret
},
// If no stdio handle is available, then propagate the null value.
Err(..) => unsafe { Ok(Handle::from_raw_handle(ptr::null_mut())) },
let use_stdio_id = |stdio_id| match stdio::get_handle(stdio_id) {
Ok(io) => unsafe {
let io = Handle::from_raw_handle(io);
let ret = io.duplicate(0, true, c::DUPLICATE_SAME_ACCESS);
io.into_raw_handle();
ret
},
// If no stdio handle is available, then propagate the null value.
Err(..) => unsafe { Ok(Handle::from_raw_handle(ptr::null_mut())) },
};
match *self {
Stdio::Inherit => use_stdio_id(stdio_id),
Stdio::InheritSpecific { from_stdio_id } => use_stdio_id(from_stdio_id),

Stdio::MakePipe => {
let ours_readable = stdio_id != c::STD_INPUT_HANDLE;
Expand Down Expand Up @@ -613,6 +616,18 @@ impl From<File> for Stdio {
}
}

impl From<io::Stdout> for Stdio {
fn from(_: io::Stdout) -> Stdio {
Stdio::InheritSpecific { from_stdio_id: c::STD_OUTPUT_HANDLE }
}
}

impl From<io::Stderr> for Stdio {
fn from(_: io::Stderr) -> Stdio {
Stdio::InheritSpecific { from_stdio_id: c::STD_ERROR_HANDLE }
}
}

////////////////////////////////////////////////////////////////////////////////
// Processes
////////////////////////////////////////////////////////////////////////////////
Expand Down

0 comments on commit 8f0fd01

Please sign in to comment.