Auto merge of #116581 - Kobzol:bootstrap-cmd-run, r=onur-ozkan

Centralize command running in boostrap (part one)

This PR tries to consolidate the various `run, try_run, run_quiet, run_quiet_delaying_failure, run_delaying_failure` etc. methods on `Builder`. This PR only touches command execution which doesn't produce output that would be later read by bootstrap, and it also only refactors spawning of commands that happens after a builder is created (commands executed during download & git submodule checkout are left as-is, for now).

The `run_cmd` method is quite meaty, but I expect that it will be changing rapidly soon, so I considered it easy to kept everything in a single method, and only after things settle down a bit, then maybe again split it up a bit.

I still kept the original shortcut methods like `run_quiet_delaying_failure`, but they now only delegate to `run_cmd`. I tried to keep the original behavior (or as close to it as possible) for all the various commands, but it is a giant mess, so there may be some deviations. Notably, `cmd.output()` is now always called, instead of just `status()`, which was called previously in some situations.

Apart from the refactored methods, there is also `Config::try_run`, `check_run`, methods that run commands that produce output, oh my… that's left for follow-up PRs :)

The driving goal of this (and following) refactors is to centralize command execution in bootstrap on a single place, to make command mocking feasible.

r? `@onur-ozkan`

6 files changed, 158 insertions, 80 deletions

diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 831a86940fb..246d742a99f 100644
--- a/src/bootstrap/src/core/build_steps/test.rs
+++ b/src/bootstrap/src/core/build_steps/test.rs
@@ -27,6 +27,7 @@ use crate::core::config::flags::Subcommand;
 use crate::core::config::TargetSelection;
 use crate::utils;
 use crate::utils::cache::{Interned, INTERNER};
+use crate::utils::exec::BootstrapCommand;
 use crate::utils::helpers::{
     self, add_link_lib_path, dylib_path, dylib_path_var, output, t, up_to_date,
 };
