Rollup merge of #126731 - Kobzol:bootstrap-cmd-refactor, r=onur-ozkan

Bootstrap command refactoring: refactor `BootstrapCommand` (step 1)

This PR is a first step towards https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap.

It refactors `BoostrapCommand` to get it closer to a state where it is an actual command wrapper that can be routed through a central place of command execution, and also to make the distinction between printing output vs handling output programatically clearer (since now it's a mess).

The existing usages of `BootstrapCommand` are complicated primarily because of different ways of handling output. There are commands that:
1) Want to eagerly print stdout/stderr of the executed command, plus print an error message if the command fails (output mode `PrintAll`). Note that this error message attempts to print stdout/stderr of the command when `-v` is enabled, but that will always be empty, since this mode uses `.status()` and not `.output()`.
2) Want to eagerly print stdout/stderr of the executed command, but do not print any additional error message if it fails (output mode `PrintOutput`)
3) Want to capture stdout/stderr of the executed command, but print an error message if it fails (output mode `PrintFailure`). This means that the user wants to either ignore the output or handle it programatically, but that's not obvious from the name.

The difference between 1) and 2) (unless explicitly specified) is determined dynamically based on the bootstrap verbosity level.

It is very difficult for me to wrap my head around all these modes. I think that in a future PR, we should split these axes into e.g. this:
1) Do I want to handle the output programmatically or print it to the terminal? This should be a separate axis, true/false. (Note that "hiding the output" essentially just means saying that I handle it programmatically, and then I ignore the output).
2) Do I want to print a message if the command fails? Yes/No/Based on verbosity (which would be the default).

Then there is also the failure mode, but that is relatively simple to handle, the command execution will just shutdown bootstrap (either eagerly or late) when the command fails.

Note that this is just a first refactoring steps, there are a lot of other things to be done, so some things might not look "final" yet. The next steps are (not necessarily in this order):
- Remove `run` and `run_cmd` and implement everything in terms of `run_tracked` and rename `run_tracked` to `run`
- Implement the refactoring specified above (change how output modes work)
- Modify `BootstrapCmd` so that it stores `Command` instead of `&mut Command` and remove all the annoying `BootstrapCmd::from` by changing `Command::new` to `BootstrapCmd::new`
- Refactor the rest of command executions not currently using `BootstrapCmd` that can access Builder to use the correct output and failure modes. This will include passing Builder to additional places.
- Handle the most complex cases, such as output streaming. That will probably need to be handled separately.
- Refactor the rest of commands that cannot access builder (e.g. `Config::parse`) by introducing a new command context that will be passed to these places, and then stored in `Builder`. Move certain fields (such as `fail_fast`) from `Builder` to the command context.
- Handle the co-operation of `Builder`, `Build`, `Config` and command context. There are some fields and logic used during command execution that are distributed amongst `Builder/Build/Config`, so it will require some refactoring to make it work if the execution will happen on a separate place (in the command context).
- Refactor logging of commands, so that it is either logged to a file or printed in a nice hierarchical way that cooperates with the `Step` debug hierarchical output.
- Implement profiling of commands (add command durations to the command log, print a log of slowest commands and their execution counts at the end of bootstrap execution, perhaps store command executions to `metrics.json`).
- Implement caching of commands.
- Implement testing of commands through snapshot tests/mocking.

Best reviewed commit by commit.

r? `@onur-ozkan`

Author: matthiaskrgr

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;
+use crate::utils::exec::{BootstrapCommand, OutputMode};
 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,
@@ -156,7 +156,10 @@ You can skip linkcheck with --skip src/tools/linkchecker"
         let _guard =
             builder.msg(Kind::Test, compiler.stage, "Linkcheck", bootstrap_host, bootstrap_host);
         let _time = helpers::timeit(builder);
