Auto merge of #127450 - Kobzol:bootstrap-cmd-refactor-5, r=onur-ozkan

Bootstrap command refactoring: improve debuggability (step 5)

Continuation of #127321.

This PR improves the debuggability of command execution, by improving the output logged when a command fails (it now includes the exact location where the command was created and where it was executed), and also by adding a "drop bomb", which will panic if a command was created, but not executed (which is probably a bug).

Here is how the output of a failed command looks like:
```
Command "git" "foo" "[bar]" (failure_mode=Exit, stdout_mode=Capture, stderr_mode=Capture) did not execute successfully.
Expected success, got exit status: 1
Created at: src/core/build_steps/compile.rs:1699:9
Executed at: src/core/build_steps/compile.rs:1699:58

STDOUT ----

STDERR ----
git: 'foo' is not a git command. See 'git --help'.
```

And this is what is printed if you forget to execute a command:
```
thread 'main' panicked at /projects/personal/rust/rust/src/tools/build_helper/src/drop_bomb/mod.rs:42:13:
command constructed at `src/core/build_steps/compile.rs:1699:9` was dropped without being executed: `git`
```

Best reviewed commit by commit.

Tracking issue: #126819

r? `@onur-ozkan`

Cargo.lock

@@ -3420,6 +3420,7 @@ version = "0.2.0"
 dependencies = [
  "ar",
  "bstr",
+ "build_helper",
  "gimli 0.28.1",
  "object 0.34.0",
  "regex",

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

@@ -2080,7 +2080,8 @@ pub fn stream_cargo(
     tail_args: Vec<String>,
     cb: &mut dyn FnMut(CargoMessage<'_>),
 ) -> bool {
-    let mut cargo = cargo.into_cmd().command;
+    let mut cmd = cargo.into_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 {

src/bootstrap/src/core/build_steps/dist.rs

@@ -1060,11 +1060,7 @@ impl Step for PlainSourceTarball {
                 cmd.arg("--sync").arg(manifest_path);
             }
 
-            let config = if !builder.config.dry_run() {
-                cmd.capture().run(builder).stdout()
-            } else {
-                String::new()
-            };
+            let config = cmd.capture().run(builder).stdout();
 
             let cargo_config_dir = plain_dst_src.join(".cargo");
             builder.create_dir(&cargo_config_dir);
@@ -2072,11 +2068,7 @@ fn maybe_install_llvm(
         let mut cmd = command(llvm_config);
         cmd.arg("--libfiles");
         builder.verbose(|| println!("running {cmd:?}"));
-        let files = if builder.config.dry_run() {
-            "".into()
-        } else {
-            cmd.capture_stdout().run(builder).stdout()
-        };
+        let files = cmd.capture_stdout().run(builder).stdout();
         let build_llvm_out = &builder.llvm_out(builder.config.build);
         let target_llvm_out = &builder.llvm_out(target);
         for file in files.trim_end().split(' ') {

src/bootstrap/src/core/build_steps/doc.rs

@@ -146,15 +146,20 @@ impl<P: Step> Step for RustbookSrc<P> {
         let out = out.join(&name);
         let index = out.join("index.html");
         let rustbook = builder.tool_exe(Tool::Rustbook);
-        let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook);
 
         if !builder.config.dry_run()
             && (!up_to_date(&src, &index) || !up_to_date(&rustbook, &index))
         {
             builder.info(&format!("Rustbook ({target}) - {name}"));
             let _ = fs::remove_dir_all(&out);
 
-            rustbook_cmd.arg("build").arg(&src).arg("-d").arg(&out).run(builder);
+            builder
+                .tool_cmd(Tool::Rustbook)
+                .arg("build")
+                .arg(&src)
+                .arg("-d")
+                .arg(&out)
+                .run(builder);
 
             for lang in &self.languages {
                 let out = out.join(lang);
@@ -253,10 +258,6 @@ impl Step for TheBook {
         // build the version info page and CSS
         let shared_assets = builder.ensure(SharedAssets { target });
 
-        // build the command first so we don't nest GHA groups
-        // FIXME: this doesn't do anything!
-        builder.rustdoc_cmd(compiler);
-
         // build the redirect pages
         let _guard = builder.msg_doc(compiler, "book redirect pages", target);
         for file in t!(fs::read_dir(redirect_path)) {

src/bootstrap/src/core/build_steps/llvm.rs

@@ -172,7 +172,7 @@ pub(crate) fn detect_llvm_sha(config: &Config, is_git: bool) -> String {
             // the LLVM shared object file is named `LLVM-12-rust-{version}-nightly`
             config.src.join("src/version"),
         ]);
-        output(&mut rev_list.command).trim().to_owned()
+        output(rev_list.as_command_mut()).trim().to_owned()
     } else if let Some(info) = channel::read_commit_info_file(&config.src) {
         info.sha.trim().to_owned()
     } else {
@@ -254,7 +254,7 @@ pub(crate) fn is_ci_llvm_modified(config: &Config) -> bool {
         // `true` here.
         let llvm_sha = detect_llvm_sha(config, true);
         let head_sha =
-            output(&mut helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD").command);
+            output(helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD").as_command_mut());
         let head_sha = head_sha.trim();
         llvm_sha == head_sha
     }

src/bootstrap/src/core/build_steps/setup.rs

@@ -484,7 +484,7 @@ impl Step for Hook {
 fn install_git_hook_maybe(config: &Config) -> io::Result<()> {
     let git = helpers::git(Some(&config.src))
         .args(["rev-parse", "--git-common-dir"])
-        .command
+        .as_command_mut()
         .output()
         .map(|output| {
             assert!(output.status.success(), "failed to run `git`");

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

@@ -471,16 +471,12 @@ impl Miri {
         // We re-use the `cargo` from above.
         cargo.arg("--print-sysroot");
 
-        if builder.config.dry_run() {
-            String::new()
-        } else {
-            builder.verbose(|| println!("running: {cargo:?}"));
-            let stdout = cargo.capture_stdout().run(builder).stdout();
-            // Output is "<sysroot>\n".
-            let sysroot = stdout.trim_end();
-            builder.verbose(|| println!("`cargo miri setup --print-sysroot` said: {sysroot:?}"));
-            sysroot.to_owned()
-        }
+        builder.verbose(|| println!("running: {cargo:?}"));
+        let stdout = cargo.capture_stdout().run(builder).stdout();
+        // Output is "<sysroot>\n".
+        let sysroot = stdout.trim_end();
+        builder.verbose(|| println!("`cargo miri setup --print-sysroot` said: {sysroot:?}"));
+        sysroot.to_owned()
     }
 }
 
@@ -1352,6 +1348,52 @@ impl Step for CrateRunMakeSupport {
     }
 }
 
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct CrateBuildHelper {
+    host: TargetSelection,
+}
+
+impl Step for CrateBuildHelper {
+    type Output = ();
+    const ONLY_HOSTS: bool = true;
+
+    fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
+        run.path("src/tools/build_helper")
+    }
+
+    fn make_run(run: RunConfig<'_>) {
+        run.builder.ensure(CrateBuildHelper { host: run.target });
+    }
+
+    /// Runs `cargo test` for build_helper.
+    fn run(self, builder: &Builder<'_>) {
+        let host = self.host;
+        let compiler = builder.compiler(0, host);
+
+        let mut cargo = tool::prepare_tool_cargo(
+            builder,
+            compiler,
+            Mode::ToolBootstrap,
+            host,
+            "test",
+            "src/tools/build_helper",
+            SourceType::InTree,
+            &[],
+        );
+        cargo.allow_features("test");
+        run_cargo_test(
+            cargo,
+            &[],
+            &[],
+            "build_helper",
+            "build_helper self test",
+            compiler,
+            host,
+            builder,
+        );
+    }
+}
+
 default_test!(Ui { path: "tests/ui", mode: "ui", suite: "ui" });
 
 default_test!(Crashes { path: "tests/crashes", mode: "crashes", suite: "crashes" });
@@ -2058,7 +2100,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
         cmd.arg("--nightly-branch").arg(git_config.nightly_branch);
 
         // FIXME: Move CiEnv back to bootstrap, it is only used here anyway
-        builder.ci_env.force_coloring_in_ci(&mut cmd.command);
+        builder.ci_env.force_coloring_in_ci(cmd.as_command_mut());
 
         #[cfg(feature = "build-metrics")]
         builder.metrics.begin_test_suite(

src/bootstrap/src/core/build_steps/toolstate.rs

@@ -106,7 +106,7 @@ fn check_changed_files(toolstates: &HashMap<Box<str>, ToolState>) {
         .arg("--name-status")
         .arg("HEAD")
         .arg("HEAD^")
-        .command
+        .as_command_mut()
         .output();
     let output = match output {
         Ok(o) => o,
@@ -329,7 +329,7 @@ fn checkout_toolstate_repo() {
         .arg("--depth=1")
         .arg(toolstate_repo())
         .arg(TOOLSTATE_DIR)
-        .command
+        .as_command_mut()
         .status();
     let success = match status {
         Ok(s) => s.success(),
@@ -343,8 +343,13 @@ fn checkout_toolstate_repo() {
 /// Sets up config and authentication for modifying the toolstate repo.
 fn prepare_toolstate_config(token: &str) {
     fn git_config(key: &str, value: &str) {
-        let status =
-            helpers::git(None).arg("config").arg("--global").arg(key).arg(value).command.status();
+        let status = helpers::git(None)
+            .arg("config")
+            .arg("--global")
+            .arg(key)
+            .arg(value)
+            .as_command_mut()
+            .status();
         let success = match status {
             Ok(s) => s.success(),
             Err(_) => false,
@@ -413,7 +418,7 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) {
             .arg("-a")
             .arg("-m")
             .arg(&message)
-            .command
+            .as_command_mut()
             .status());
         if !status.success() {
             success = true;
@@ -424,7 +429,7 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) {
             .arg("push")
             .arg("origin")
             .arg("master")
-            .command
+            .as_command_mut()
             .status());
         // If we successfully push, exit.
         if status.success() {
@@ -437,14 +442,14 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) {
             .arg("fetch")
             .arg("origin")
             .arg("master")
-            .command
+            .as_command_mut()
             .status());
         assert!(status.success());
         let status = t!(helpers::git(Some(Path::new(TOOLSTATE_DIR)))
             .arg("reset")
             .arg("--hard")
             .arg("origin/master")
-            .command
+            .as_command_mut()
             .status());
         assert!(status.success());
     }
@@ -460,7 +465,7 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) {
 /// `publish_toolstate.py` script if the PR passes all tests and is merged to
 /// master.
 fn publish_test_results(current_toolstate: &ToolstateData) {
-    let commit = t!(helpers::git(None).arg("rev-parse").arg("HEAD").command.output());
+    let commit = t!(helpers::git(None).arg("rev-parse").arg("HEAD").as_command_mut().output());
     let commit = t!(String::from_utf8(commit.stdout));
 
     let toolstate_serialized = t!(serde_json::to_string(&current_toolstate));

src/bootstrap/src/core/builder.rs

@@ -860,6 +860,7 @@ impl<'a> Builder<'a> {
                 test::Clippy,
                 test::CompiletestTest,
                 test::CrateRunMakeSupport,
+                test::CrateBuildHelper,
                 test::RustdocJSStd,
                 test::RustdocJSNotStd,
                 test::RustdocGUI,
@@ -2104,7 +2105,7 @@ impl<'a> Builder<'a> {
         // Try to use a sysroot-relative bindir, in case it was configured absolutely.
         cargo.env("RUSTC_INSTALL_BINDIR", self.config.bindir_relative());
 
-        self.ci_env.force_coloring_in_ci(&mut cargo.command);
+        self.ci_env.force_coloring_in_ci(cargo.as_command_mut());
 
         // When we build Rust dylibs they're all intended for intermediate
         // usage, so make sure we pass the -Cprefer-dynamic flag instead of

src/bootstrap/src/core/config/config.rs

@@ -1259,7 +1259,7 @@ impl Config {
         cmd.arg("rev-parse").arg("--show-cdup");
         // Discard stderr because we expect this to fail when building from a tarball.
         let output = cmd
-            .command
+            .as_command_mut()
             .stderr(std::process::Stdio::null())
             .output()
             .ok()
@@ -2163,7 +2163,7 @@ impl Config {
 
         let mut git = helpers::git(Some(&self.src));
         git.arg("show").arg(format!("{commit}:{}", file.to_str().unwrap()));
-        output(&mut git.command)
+        output(git.as_command_mut())
     }
 
     /// Bootstrap embeds a version number into the name of shared libraries it uploads in CI.
@@ -2469,11 +2469,11 @@ impl Config {
         // Look for a version to compare to based on the current commit.
         // Only commits merged by bors will have CI artifacts.
         let merge_base = output(
-            &mut helpers::git(Some(&self.src))
+            helpers::git(Some(&self.src))
                 .arg("rev-list")
                 .arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email))
                 .args(["-n1", "--first-parent", "HEAD"])
-                .command,
+                .as_command_mut(),
         );
         let commit = merge_base.trim_end();
         if commit.is_empty() {
@@ -2489,7 +2489,7 @@ impl Config {
             .args(["diff-index", "--quiet", commit])
             .arg("--")
             .args([self.src.join("compiler"), self.src.join("library")])
-            .command
+            .as_command_mut()
             .status())
         .success();
         if has_changes {
@@ -2563,11 +2563,11 @@ impl Config {
         // Look for a version to compare to based on the current commit.
         // Only commits merged by bors will have CI artifacts.
         let merge_base = output(
-            &mut helpers::git(Some(&self.src))
+            helpers::git(Some(&self.src))
                 .arg("rev-list")
                 .arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email))
                 .args(["-n1", "--first-parent", "HEAD"])
-                .command,
+                .as_command_mut(),
         );
         let commit = merge_base.trim_end();
         if commit.is_empty() {
@@ -2589,7 +2589,7 @@ impl Config {
             git.arg(top_level.join(path));
         }
 
-        let has_changes = !t!(git.command.status()).success();
+        let has_changes = !t!(git.as_command_mut().status()).success();
         if has_changes {
             if if_unchanged {
                 if self.verbose > 0 {

src/bootstrap/src/core/sanity.rs

@@ -209,7 +209,7 @@ than building it.
 
     #[cfg(not(feature = "bootstrap-self-test"))]
     let stage0_supported_target_list: HashSet<String> = crate::utils::helpers::output(
-        &mut command(&build.config.initial_rustc).args(["--print", "target-list"]).command,
+        command(&build.config.initial_rustc).args(["--print", "target-list"]).as_command_mut(),
     )
     .lines()
     .map(|s| s.to_string())