Consolidate semantics of failure behaviors and output capturing in BootstrapCommand

Author: Kobzol

src/bootstrap/src/core/build_steps/format.rs

@@ -160,13 +160,11 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
             override_builder.add(&format!("!{ignore}")).expect(&ignore);
         }
     }
-    let git_available =
-        build.run(helpers::git(None).print_on_failure().arg("--version")).is_success();
+    let git_available = build.run(helpers::git(None).capture().arg("--version")).is_success();
 
     let mut adjective = None;
     if git_available {
-        let in_working_tree =
-            build.run(helpers::git(Some(&build.src)).print_on_failure()).is_success();
+        let in_working_tree = build.run(helpers::git(Some(&build.src)).capture()).is_success();
         if in_working_tree {
             let untracked_paths_output = output(
                 &mut helpers::git(Some(&build.src))

src/bootstrap/src/core/build_steps/test.rs

@@ -26,7 +26,7 @@ use crate::core::builder::{Builder, Compiler, Kind, RunConfig, ShouldRun, Step};
 use crate::core::config::flags::get_completion;
 use crate::core::config::flags::Subcommand;
 use crate::core::config::TargetSelection;
-use crate::utils::exec::{BootstrapCommand, OutputMode};
+use crate::utils::exec::BootstrapCommand;
 use crate::utils::helpers::{
     self, add_link_lib_path, add_rustdoc_cargo_linker_args, dylib_path, dylib_path_var,
     linker_args, linker_flags, output, t, target_supports_cranelift_backend, up_to_date,
@@ -2309,7 +2309,7 @@ impl Step for ErrorIndex {
         let guard =
             builder.msg(Kind::Test, compiler.stage, "error-index", compiler.host, compiler.host);
         let _time = helpers::timeit(builder);
-        builder.run(tool.output_mode(OutputMode::OnlyOnFailure));
+        builder.run(tool.capture());
         drop(guard);
         // The tests themselves need to link to std, so make sure it is
         // available.
@@ -2340,7 +2340,7 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) ->
 
     cmd = cmd.delay_failure();
     if !builder.config.verbose_tests {
-        cmd = cmd.print_on_failure();
+        cmd = cmd.capture();
     }
     builder.run(cmd).is_success()
 }

src/bootstrap/src/lib.rs

@@ -607,7 +607,7 @@ impl Build {
         let output = self
             .run(
                 helpers::git(Some(&self.src))
-                    .quiet()
+                    .capture()
                     .args(["config", "--file"])
                     .arg(self.config.src.join(".gitmodules"))
                     .args(["--get-regexp", "path"]),
@@ -971,60 +971,62 @@ impl Build {
 
         self.verbose(|| println!("running: {command:?}"));
 
-        let output_mode = command.output_mode.unwrap_or_else(|| match self.is_verbose() {
-            true => OutputMode::All,
-            false => OutputMode::OnlyOutput,
-        });
-        let (output, print_error): (io::Result<CommandOutput>, bool) = match output_mode {
-            mode @ (OutputMode::All | OutputMode::OnlyOutput) => (
-                command.command.status().map(|status| status.into()),
-                matches!(mode, OutputMode::All),
-            ),
-            mode @ (OutputMode::OnlyOnFailure | OutputMode::Quiet) => (
-                command.command.output().map(|o| o.into()),
-                matches!(mode, OutputMode::OnlyOnFailure),
-            ),
+        let output: io::Result<CommandOutput> = match command.output_mode {
+            OutputMode::Print => command.command.status().map(|status| status.into()),
+            OutputMode::CaptureAll => command.command.output().map(|o| o.into()),
+            OutputMode::CaptureStdout => {
+                command.command.stderr(Stdio::inherit());
+                command.command.output().map(|o| o.into())
+            }
         };
 
         let output = match output {
             Ok(output) => output,
-            Err(e) => fail(&format!("failed to execute command: {:?}\nerror: {}", command, e)),
+            Err(e) => fail(&format!("failed to execute command: {command:?}\nerror: {e}")),
         };
         if !output.is_success() {
-            if print_error {
-                println!(
-                    "\n\nCommand did not execute successfully.\
-                \nExpected success, got: {}",
-                    output.status(),
-                );
+            use std::fmt::Write;
+
+            // Here we build an error message, and below we decide if it should be printed or not.
+            let mut message = String::new();
+            writeln!(
+                message,
+                "\n\nCommand {command:?} did not execute successfully.\
+            \nExpected success, got: {}",
+                output.status(),
+            )
+            .unwrap();
 
-                if !self.is_verbose() {
-                    println!("Add `-v` to see more details.\n");
-                }
+            // If the output mode is OutputMode::Print, the output has already been printed to
+            // stdout/stderr, and we thus don't have anything captured to print anyway.
+            if matches!(command.output_mode, OutputMode::CaptureAll | OutputMode::CaptureStdout) {
+                writeln!(message, "\nSTDOUT ----\n{}", output.stdout().trim()).unwrap();
 
-                self.verbose(|| {
-                    println!(
-                        "\nSTDOUT ----\n{}\n\
-                    STDERR ----\n{}\n",
-                        output.stdout(),
-                        output.stderr(),
-                    )
-                });
+                // Stderr is added to the message only if it was captured
+                if matches!(command.output_mode, OutputMode::CaptureAll) {
+                    writeln!(message, "\nSTDERR ----\n{}", output.stderr().trim()).unwrap();
+                }
             }
 
             match command.failure_behavior {
                 BehaviorOnFailure::DelayFail => {
                     if self.fail_fast {
+                        println!("{message}");
                         exit!(1);
                     }
 
                     let mut failures = self.delayed_failures.borrow_mut();
-                    failures.push(format!("{command:?}"));
+                    failures.push(message);
                 }
                 BehaviorOnFailure::Exit => {
+                    println!("{message}");
                     exit!(1);
                 }
-                BehaviorOnFailure::Ignore => {}
+                BehaviorOnFailure::Ignore => {
+                    // If failures are allowed, either the error has been printed already
+                    // (OutputMode::Print) or the user used a capture output mode and wants to
+                    // handle the error output on their own.
+                }
             }
         }
         output
@@ -1515,7 +1517,7 @@ impl Build {
             // (Note that we use a `..` range, not the `...` symmetric difference.)
             self.run(
                 helpers::git(Some(&self.src))
-                    .quiet()
+                    .capture()
                     .arg("rev-list")
                     .arg("--count")
                     .arg("--merges")

src/bootstrap/src/utils/exec.rs

@@ -13,18 +13,19 @@ pub enum BehaviorOnFailure {
     Ignore,
 }
 
-/// How should the output of the command be handled.
+/// How should the output of the command be handled (whether it should be captured or printed).
 #[derive(Debug, Copy, Clone)]
 pub enum OutputMode {
-    /// Print both the output (by inheriting stdout/stderr) and also the command itself, if it
-    /// fails.
-    All,
-    /// Print the output (by inheriting stdout/stderr).
-    OnlyOutput,
-    /// Suppress the output if the command succeeds, otherwise print the output.
-    OnlyOnFailure,
-    /// Suppress the output of the command.
-    Quiet,
+    /// Prints the stdout/stderr of the command to stdout/stderr of bootstrap (by inheriting these
+    /// streams).
+    /// Corresponds to calling `cmd.status()`.
+    Print,
+    /// Captures the stdout and stderr of the command into memory.
+    /// Corresponds to calling `cmd.output()`.
+    CaptureAll,
+    /// Captures the stdout of the command into memory, inherits stderr.
+    /// Corresponds to calling `cmd.output()`.
+    CaptureStdout,
 }
 
 /// Wrapper around `std::process::Command`.
@@ -34,26 +35,26 @@ pub enum OutputMode {
 /// If you want to delay failures until the end of bootstrap, use [delay_failure].
 ///
 /// By default, the command will print its stdout/stderr to stdout/stderr of bootstrap
-/// ([OutputMode::OnlyOutput]). If bootstrap uses verbose mode, then it will also print the
-/// command itself in case of failure ([OutputMode::All]).
-/// If you want to handle the output programmatically, use `output_mode(OutputMode::OnlyOnFailure)`,
-/// which will print the output only if the command fails.
+/// ([OutputMode::Print]).
+/// If you want to handle the output programmatically, use [BootstrapCommand::capture].
+///
+/// Bootstrap will print a debug log to stdout if the command fails and failure is not allowed.
 ///
 /// [allow_failure]: BootstrapCommand::allow_failure
 /// [delay_failure]: BootstrapCommand::delay_failure
 #[derive(Debug)]
 pub struct BootstrapCommand {
     pub command: Command,
     pub failure_behavior: BehaviorOnFailure,
-    pub output_mode: Option<OutputMode>,
+    pub output_mode: OutputMode,
 }
 
 impl BootstrapCommand {
     pub fn new<S: AsRef<OsStr>>(program: S) -> Self {
         Self {
             command: Command::new(program),
             failure_behavior: BehaviorOnFailure::Exit,
-            output_mode: None,
+            output_mode: OutputMode::Print,
         }
     }
 
@@ -110,18 +111,14 @@ impl BootstrapCommand {
         Self { failure_behavior: BehaviorOnFailure::Ignore, ..self }
     }
 
-    /// Do not print the output of the command, unless it fails.
-    pub fn print_on_failure(self) -> Self {
-        self.output_mode(OutputMode::OnlyOnFailure)
-    }
-
-    /// Do not print the output of the command.
-    pub fn quiet(self) -> Self {
-        self.output_mode(OutputMode::Quiet)
+    /// Capture the output of the command, do not print it.
+    pub fn capture(self) -> Self {
+        Self { output_mode: OutputMode::CaptureAll, ..self }
     }
 
-    pub fn output_mode(self, output_mode: OutputMode) -> Self {
-        Self { output_mode: Some(output_mode), ..self }
+    /// Capture stdout of the command, do not print it.
+    pub fn capture_stdout(self) -> Self {
+        Self { output_mode: OutputMode::CaptureStdout, ..self }
     }
 }
 
@@ -154,6 +151,10 @@ impl CommandOutput {
         String::from_utf8(self.0.stdout.clone()).expect("Cannot parse process stdout as UTF-8")
     }
 
+    pub fn stdout_if_ok(&self) -> Option<String> {
+        if self.is_success() { Some(self.stdout()) } else { None }
+    }
+
     pub fn stderr(&self) -> String {
         String::from_utf8(self.0.stderr.clone()).expect("Cannot parse process stderr as UTF-8")
     }