Remove temporary `BootstrapCommand` trait impls

9 files changed, 114 insertions, 143 deletions

diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs
index de3b938e427..c267d3b0c0c 100644
--- a/src/bootstrap/src/core/build_steps/compile.rs
+++ b/src/bootstrap/src/core/build_steps/compile.rs
@@ -2080,7 +2080,7 @@ pub fn stream_cargo(
     tail_args: Vec<String>,
     cb: &mut dyn FnMut(CargoMessage<'_>),
 ) -> bool {
-    let mut cargo = BootstrapCommand::from(cargo).command;
+    let mut cargo = cargo.into_cmd().command;
     // 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 {
diff --git a/src/bootstrap/src/core/build_steps/doc.rs b/src/bootstrap/src/core/build_steps/doc.rs
index 4a5af25b3b2..823e842693e 100644
--- a/src/bootstrap/src/core/build_steps/doc.rs
+++ b/src/bootstrap/src/core/build_steps/doc.rs
@@ -738,7 +738,7 @@ fn doc_std(
         format!("library{} in {} format", crate_description(requested_crates), format.as_str());
     let _guard = builder.msg_doc(compiler, description, target);
 
-    builder.run(cargo);
+    builder.run(cargo.into_cmd());
     builder.cp_link_r(&out_dir, out);
 }
 
@@ -863,7 +863,7 @@ impl Step for Rustc {
         let proc_macro_out_dir = builder.stage_out(compiler, Mode::Rustc).join("doc");
         symlink_dir_force(&builder.config, &out, &proc_macro_out_dir);
 
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
 
         if !builder.config.dry_run() {
             // Sanity check on linked compiler crates
@@ -996,7 +996,7 @@ macro_rules! tool_doc {
                 symlink_dir_force(&builder.config, &out, &proc_macro_out_dir);
 
                 let _guard = builder.msg_doc(compiler, stringify!($tool).to_lowercase(), target);
-                builder.run(cargo);
+                builder.run(cargo.into_cmd());
 
                 if !builder.config.dry_run() {
                     // Sanity check on linked doc directories
diff --git a/src/bootstrap/src/core/build_steps/format.rs b/src/bootstrap/src/core/build_steps/format.rs
index 4583eb1c6cd..0372360c3cb 100644
--- a/src/bootstrap/src/core/build_steps/format.rs
+++ b/src/bootstrap/src/core/build_steps/format.rs
@@ -160,16 +160,15 @@ 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().allow_failure().arg("--version"))
-        .is_success();
+    let git_available =
+        build.run(helpers::git(None).capture().allow_failure().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()
+                    .capture()
                     .allow_failure()
                     .arg("rev-parse")
                     .arg("--is-inside-work-tree"),
diff --git a/src/bootstrap/src/core/build_steps/run.rs b/src/bootstrap/src/core/build_steps/run.rs
index 22d5efa5d95..496fe446d72 100644
--- a/src/bootstrap/src/core/build_steps/run.rs
+++ b/src/bootstrap/src/core/build_steps/run.rs
@@ -158,7 +158,7 @@ impl Step for Miri {
         // after another --, so this must be at the end.
         miri.args(builder.config.args());
 
-        builder.run(miri);
+        builder.run(miri.into_cmd());
     }
 }
 
diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index 918e4e2b907..9fc337dfd50 100644
--- a/src/bootstrap/src/core/build_steps/test.rs
+++ b/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,
@@ -150,16 +150,13 @@ You can skip linkcheck with --skip src/tools/linkchecker"
         builder.default_doc(&[]);
 
         // Build the linkchecker before calling `msg`, since GHA doesn't support nested groups.
-        let mut linkchecker = builder.tool_cmd(Tool::Linkchecker);
+        let linkchecker = builder.tool_cmd(Tool::Linkchecker);
 
         // Run the linkchecker.
         let _guard =
             builder.msg(Kind::Test, compiler.stage, "Linkcheck", bootstrap_host, bootstrap_host);
         let _time = helpers::timeit(builder);
-        builder.run(
-            BootstrapCommand::from(linkchecker.arg(builder.out.join(host.triple).join("doc")))
-                .delay_failure(),
-        );
+        builder.run(linkchecker.delay_failure().arg(builder.out.join(host.triple).join("doc")));
     }
 
     fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
@@ -217,10 +214,7 @@ impl Step for HtmlCheck {
         ));
 
         builder.run(
-            BootstrapCommand::from(
-                builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target)),
-            )
-            .delay_failure(),
+            builder.tool_cmd(Tool::HtmlChecker).delay_failure().arg(builder.doc_out(self.target)),
         );
     }
 }