@@ -808,8 +809,8 @@ impl Step for Clippy {
 
         let _guard = builder.msg_sysroot_tool(Kind::Test, compiler.stage, "clippy", host, host);
 
-        #[allow(deprecated)] // Clippy reports errors if it blessed the outputs
-        if builder.config.try_run(&mut cargo).is_ok() {
+        // Clippy reports errors if it blessed the outputs
+        if builder.run_cmd(BootstrapCommand::from(&mut cargo).allow_failure()) {
             // The tests succeeded; nothing to do.
             return;
         }
@@ -3094,7 +3095,7 @@ impl Step for CodegenCranelift {
             .arg("testsuite.extended_sysroot");
         cargo.args(builder.config.test_args());
 
-        #[allow(deprecated)]
-        builder.config.try_run(&mut cargo.into()).unwrap();
+        let mut cmd: Command = cargo.into();
+        builder.run_cmd(BootstrapCommand::from(&mut cmd).fail_fast());
     }
 }
diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs
index 5702fa62d7c..d5f759ea159 100644
--- a/src/bootstrap/src/core/build_steps/tool.rs
+++ b/src/bootstrap/src/core/build_steps/tool.rs
@@ -8,6 +8,7 @@ use crate::core::build_steps::toolstate::ToolState;
 use crate::core::builder::{Builder, Cargo as CargoCommand, RunConfig, ShouldRun, Step};
 use crate::core::config::TargetSelection;
 use crate::utils::channel::GitInfo;
+use crate::utils::exec::BootstrapCommand;
 use crate::utils::helpers::{add_dylib_path, exe, t};
 use crate::Compiler;
 use crate::Mode;
@@ -108,8 +109,8 @@ impl Step for ToolBuild {
         );
 
         let mut cargo = Command::from(cargo);
-        #[allow(deprecated)] // we check this in `is_optional_tool` in a second
-        let is_expected = builder.config.try_run(&mut cargo).is_ok();
+        // we check this in `is_optional_tool` in a second
+        let is_expected = builder.run_cmd(BootstrapCommand::from(&mut cargo).allow_failure());
 
         builder.save_toolstate(
             tool,
diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs
index 705c27bb922..8dd1a698dfa 100644
--- a/src/bootstrap/src/lib.rs
+++ b/src/bootstrap/src/lib.rs
@@ -23,11 +23,12 @@ use std::fmt::Display;
 use std::fs::{self, File};
 use std::io;
 use std::path::{Path, PathBuf};
-use std::process::{Command, Stdio};
+use std::process::{Command, Output, Stdio};
 use std::str;
 
 use build_helper::ci::{gha, CiEnv};
 use build_helper::exit;
+use build_helper::util::fail;
 use filetime::FileTime;
 use once_cell::sync::OnceCell;
 use termcolor::{ColorChoice, StandardStream, WriteColor};
@@ -39,10 +40,8 @@ use crate::core::config::flags;
 use crate::core::config::{DryRun, Target};
 use crate::core::config::{LlvmLibunwind, TargetSelection};
 use crate::utils::cache::{Interned, INTERNER};
-use crate::utils::helpers::{
-    self, dir_is_empty, exe, libdir, mtime, output, run, run_suppressed, symlink_dir,
-    try_run_suppressed,
-};
+use crate::utils::exec::{BehaviorOnFailure, BootstrapCommand, OutputMode};
+use crate::utils::helpers::{self, dir_is_empty, exe, libdir, mtime, output, symlink_dir};
 
 mod core;
 mod utils;
@@ -580,15 +579,15 @@ impl Build {
         }
 
         // Save any local changes, but avoid running `git stash pop` if there are none (since it will exit with an error).
-        #[allow(deprecated)] // diff-index reports the modifications through the exit status
-        let has_local_modifications = self
-            .config
-            .try_run(
+        // diff-index reports the modifications through the exit status
+        let has_local_modifications = !self.run_cmd(
+            BootstrapCommand::from(
                 Command::new("git")
                     .args(&["diff-index", "--quiet", "HEAD"])
                     .current_dir(&absolute_path),
             )
-            .is_err();
+            .allow_failure(),
+        );
         if has_local_modifications {
             self.run(Command::new("git").args(&["stash", "push"]).current_dir(&absolute_path));
         }
@@ -921,55 +920,103 @@ impl Build {
 
     /// Runs a command, printing out nice contextual information if it fails.
     fn run(&self, cmd: &mut Command) {
-        if self.config.dry_run() {
-            return;
-        }
-        self.verbose(&format!("running: {cmd:?}"));
-        run(cmd, self.is_verbose())
+        self.run_cmd(BootstrapCommand::from(cmd).fail_fast().output_mode(
+            match self.is_verbose() {
+                true => OutputMode::PrintAll,
+                false => OutputMode::PrintOutput,
+            },
+        ));
+    }
+
+    /// Runs a command, printing out contextual info if it fails, and delaying errors until the build finishes.
+    pub(crate) fn run_delaying_failure(&self, cmd: &mut Command) -> bool {
+        self.run_cmd(BootstrapCommand::from(cmd).delay_failure().output_mode(
+            match self.is_verbose() {
+                true => OutputMode::PrintAll,
+                false => OutputMode::PrintOutput,
+            },
+        ))
     }
 
     /// Runs a command, printing out nice contextual information if it fails.
     fn run_quiet(&self, cmd: &mut Command) {
-        if self.config.dry_run() {
-            return;
-        }
-        self.verbose(&format!("running: {cmd:?}"));
-        run_suppressed(cmd)
+        self.run_cmd(
+            BootstrapCommand::from(cmd).fail_fast().output_mode(OutputMode::SuppressOnSuccess),
+        );
     }
 
     /// Runs a command, printing out nice contextual information if it fails.
     /// Exits if the command failed to execute at all, otherwise returns its
     /// `status.success()`.
     fn run_quiet_delaying_failure(&self, cmd: &mut Command) -> bool {
+        self.run_cmd(
+            BootstrapCommand::from(cmd).delay_failure().output_mode(OutputMode::SuppressOnSuccess),
+        )
+    }
+
+    /// A centralized function for running commands that do not return output.
+    pub(crate) fn run_cmd<'a, C: Into<BootstrapCommand<'a>>>(&self, cmd: C) -> bool {
         if self.config.dry_run() {
             return true;
         }
-        if !self.fail_fast {
-            self.verbose(&format!("running: {cmd:?}"));
-            if !try_run_suppressed(cmd) {
-                let mut failures = self.delayed_failures.borrow_mut();
-                failures.push(format!("{cmd:?}"));
-                return false;
+
+        let command = cmd.into();
+        self.verbose(&format!("running: {command:?}"));
+
+        let (output, print_error) = match command.output_mode {
+            mode @ (OutputMode::PrintAll | OutputMode::PrintOutput) => (
+                command.command.status().map(|status| Output {
+                    status,
+                    stdout: Vec::new(),
+                    stderr: Vec::new(),
+                }),
+                matches!(mode, OutputMode::PrintAll),
+            ),
+            OutputMode::SuppressOnSuccess => (command.command.output(), true),
+        };
+
+        let output = match output {
+            Ok(output) => output,
+            Err(e) => fail(&format!("failed to execute command: {:?}\nerror: {}", command, e)),
+        };
+        let result = if !output.status.success() {
+            if print_error {
+                println!(
+                    "\n\ncommand did not execute successfully: {:?}\n\
+                    expected success, got: {}\n\n\
+                    stdout ----\n{}\n\
+                    stderr ----\n{}\n\n",
+                    command.command,
+                    output.status,
+                    String::from_utf8_lossy(&output.stdout),
+                    String::from_utf8_lossy(&output.stderr)
+                );
             }
+            Err(())
         } else {
-            self.run_quiet(cmd);
-        }
-        true
-    }
+            Ok(())
+        };
 
-    /// Runs a command, printing out contextual info if it fails, and delaying errors until the build finishes.
-    pub(crate) fn run_delaying_failure(&self, cmd: &mut Command) -> bool {
-        if !self.fail_fast {
-            #[allow(deprecated)] // can't use Build::try_run, that's us
-            if self.config.try_run(cmd).is_err() {
-                let mut failures = self.delayed_failures.borrow_mut();
-                failures.push(format!("{cmd:?}"));
-                return false;
+        match result {
+            Ok(_) => true,
+            Err(_) => {
+                match command.failure_behavior {
+                    BehaviorOnFailure::DelayFail => {
+                        if self.fail_fast {
+                            exit!(1);
+                        }
+
+                        let mut failures = self.delayed_failures.borrow_mut();
+                        failures.push(format!("{command:?}"));
+                    }
+                    BehaviorOnFailure::Exit => {
+                        exit!(1);
+                    }
+                    BehaviorOnFailure::Ignore => {}
+                }
+                false
             }
-        } else {
-            self.run(cmd);
         }
-        true
     }
 
     pub fn is_verbose_than(&self, level: usize) -> bool {
diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs
new file mode 100644
index 00000000000..9c1c21cd958
--- /dev/null
+++ b/src/bootstrap/src/utils/exec.rs
@@ -0,0 +1,60 @@
+use std::process::Command;
+
+/// What should be done when the command fails.
+#[derive(Debug, Copy, Clone)]
+pub enum BehaviorOnFailure {
+    /// Immediately stop bootstrap.
+    Exit,
+    /// Delay failure until the end of bootstrap invocation.
+    DelayFail,
+    /// Ignore the failure, the command can fail in an expected way.
+    Ignore,
+}
+
+/// How should the output of the command be handled.
+#[derive(Debug, Copy, Clone)]
+pub enum OutputMode {
+    /// Print both the output (by inheriting stdout/stderr) and also the command itself, if it
+    /// fails.
+    PrintAll,
+    /// Print the output (by inheriting stdout/stderr).
+    PrintOutput,
+    /// Suppress the output if the command succeeds, otherwise print the output.
+    SuppressOnSuccess,
+}
+
+/// Wrapper around `std::process::Command`.
+#[derive(Debug)]
+pub struct BootstrapCommand<'a> {
+    pub command: &'a mut Command,
+    pub failure_behavior: BehaviorOnFailure,
+    pub output_mode: OutputMode,
+}
+
+impl<'a> BootstrapCommand<'a> {
+    pub fn delay_failure(self) -> Self {
+        Self { failure_behavior: BehaviorOnFailure::DelayFail, ..self }
+    }
+
+    pub fn fail_fast(self) -> Self {
+        Self { failure_behavior: BehaviorOnFailure::Exit, ..self }
+    }
+
+    pub fn allow_failure(self) -> Self {
+        Self { failure_behavior: BehaviorOnFailure::Ignore, ..self }
+    }
+
+    pub fn output_mode(self, output_mode: OutputMode) -> Self {
+        Self { output_mode, ..self }
+    }
+}
+
+impl<'a> From<&'a mut Command> for BootstrapCommand<'a> {
+    fn from(command: &'a mut Command) -> Self {
+        Self {
+            command,
+            failure_behavior: BehaviorOnFailure::Exit,
+            output_mode: OutputMode::SuppressOnSuccess,
+        }
+    }
+}
diff --git a/src/bootstrap/src/utils/helpers.rs b/src/bootstrap/src/utils/helpers.rs
index bb84b70d987..b58a1c25842 100644
--- a/src/bootstrap/src/utils/helpers.rs
+++ b/src/bootstrap/src/utils/helpers.rs
@@ -3,7 +3,7 @@
 //! Simple things like testing the various filesystem operations here and there,
 //! not a lot of interesting happenings here unfortunately.
 
-use build_helper::util::{fail, try_run};
+use build_helper::util::fail;
 use std::env;
 use std::fs;
 use std::io;
@@ -216,12 +216,6 @@ pub fn is_valid_test_suite_arg<'a, P: AsRef<Path>>(
     }
 }
 
-pub fn run(cmd: &mut Command, print_cmd_on_fail: bool) {
-    if try_run(cmd, print_cmd_on_fail).is_err() {
-        crate::exit!(1);
-    }
-}
-
 pub fn check_run(cmd: &mut Command, print_cmd_on_fail: bool) -> bool {
     let status = match cmd.status() {
         Ok(status) => status,
@@ -239,32 +233,6 @@ pub fn check_run(cmd: &mut Command, print_cmd_on_fail: bool) -> bool {
     status.success()
 }
 
-pub fn run_suppressed(cmd: &mut Command) {
-    if !try_run_suppressed(cmd) {
-        crate::exit!(1);
-    }
-}
-
-pub fn try_run_suppressed(cmd: &mut Command) -> bool {
-    let output = match cmd.output() {
-        Ok(status) => status,
-        Err(e) => fail(&format!("failed to execute command: {cmd:?}\nerror: {e}")),
-    };
-    if !output.status.success() {
-        println!(
-            "\n\ncommand did not execute successfully: {:?}\n\
-             expected success, got: {}\n\n\
-             stdout ----\n{}\n\
-             stderr ----\n{}\n\n",
-            cmd,
-            output.status,
-            String::from_utf8_lossy(&output.stdout),
-            String::from_utf8_lossy(&output.stderr)
-        );
-    }
-    output.status.success()
-}
-
 pub fn make(host: &str) -> PathBuf {
     if host.contains("dragonfly")
         || host.contains("freebsd")
diff --git a/src/bootstrap/src/utils/mod.rs b/src/bootstrap/src/utils/mod.rs
index 7dcb6a82862..8ca22d00865 100644
--- a/src/bootstrap/src/utils/mod.rs
+++ b/src/bootstrap/src/utils/mod.rs
@@ -6,6 +6,7 @@ pub(crate) mod cache;
 pub(crate) mod cc_detect;
 pub(crate) mod channel;
 pub(crate) mod dylib;
+pub(crate) mod exec;
 pub(crate) mod helpers;
 pub(crate) mod job;
 #[cfg(feature = "build-metrics")]