Port streaming commands in bootstrap to BootstrapCommand and remove as_command_mut

src/bootstrap/src/core/build_steps/compile.rs

@@ -12,7 +12,6 @@ use std::ffi::OsStr;
 use std::io::BufReader;
 use std::io::prelude::*;
 use std::path::{Path, PathBuf};
-use std::process::Stdio;
 use std::{env, fs, str};
 
 use serde_derive::Deserialize;
@@ -2507,7 +2506,6 @@ pub fn stream_cargo(
     #[cfg(feature = "tracing")]
     let _run_span = crate::trace_cmd!(cmd);
 
-    let cargo = cmd.as_command_mut();
     // Instruct Cargo to give us json messages on stdout, critically leaving
     // stderr as piped so we can get those pretty colors.
     let mut message_format = if builder.config.json_output {
@@ -2519,27 +2517,24 @@ pub fn stream_cargo(
         message_format.push_str(",json-diagnostic-");
         message_format.push_str(s);
     }
-    cargo.arg("--message-format").arg(message_format).stdout(Stdio::piped());
+    cmd.arg("--message-format").arg(message_format);
 
     for arg in tail_args {
-        cargo.arg(arg);
+        cmd.arg(arg);
     }
 
-    builder.verbose(|| println!("running: {cargo:?}"));
+    builder.verbose(|| println!("running: {cmd:?}"));
 
-    if builder.config.dry_run() {
-        return true;
-    }
+    let streaming_command = cmd.stream_capture_stdout(&builder.config.exec_ctx);
 
-    let mut child = match cargo.spawn() {
-        Ok(child) => child,
-        Err(e) => panic!("failed to execute command: {cargo:?}\nERROR: {e}"),
+    let Some(mut streaming_command) = streaming_command else {
+        return true;
     };
 
     // Spawn Cargo slurping up its JSON output. We'll start building up the
     // `deps` array of all files it generated along with a `toplevel` array of
     // files we need to probe for later.
-    let stdout = BufReader::new(child.stdout.take().unwrap());
+    let stdout = BufReader::new(streaming_command.stdout.take().unwrap());
     for line in stdout.lines() {
         let line = t!(line);
         match serde_json::from_str::<CargoMessage<'_>>(&line) {
@@ -2556,13 +2551,14 @@ pub fn stream_cargo(
     }
 
     // Make sure Cargo actually succeeded after we read all of its stdout.
-    let status = t!(child.wait());
+    let status = t!(streaming_command.wait());
     if builder.is_verbose() && !status.success() {
         eprintln!(
-            "command did not execute successfully: {cargo:?}\n\
+            "command did not execute successfully: {cmd:?}\n\
                   expected success, got: {status}"
         );
     }
+
     status.success()
 }
 

src/bootstrap/src/utils/exec.rs

@@ -13,7 +13,9 @@ use std::fmt::{Debug, Formatter};
 use std::hash::Hash;
 use std::panic::Location;
 use std::path::Path;
-use std::process::{Child, Command, CommandArgs, CommandEnvs, ExitStatus, Output, Stdio};
+use std::process::{
+    Child, ChildStderr, ChildStdout, Command, CommandArgs, CommandEnvs, ExitStatus, Output, Stdio,
+};
 use std::sync::{Arc, Mutex};
 
 use build_helper::ci::CiEnv;
@@ -209,15 +211,14 @@ impl<'a> BootstrapCommand {
         exec_ctx.as_ref().start(self, OutputMode::Capture, OutputMode::Print)
     }
 
-    /// Provides access to the stdlib Command inside.
-    /// FIXME: This function should be eventually removed from bootstrap.
-    pub fn as_command_mut(&mut self) -> &mut Command {
-        // We proactively mark this command as executed since we can't be certain how the returned
-        // command will be handled. Caching must also be avoided here, as the inner command could be
-        // modified externally without us being aware.
-        self.mark_as_executed();
-        self.do_not_cache();
-        &mut self.command
+    /// Spawn the command in background, while capturing and returning stdout, and printing stderr.
+    /// Returns None in dry-mode
+    #[track_caller]
+    pub fn stream_capture_stdout(
+        &'a mut self,
+        exec_ctx: impl AsRef<ExecutionContext>,
+    ) -> Option<StreamingCommand> {
+        exec_ctx.as_ref().stream(self, OutputMode::Capture, OutputMode::Print)
     }
 
     /// Mark the command as being executed, disarming the drop bomb.
@@ -449,6 +450,12 @@ enum CommandState<'a> {
     },
 }
 
+pub struct StreamingCommand {
+    child: Child,
+    pub stdout: Option<ChildStdout>,
+    pub stderr: Option<ChildStderr>,
+}
+
 #[must_use]
 pub struct DeferredCommand<'a> {
     state: CommandState<'a>,
@@ -617,6 +624,33 @@ impl ExecutionContext {
         }
         exit!(1);
     }