@@ -260,14 +254,13 @@ impl Step for Cargotest {
 
         let _time = helpers::timeit(builder);
         let mut cmd = builder.tool_cmd(Tool::CargoTest);
-        let cmd = cmd
-            .arg(&cargo)
+        cmd.arg(&cargo)
             .arg(&out_dir)
             .args(builder.config.test_args())
             .env("RUSTC", builder.rustc(compiler))
             .env("RUSTDOC", builder.rustdoc(compiler));
-        add_rustdoc_cargo_linker_args(cmd, builder, compiler.host, LldThreads::No);
-        builder.run(BootstrapCommand::from(cmd).delay_failure());
+        add_rustdoc_cargo_linker_args(&mut cmd, builder, compiler.host, LldThreads::No);
+        builder.run(cmd.delay_failure());
     }
 }
 
@@ -763,12 +756,12 @@ impl Step for Clippy {
         cargo.env("HOST_LIBS", host_libs);
 
         cargo.add_rustc_lib_path(builder);
-        let mut cargo = prepare_cargo_test(cargo, &[], &[], "clippy", compiler, host, builder);
+        let cargo = prepare_cargo_test(cargo, &[], &[], "clippy", compiler, host, builder);
 
         let _guard = builder.msg_sysroot_tool(Kind::Test, compiler.stage, "clippy", host, host);
 
         // Clippy reports errors if it blessed the outputs
-        if builder.run(BootstrapCommand::from(&mut cargo).allow_failure()).is_success() {
+        if builder.run(cargo.allow_failure()).is_success() {
             // The tests succeeded; nothing to do.
             return;
         }
@@ -821,7 +814,7 @@ impl Step for RustdocTheme {
             .env("RUSTC_BOOTSTRAP", "1");
         cmd.args(linker_args(builder, self.compiler.host, LldThreads::No));
 
-        builder.run(BootstrapCommand::from(&mut cmd).delay_failure());
+        builder.run(cmd.delay_failure());
     }
 }
 
@@ -1099,7 +1092,7 @@ HELP: to skip test's attempt to check tidiness, pass `--skip src/tools/tidy` to
         }
 
         builder.info("tidy check");
-        builder.run(BootstrapCommand::from(&mut cmd).delay_failure());
+        builder.run(cmd.delay_failure());
 
         builder.info("x.py completions check");
         let [bash, zsh, fish, powershell] = ["x.py.sh", "x.py.zsh", "x.py.fish", "x.py.ps1"]
@@ -1306,7 +1299,7 @@ impl Step for RunMakeSupport {
             &[],
         );
 
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
 
         let lib_name = "librun_make_support.rlib";
         let lib = builder.tools_dir(self.compiler).join(lib_name);
@@ -2187,7 +2180,7 @@ impl BookTest {
             compiler.host,
         );
         let _time = helpers::timeit(builder);
-        let cmd = BootstrapCommand::from(&mut rustbook_cmd).delay_failure();
+        let cmd = rustbook_cmd.delay_failure();
         let toolstate =
             if builder.run(cmd).is_success() { ToolState::TestPass } else { ToolState::TestFail };
         builder.save_toolstate(self.name, toolstate);
@@ -2318,7 +2311,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(BootstrapCommand::from(&mut 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.
@@ -2349,7 +2342,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()
 }
@@ -2375,10 +2368,13 @@ impl Step for RustcGuide {
         builder.update_submodule(&relative_path);
 
         let src = builder.src.join(relative_path);
-        let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook);
-        let cmd = BootstrapCommand::from(rustbook_cmd.arg("linkcheck").arg(&src)).delay_failure();
-        let toolstate =
-            if builder.run(cmd).is_success() { ToolState::TestPass } else { ToolState::TestFail };
+        let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook).delay_failure();
+        rustbook_cmd.arg("linkcheck").arg(&src);
+        let toolstate = if builder.run(rustbook_cmd).is_success() {
+            ToolState::TestPass
+        } else {
+            ToolState::TestFail
+        };
         builder.save_toolstate("rustc-dev-guide", toolstate);
     }
 }
@@ -3347,7 +3343,7 @@ impl Step for CodegenCranelift {
             .arg("testsuite.extended_sysroot");
         cargo.args(builder.config.test_args());
 
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
     }
 }
 
@@ -3472,6 +3468,6 @@ impl Step for CodegenGCC {
             .arg("--std-tests");
         cargo.args(builder.config.test_args());
 
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
     }
 }
diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs
index b3464043912..20c7a30f1a8 100644
--- a/src/bootstrap/src/core/build_steps/tool.rs
+++ b/src/bootstrap/src/core/build_steps/tool.rs
@@ -603,7 +603,7 @@ impl Step for Rustdoc {
             &self.compiler.host,
             &target,
         );
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
 
         // Cargo adds a number of paths to the dylib search path on windows, which results in
         // the wrong rustdoc being executed. To avoid the conflicting rustdocs, we name the "tool"