-        builder.run_delaying_failure(linkchecker.arg(builder.out.join(host.triple).join("doc")));
+        builder.run_tracked(
+            BootstrapCommand::from(linkchecker.arg(builder.out.join(host.triple).join("doc")))
+                .delay_failure(),
+        );
     }
 
     fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
@@ -213,8 +216,11 @@ impl Step for HtmlCheck {
             builder,
         ));
 
-        builder.run_delaying_failure(
-            builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target)),
+        builder.run_tracked(
+            BootstrapCommand::from(
+                builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target)),
+            )
+            .delay_failure(),
         );
     }
 }
@@ -261,7 +267,7 @@ impl Step for Cargotest {
             .env("RUSTC", builder.rustc(compiler))
             .env("RUSTDOC", builder.rustdoc(compiler));
         add_rustdoc_cargo_linker_args(cmd, builder, compiler.host, LldThreads::No);
-        builder.run_delaying_failure(cmd);
+        builder.run_tracked(BootstrapCommand::from(cmd).delay_failure());
     }
 }
 
@@ -813,7 +819,7 @@ impl Step for RustdocTheme {
             .env("RUSTC_BOOTSTRAP", "1");
         cmd.args(linker_args(builder, self.compiler.host, LldThreads::No));
 
-        builder.run_delaying_failure(&mut cmd);
+        builder.run_tracked(BootstrapCommand::from(&mut cmd).delay_failure());
     }
 }
 
@@ -1093,7 +1099,7 @@ HELP: to skip test's attempt to check tidiness, pass `--skip src/tools/tidy` to
         }
 
         builder.info("tidy check");