+
+    /// Spawns the command with configured stdout and stderr handling.
+    ///
+    /// Returns None if in dry-run mode or Panics if the command fails to spawn.
+    pub fn stream(
+        &self,
+        command: &mut BootstrapCommand,
+        stdout: OutputMode,
+        stderr: OutputMode,
+    ) -> Option<StreamingCommand> {
+        command.mark_as_executed();
+        if !command.run_in_dry_run && self.dry_run() {
+            return None;
+        }
+        let cmd = &mut command.command;
+        cmd.stdout(stdout.stdio());
+        cmd.stderr(stderr.stdio());
+        let child = cmd.spawn();
+        let mut child = match child {
+            Ok(child) => child,
+            Err(e) => panic!("failed to execute command: {cmd:?}\nERROR: {e}"),
+        };
+
+        let stdout = child.stdout.take();
+        let stderr = child.stderr.take();
+        Some(StreamingCommand { child, stdout, stderr })
+    }
 }
 
 impl AsRef<ExecutionContext> for ExecutionContext {
@@ -625,6 +659,12 @@ impl AsRef<ExecutionContext> for ExecutionContext {
     }
 }
 
+impl StreamingCommand {
+    pub fn wait(mut self) -> Result<ExitStatus, std::io::Error> {
+        self.child.wait()
+    }
+}
+
 impl<'a> DeferredCommand<'a> {
     pub fn wait_for_output(self, exec_ctx: impl AsRef<ExecutionContext>) -> CommandOutput {
         match self.state {

src/bootstrap/src/utils/render_tests.rs

@@ -7,7 +7,7 @@
 //! to reimplement all the rendering logic in this module because of that.
 
 use std::io::{BufRead, BufReader, Read, Write};
-use std::process::{ChildStdout, Stdio};
+use std::process::ChildStdout;
 use std::time::Duration;
 
 use termcolor::{Color, ColorSpec, WriteColor};
@@ -34,50 +34,44 @@ pub(crate) fn try_run_tests(
     cmd: &mut BootstrapCommand,
     stream: bool,
 ) -> bool {
-    if builder.config.dry_run() {
-        cmd.mark_as_executed();
+    if run_tests(builder, cmd, stream) {
         return true;
     }
 
-    if !run_tests(builder, cmd, stream) {
-        if builder.fail_fast {
-            crate::exit!(1);
-        } else {
-            builder.config.exec_ctx().add_to_delay_failure(format!("{cmd:?}"));
-            false
-        }
-    } else {
-        true
+    if builder.fail_fast {
+        crate::exit!(1);
     }
+
+    builder.config.exec_ctx().add_to_delay_failure(format!("{cmd:?}"));
+
+    false
 }
 
 fn run_tests(builder: &Builder<'_>, cmd: &mut BootstrapCommand, stream: bool) -> bool {
-    let cmd = cmd.as_command_mut();
-    cmd.stdout(Stdio::piped());
-
     builder.verbose(|| println!("running: {cmd:?}"));
 
-    let mut process = cmd.spawn().unwrap();
+    let Some(mut streaming_command) = cmd.stream_capture_stdout(&builder.config.exec_ctx) else {
+        return true;
+    };
 
     // This runs until the stdout of the child is closed, which means the child exited. We don't
     // run this on another thread since the builder is not Sync.
-    let renderer = Renderer::new(process.stdout.take().unwrap(), builder);
+    let renderer = Renderer::new(streaming_command.stdout.take().unwrap(), builder);
     if stream {
         renderer.stream_all();
     } else {
         renderer.render_all();
     }
 
-    let result = process.wait_with_output().unwrap();
-    if !result.status.success() && builder.is_verbose() {
+    let status = streaming_command.wait().unwrap();
+    if !status.success() && builder.is_verbose() {
         println!(
             "\n\ncommand did not execute successfully: {cmd:?}\n\
-             expected success, got: {}",
-            result.status
+             expected success, got: {status}",
         );
     }
 
-    result.status.success()
+    status.success()
 }
 
 struct Renderer<'a> {