@@ -858,7 +858,7 @@ impl Step for LlvmBitcodeLinker {
             &self.extra_features,
         );
 
-        builder.run(cargo);
+        builder.run(cargo.into_cmd());
 
         let tool_out = builder
             .cargo_out(self.compiler, Mode::ToolRustc, self.target)
diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs
index 58d6e7a58e3..35de91c41d4 100644
--- a/src/bootstrap/src/core/builder.rs
+++ b/src/bootstrap/src/core/builder.rs
@@ -2398,6 +2398,10 @@ impl Cargo {
         cargo
     }
 
+    pub fn into_cmd(self) -> BootstrapCommand {
+        self.into()
+    }
+
     /// Same as `Cargo::new` except this one doesn't configure the linker with `Cargo::configure_linker`
     pub fn new_for_mir_opt_tests(
         builder: &Builder<'_>,
@@ -2622,9 +2626,3 @@ impl From<Cargo> for BootstrapCommand {
         cargo.command
     }
 }
-
-impl From<Cargo> for Command {
-    fn from(cargo: Cargo) -> Command {
-        BootstrapCommand::from(cargo).command
-    }
-}
diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs
index ae2982ea56f..ae3d18e8774 100644
--- a/src/bootstrap/src/lib.rs
+++ b/src/bootstrap/src/lib.rs
@@ -555,10 +555,7 @@ impl Build {
         // Save any local changes, but avoid running `git stash pop` if there are none (since it will exit with an error).
         // diff-index reports the modifications through the exit status
         let has_local_modifications = self
-            .run(
-                BootstrapCommand::from(submodule_git().args(["diff-index", "--quiet", "HEAD"]))
-                    .allow_failure(),
-            )
+            .run(submodule_git().allow_failure().args(["diff-index", "--quiet", "HEAD"]))
             .is_failure();
         if has_local_modifications {
             self.run(submodule_git().args(["stash", "push"]));
@@ -582,7 +579,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"]),
@@ -937,69 +934,71 @@ impl Build {
 
     /// Execute a command and return its output.
     /// This method should be used for all command executions in bootstrap.
-    fn run<C: Into<BootstrapCommand>>(&self, command: C) -> CommandOutput {
+    fn run<C: AsMut<BootstrapCommand>>(&self, mut command: C) -> CommandOutput {
         if self.config.dry_run() {
             return CommandOutput::default();
         }
 
-        let mut command = command.into();
+        let command = command.as_mut();
 
         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
@@ -1490,7 +1489,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")
diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs
index dc30876601c..5f9e1648161 100644
--- a/src/bootstrap/src/utils/exec.rs
+++ b/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,10 +35,10 @@ 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
@@ -45,12 +46,16 @@ pub enum OutputMode {
 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 {
-        Command::new(program).into()
+        Self {
+            command: Command::new(program),
+            failure_behavior: BehaviorOnFailure::Exit,
+            output_mode: OutputMode::Print,
+        }
     }
 
     pub fn arg<S: AsRef<OsStr>>(&mut self, arg: S) -> &mut Self {
@@ -106,52 +111,22 @@ 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 }
     }
 }
 
-/// FIXME: This implementation is temporary, until all `Command` invocations are migrated to
-/// `BootstrapCommand`.
-impl<'a> From<&'a mut BootstrapCommand> for BootstrapCommand {
-    fn from(command: &'a mut BootstrapCommand) -> Self {
-        // This is essentially a manual `Command::clone`
-        let mut cmd = Command::new(command.command.get_program());
-        if let Some(dir) = command.command.get_current_dir() {
-            cmd.current_dir(dir);
-        }
-        cmd.args(command.command.get_args());
-        for (key, value) in command.command.get_envs() {
-            match value {
-                Some(value) => {
-                    cmd.env(key, value);
-                }
-                None => {
-                    cmd.env_remove(key);
-                }
-            }
-        }
-        Self {
-            command: cmd,
-            output_mode: command.output_mode,
-            failure_behavior: command.failure_behavior,
-        }
-    }
-}
-
-impl From<Command> for BootstrapCommand {
-    fn from(command: Command) -> Self {
-        Self { command, failure_behavior: BehaviorOnFailure::Exit, output_mode: None }
+/// This implementation exists to make it possible to pass both [BootstrapCommand] and
+/// `&mut BootstrapCommand` to `Build.run()`.
+impl AsMut<BootstrapCommand> for BootstrapCommand {
+    fn as_mut(&mut self) -> &mut BootstrapCommand {
+        self
     }
 }
 
@@ -176,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")
     }