-        builder.run_delaying_failure(&mut cmd);
+        builder.run_tracked(BootstrapCommand::from(&mut 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"]
@@ -2179,7 +2185,8 @@ impl BookTest {
             compiler.host,
         );
         let _time = helpers::timeit(builder);
-        let toolstate = if builder.run_delaying_failure(&mut rustbook_cmd) {
+        let cmd = BootstrapCommand::from(&mut rustbook_cmd).delay_failure();
+        let toolstate = if builder.run_tracked(cmd).is_success() {
             ToolState::TestPass
         } else {
             ToolState::TestFail
@@ -2312,7 +2319,8 @@ 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_quiet(&mut tool);
+        builder
+            .run_tracked(BootstrapCommand::from(&mut tool).output_mode(OutputMode::OnlyOnFailure));
         drop(guard);
         // The tests themselves need to link to std, so make sure it is
         // available.
@@ -2341,11 +2349,11 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) ->
     let test_args = builder.config.test_args().join(" ");
     cmd.arg("--test-args").arg(test_args);
 
-    if builder.config.verbose_tests {
-        builder.run_delaying_failure(&mut cmd)
-    } else {
-        builder.run_quiet_delaying_failure(&mut cmd)
+    let mut cmd = BootstrapCommand::from(&mut cmd).delay_failure();
+    if !builder.config.verbose_tests {
+        cmd = cmd.quiet();
     }
+    builder.run_tracked(cmd).is_success()
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -2370,7 +2378,8 @@ impl Step for RustcGuide {
 
         let src = builder.src.join(relative_path);
         let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook);
-        let toolstate = if builder.run_delaying_failure(rustbook_cmd.arg("linkcheck").arg(&src)) {
+        let cmd = BootstrapCommand::from(rustbook_cmd.arg("linkcheck").arg(&src)).delay_failure();
+        let toolstate = if builder.run_tracked(cmd).is_success() {
             ToolState::TestPass
         } else {
             ToolState::TestFail
@@ -2984,7 +2993,7 @@ impl Step for Bootstrap {
             .current_dir(builder.src.join("src/bootstrap/"));
         // NOTE: we intentionally don't pass test_args here because the args for unittest and cargo test are mutually incompatible.
         // Use `python -m unittest` manually if you want to pass arguments.
-        builder.run_delaying_failure(&mut check_bootstrap);
+        builder.run_tracked(BootstrapCommand::from(&mut check_bootstrap).delay_failure());
 
         let mut cmd = Command::new(&builder.initial_cargo);
         cmd.arg("test")
@@ -3061,7 +3070,7 @@ impl Step for TierCheck {
             self.compiler.host,
             self.compiler.host,
         );
-        builder.run_delaying_failure(&mut cargo.into());
+        builder.run_tracked(BootstrapCommand::from(&mut cargo.into()).delay_failure());
     }
 }
 
@@ -3147,7 +3156,7 @@ impl Step for RustInstaller {
         cmd.env("CARGO", &builder.initial_cargo);
         cmd.env("RUSTC", &builder.initial_rustc);
         cmd.env("TMP_DIR", &tmpdir);
-        builder.run_delaying_failure(&mut cmd);
+        builder.run_tracked(BootstrapCommand::from(&mut cmd).delay_failure());
     }
 
     fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {

src/bootstrap/src/lib.rs

@@ -23,7 +23,7 @@ use std::fmt::Display;
 use std::fs::{self, File};
 use std::io;
 use std::path::{Path, PathBuf};
-use std::process::{Command, Output, Stdio};
+use std::process::{Command, Stdio};
 use std::str;
 use std::sync::OnceLock;
 
@@ -41,7 +41,7 @@ use crate::core::builder::Kind;
 use crate::core::config::{flags, LldMode};
 use crate::core::config::{DryRun, Target};
 use crate::core::config::{LlvmLibunwind, TargetSelection};
-use crate::utils::exec::{BehaviorOnFailure, BootstrapCommand, OutputMode};
+use crate::utils::exec::{BehaviorOnFailure, BootstrapCommand, CommandOutput, OutputMode};
 use crate::utils::helpers::{self, dir_is_empty, exe, libdir, mtime, output, symlink_dir};
 
 mod core;
@@ -585,8 +585,8 @@ impl Build {
             BootstrapCommand::from(submodule_git().args(["diff-index", "--quiet", "HEAD"]))
                 .allow_failure()
                 .output_mode(match self.is_verbose() {
-                    true => OutputMode::PrintAll,
-                    false => OutputMode::PrintOutput,
+                    true => OutputMode::All,
+                    false => OutputMode::OnlyOutput,
                 }),
         );
         if has_local_modifications {
@@ -958,73 +958,36 @@ impl Build {
         })
     }
 
-    /// Runs a command, printing out nice contextual information if it fails.
-    fn run(&self, cmd: &mut Command) {
-        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) {
-        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 {
+    /// Execute a command and return its output.
+    fn run_tracked(&self, command: BootstrapCommand<'_>) -> CommandOutput {
         if self.config.dry_run() {
-            return true;
+            return CommandOutput::default();
         }
 
-        let command = cmd.into();
         self.verbose(|| println!("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),
+        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),
             ),
-            OutputMode::SuppressOnSuccess => (command.command.output(), true),
+            OutputMode::OnlyOnFailure => (command.command.output().map(|o| o.into()), 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 !output.is_success() {
             if print_error {
                 println!(
                     "\n\nCommand did not execute successfully.\
-                    \nExpected success, got: {}",
-                    output.status,
+                \nExpected success, got: {}",
+                    output.status(),
                 );
 
                 if !self.is_verbose() {
@@ -1034,37 +997,45 @@ impl Build {
                 self.verbose(|| {
                     println!(
                         "\nSTDOUT ----\n{}\n\
-                        STDERR ----\n{}\n",
-                        String::from_utf8_lossy(&output.stdout),
-                        String::from_utf8_lossy(&output.stderr)
+                    STDERR ----\n{}\n",
+                        output.stdout(),
+                        output.stderr(),
                     )
                 });
             }
-            Err(())
-        } else {
-            Ok(())
-        };
 
-        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 => {
+            match command.failure_behavior {
+                BehaviorOnFailure::DelayFail => {
+                    if self.fail_fast {
                         exit!(1);
                     }
-                    BehaviorOnFailure::Ignore => {}
+
+                    let mut failures = self.delayed_failures.borrow_mut();
+                    failures.push(format!("{command:?}"));
+                }
+                BehaviorOnFailure::Exit => {
+                    exit!(1);
                 }
-                false
+                BehaviorOnFailure::Ignore => {}
             }
         }
+        output
+    }
+
+    /// Runs a command, printing out nice contextual information if it fails.
+    fn run(&self, cmd: &mut Command) {
+        self.run_cmd(BootstrapCommand::from(cmd).fail_fast().output_mode(
+            match self.is_verbose() {
+                true => OutputMode::All,
+                false => OutputMode::OnlyOutput,
+            },
+        ));
+    }
+
+    /// 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 {
+        let command = cmd.into();
+        self.run_tracked(command).is_success()
     }
 
     /// Check if verbosity is greater than the `level`

src/bootstrap/src/utils/exec.rs

@@ -1,4 +1,4 @@
-use std::process::Command;
+use std::process::{Command, ExitStatus, Output};
 
 /// What should be done when the command fails.
 #[derive(Debug, Copy, Clone)]
@@ -16,19 +16,19 @@ pub enum BehaviorOnFailure {
 pub enum OutputMode {
     /// Print both the output (by inheriting stdout/stderr) and also the command itself, if it
     /// fails.
-    PrintAll,
+    All,
     /// Print the output (by inheriting stdout/stderr).
-    PrintOutput,
+    OnlyOutput,
     /// Suppress the output if the command succeeds, otherwise print the output.
-    SuppressOnSuccess,
+    OnlyOnFailure,
 }
 
 /// Wrapper around `std::process::Command`.
 #[derive(Debug)]
 pub struct BootstrapCommand<'a> {
     pub command: &'a mut Command,
     pub failure_behavior: BehaviorOnFailure,
-    pub output_mode: OutputMode,
+    pub output_mode: Option<OutputMode>,
 }
 
 impl<'a> BootstrapCommand<'a> {
@@ -44,17 +44,62 @@ impl<'a> BootstrapCommand<'a> {
         Self { failure_behavior: BehaviorOnFailure::Ignore, ..self }
     }
 
+    /// Do not print the output of the command, unless it fails.
+    pub fn quiet(self) -> Self {
+        self.output_mode(OutputMode::OnlyOnFailure)
+    }
+
     pub fn output_mode(self, output_mode: OutputMode) -> Self {
-        Self { output_mode, ..self }
+        Self { output_mode: Some(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::PrintAll,
-        }
+        Self { command, failure_behavior: BehaviorOnFailure::Exit, output_mode: None }
+    }
+}
+
+/// Represents the output of an executed process.
+#[allow(unused)]
+pub struct CommandOutput(Output);
+
+impl CommandOutput {
+    pub fn is_success(&self) -> bool {
+        self.0.status.success()
+    }
+
+    pub fn is_failure(&self) -> bool {
+        !self.is_success()
+    }
+
+    pub fn status(&self) -> ExitStatus {
+        self.0.status
+    }
+
+    pub fn stdout(&self) -> String {
+        String::from_utf8(self.0.stdout.clone()).expect("Cannot parse process stdout as UTF-8")
+    }
+
+    pub fn stderr(&self) -> String {
+        String::from_utf8(self.0.stderr.clone()).expect("Cannot parse process stderr as UTF-8")
+    }
+}
+
+impl Default for CommandOutput {
+    fn default() -> Self {
+        Self(Output { status: Default::default(), stdout: vec![], stderr: vec![] })
+    }
+}
+
+impl From<Output> for CommandOutput {
+    fn from(output: Output) -> Self {
+        Self(output)
+    }
+}
+
+impl From<ExitStatus> for CommandOutput {
+    fn from(status: ExitStatus) -> Self {
+        Self(Output { status, stdout: vec![], stderr: vec![] })
     }
 }