Auto merge of #127120 - Kobzol:bootstrap-cmd-refactor-3, r=onur-ozkan

Bootstrap command refactoring: consolidate output modes (step 3)

This PR is a continuation to #126731. It consolidates the output modes of bootstrap (`Print` vs `CaptureAll` vs `CaptureStdout`) and simplifies the logic around error printing (now a command error is always printed if the failure is not ignored). It also ports even more usages of `Command` to `BootstrapCommand`, most notably the git helpers and many usages of the `output` function.

The last commit was added because the third commit made two variants of the `Tool` enum unused (no idea why, but it seems to have been a false positive that they were used before).

It can be reviewed now, but I would wait with merging until at least a few days after #126731, just to catch any potential issues from that PR before we move further.

As a next step, I want to clean up the API of the command a little bit to make usage easier (currently it's a bit verbose), and then continue with the rest of the tasks from the tracking issue.

As always, best reviewed commit by commit.

Tracking issue: #126819

r? `@onur-ozkan`

try-job: aarch64-apple

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

@@ -29,7 +29,7 @@ use crate::core::builder::{Builder, Kind, PathSet, RunConfig, ShouldRun, Step, T
 use crate::core::config::{DebuginfoLevel, LlvmLibunwind, RustcLto, TargetSelection};
 use crate::utils::exec::BootstrapCommand;
 use crate::utils::helpers::{
-    exe, get_clang_cl_resource_dir, is_debug_info, is_dylib, output, symlink_dir, t, up_to_date,
+    exe, get_clang_cl_resource_dir, is_debug_info, is_dylib, symlink_dir, t, up_to_date,
 };
 use crate::LLVM_TOOLS;
 use crate::{CLang, Compiler, DependencyType, GitRepo, Mode};
@@ -1488,10 +1488,10 @@ pub fn compiler_file(
     if builder.config.dry_run() {
         return PathBuf::new();
     }
-    let mut cmd = Command::new(compiler);
+    let mut cmd = BootstrapCommand::new(compiler);
     cmd.args(builder.cflags(target, GitRepo::Rustc, c));
     cmd.arg(format!("-print-file-name={file}"));
-    let out = output(&mut cmd);
+    let out = builder.run(cmd.capture_stdout()).stdout();
     PathBuf::from(out.trim())
 }
 
@@ -1836,7 +1836,9 @@ impl Step for Assemble {
             let llvm::LlvmResult { llvm_config, .. } =
                 builder.ensure(llvm::Llvm { target: target_compiler.host });
             if !builder.config.dry_run() && builder.config.llvm_tools_enabled {
-                let llvm_bin_dir = output(Command::new(llvm_config).arg("--bindir"));
+                let llvm_bin_dir = builder
+                    .run(BootstrapCommand::new(llvm_config).capture_stdout().arg("--bindir"))
+                    .stdout();
                 let llvm_bin_dir = Path::new(llvm_bin_dir.trim());
 
                 // Since we've already built the LLVM tools, install them to the sysroot.
@@ -2080,7 +2082,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 {
@@ -2161,8 +2163,7 @@ pub fn strip_debug(builder: &Builder<'_>, target: TargetSelection, path: &Path)
     }
 
     let previous_mtime = FileTime::from_last_modification_time(&path.metadata().unwrap());
-    // NOTE: `output` will propagate any errors here.
-    output(Command::new("strip").arg("--strip-debug").arg(path));
+    builder.run(BootstrapCommand::new("strip").capture().arg("--strip-debug").arg(path));
 
     // After running `strip`, we have to set the file modification time to what it was before,
     // otherwise we risk Cargo invalidating its fingerprint and rebuilding the world next time

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

@@ -14,7 +14,6 @@ use std::ffi::OsStr;
 use std::fs;
 use std::io::Write;
 use std::path::{Path, PathBuf};
-use std::process::Command;
 
 use object::read::archive::ArchiveFile;
 use object::BinaryFormat;
@@ -28,7 +27,7 @@ use crate::core::config::TargetSelection;
 use crate::utils::channel::{self, Info};
 use crate::utils::exec::BootstrapCommand;
 use crate::utils::helpers::{
-    exe, is_dylib, move_file, output, t, target_supports_cranelift_backend, timeit,
+    exe, is_dylib, move_file, t, target_supports_cranelift_backend, timeit,
 };
 use crate::utils::tarball::{GeneratedTarball, OverlayKind, Tarball};
 use crate::{Compiler, DependencyType, Mode, LLVM_TOOLS};
@@ -181,9 +180,9 @@ fn make_win_dist(
     }
 
     //Ask gcc where it keeps its stuff
-    let mut cmd = Command::new(builder.cc(target));
+    let mut cmd = BootstrapCommand::new(builder.cc(target));
     cmd.arg("-print-search-dirs");
-    let gcc_out = output(&mut cmd);
+    let gcc_out = builder.run(cmd.capture_stdout()).stdout();
 
     let mut bin_path: Vec<_> = env::split_paths(&env::var_os("PATH").unwrap_or_default()).collect();
     let mut lib_path = Vec::new();
@@ -1024,7 +1023,7 @@ impl Step for PlainSourceTarball {
             }
 
             // Vendor all Cargo dependencies
-            let mut cmd = Command::new(&builder.initial_cargo);
+            let mut cmd = BootstrapCommand::new(&builder.initial_cargo);
             cmd.arg("vendor")
                 .arg("--versioned-dirs")
                 .arg("--sync")
@@ -1062,7 +1061,7 @@ impl Step for PlainSourceTarball {
             }
 
             let config = if !builder.config.dry_run() {
-                t!(String::from_utf8(t!(cmd.output()).stdout))
+                builder.run(cmd.capture()).stdout()
             } else {
                 String::new()
             };
@@ -2079,10 +2078,14 @@ fn maybe_install_llvm(
     } else if let llvm::LlvmBuildStatus::AlreadyBuilt(llvm::LlvmResult { llvm_config, .. }) =
         llvm::prebuilt_llvm_config(builder, target)
     {
-        let mut cmd = Command::new(llvm_config);
+        let mut cmd = BootstrapCommand::new(llvm_config);
         cmd.arg("--libfiles");
         builder.verbose(|| println!("running {cmd:?}"));
-        let files = if builder.config.dry_run() { "".into() } else { output(&mut cmd) };
+        let files = if builder.config.dry_run() {
+            "".into()
+        } else {
+            builder.run(cmd.capture_stdout()).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

@@ -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

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

@@ -1,13 +1,14 @@
 //! Runs rustfmt on the repository.
 
 use crate::core::builder::Builder;
-use crate::utils::helpers::{self, output, program_out_of_date, t};
+use crate::utils::exec::BootstrapCommand;
+use crate::utils::helpers::{self, program_out_of_date, t};
 use build_helper::ci::CiEnv;
 use build_helper::git::get_git_modified_files;
 use ignore::WalkBuilder;
 use std::collections::VecDeque;
 use std::path::{Path, PathBuf};
-use std::process::{Command, Stdio};
+use std::process::Command;
 use std::sync::mpsc::SyncSender;
 use std::sync::Mutex;
 
@@ -53,19 +54,17 @@ fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl F
 fn get_rustfmt_version(build: &Builder<'_>) -> Option<(String, PathBuf)> {
     let stamp_file = build.out.join("rustfmt.stamp");
 
-    let mut cmd = Command::new(match build.initial_rustfmt() {
+    let mut cmd = BootstrapCommand::new(match build.initial_rustfmt() {
         Some(p) => p,
         None => return None,
     });
     cmd.arg("--version");
-    let output = match cmd.output() {
-        Ok(status) => status,
-        Err(_) => return None,
-    };
-    if !output.status.success() {
+
+    let output = build.run(cmd.capture().allow_failure());
+    if output.is_failure() {
         return None;
     }
-    Some((String::from_utf8(output.stdout).unwrap(), stamp_file))
+    Some((output.stdout(), stamp_file))
 }
 
 /// Return whether the format cache can be reused.
@@ -160,36 +159,31 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
             override_builder.add(&format!("!{ignore}")).expect(&ignore);
         }
     }
-    let git_available = match helpers::git(None)
-        .arg("--version")
-        .stdout(Stdio::null())
-        .stderr(Stdio::null())
-        .status()
-    {
-        Ok(status) => status.success(),
-        Err(_) => false,
-    };
+    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 = match helpers::git(Some(&build.src))
-            .arg("rev-parse")
-            .arg("--is-inside-work-tree")
-            .stdout(Stdio::null())
-            .stderr(Stdio::null())
-            .status()
-        {
-            Ok(status) => status.success(),
-            Err(_) => false,
-        };
-        if in_working_tree {
-            let untracked_paths_output = output(
+        let in_working_tree = build
+            .run(
                 helpers::git(Some(&build.src))
-                    .arg("status")
-                    .arg("--porcelain")
-                    .arg("-z")
-                    .arg("--untracked-files=normal"),
-            );
+                    .capture()
+                    .allow_failure()
+                    .arg("rev-parse")
+                    .arg("--is-inside-work-tree"),
+            )
+            .is_success();
+        if in_working_tree {
+            let untracked_paths_output = build
+                .run(
+                    helpers::git(Some(&build.src))
+                        .capture_stdout()
+                        .arg("status")
+                        .arg("--porcelain")
+                        .arg("-z")
+                        .arg("--untracked-files=normal"),
+                )
+                .stdout();
             let untracked_paths: Vec<_> = untracked_paths_output
                 .split_terminator('\0')
                 .filter_map(

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

@@ -14,7 +14,6 @@ use std::ffi::{OsStr, OsString};
 use std::fs::{self, File};
 use std::io;
 use std::path::{Path, PathBuf};
-use std::process::Command;
 use std::sync::OnceLock;
 
 use crate::core::builder::{Builder, RunConfig, ShouldRun, Step};
@@ -25,6 +24,7 @@ use crate::utils::helpers::{
 };
 use crate::{generate_smart_stamp_hash, CLang, GitRepo, Kind};
 
+use crate::utils::exec::BootstrapCommand;
 use build_helper::ci::CiEnv;
 use build_helper::git::get_git_merge_base;
 
@@ -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).trim().to_owned()
+        output(&mut rev_list.command).trim().to_owned()
     } else if let Some(info) = channel::read_commit_info_file(&config.src) {
         info.sha.trim().to_owned()
     } else {
@@ -253,7 +253,8 @@ pub(crate) fn is_ci_llvm_modified(config: &Config) -> bool {
         // We assume we have access to git, so it's okay to unconditionally pass
         // `true` here.
         let llvm_sha = detect_llvm_sha(config, true);
-        let head_sha = output(helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD"));
+        let head_sha =
+            output(&mut helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD").command);
         let head_sha = head_sha.trim();
         llvm_sha == head_sha
     }
@@ -477,7 +478,9 @@ impl Step for Llvm {
             let LlvmResult { llvm_config, .. } =
                 builder.ensure(Llvm { target: builder.config.build });
             if !builder.config.dry_run() {
-                let llvm_bindir = output(Command::new(&llvm_config).arg("--bindir"));
+                let llvm_bindir = builder
+                    .run(BootstrapCommand::new(&llvm_config).capture_stdout().arg("--bindir"))
+                    .stdout();
                 let host_bin = Path::new(llvm_bindir.trim());
                 cfg.define(
                     "LLVM_TABLEGEN",
@@ -527,8 +530,8 @@ impl Step for Llvm {
 
         // Helper to find the name of LLVM's shared library on darwin and linux.
         let find_llvm_lib_name = |extension| {
-            let mut cmd = Command::new(&res.llvm_config);
-            let version = output(cmd.arg("--version"));
+            let cmd = BootstrapCommand::new(&res.llvm_config);
+            let version = builder.run(cmd.capture_stdout().arg("--version")).stdout();
             let major = version.split('.').next().unwrap();
 
             match &llvm_version_suffix {
@@ -584,8 +587,8 @@ fn check_llvm_version(builder: &Builder<'_>, llvm_config: &Path) {
         return;
     }
 
-    let mut cmd = Command::new(llvm_config);
-    let version = output(cmd.arg("--version"));
+    let cmd = BootstrapCommand::new(llvm_config);
+    let version = builder.run(cmd.capture_stdout().arg("--version")).stdout();
     let mut parts = version.split('.').take(2).filter_map(|s| s.parse::<u32>().ok());
     if let (Some(major), Some(_minor)) = (parts.next(), parts.next()) {
         if major >= 17 {

src/bootstrap/src/core/build_steps/run.rs

@@ -4,15 +4,14 @@
 //! If it can be reached from `./x.py run` it can go here.
 
 use std::path::PathBuf;
-use std::process::Command;
 
 use crate::core::build_steps::dist::distdir;
 use crate::core::build_steps::test;
 use crate::core::build_steps::tool::{self, SourceType, Tool};
 use crate::core::builder::{Builder, RunConfig, ShouldRun, Step};
 use crate::core::config::flags::get_completion;
 use crate::core::config::TargetSelection;
-use crate::utils::helpers::output;
+use crate::utils::exec::BootstrapCommand;
 use crate::Mode;
 
 #[derive(Debug, PartialOrd, Ord, Clone, Hash, PartialEq, Eq)]
@@ -41,7 +40,8 @@ impl Step for BuildManifest {
             panic!("\n\nfailed to specify `dist.upload-addr` in `config.toml`\n\n")
         });
 
-        let today = output(Command::new("date").arg("+%Y-%m-%d"));
+        let today =
+            builder.run(BootstrapCommand::new("date").capture_stdout().arg("+%Y-%m-%d")).stdout();
 
         cmd.arg(sign);
         cmd.arg(distdir(builder));
@@ -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());
     }
 }
 

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

@@ -484,6 +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
         .output()
         .map(|output| {
             assert!(output.status.success(), "failed to run `git`");

src/bootstrap/src/core/build_steps/suggest.rs

@@ -13,24 +13,15 @@ use crate::core::builder::Builder;
 pub fn suggest(builder: &Builder<'_>, run: bool) {
     let git_config = builder.config.git_config();
     let suggestions = builder
-        .tool_cmd(Tool::SuggestTests)
-        .env("SUGGEST_TESTS_GIT_REPOSITORY", git_config.git_repository)
-        .env("SUGGEST_TESTS_NIGHTLY_BRANCH", git_config.nightly_branch)
-        .command
-        .output()
-        .expect("failed to run `suggest-tests` tool");
+        .run(
+            builder
+                .tool_cmd(Tool::SuggestTests)
+                .capture_stdout()
+                .env("SUGGEST_TESTS_GIT_REPOSITORY", git_config.git_repository)
+                .env("SUGGEST_TESTS_NIGHTLY_BRANCH", git_config.nightly_branch),
+        )
+        .stdout();
 
-    if !suggestions.status.success() {
-        println!("failed to run `suggest-tests` tool ({})", suggestions.status);
-        println!(
-            "`suggest_tests` stdout:\n{}`suggest_tests` stderr:\n{}",
-            String::from_utf8(suggestions.stdout).unwrap(),
-            String::from_utf8(suggestions.stderr).unwrap()
-        );
-        panic!("failed to run `suggest-tests`");
-    }
-
-    let suggestions = String::from_utf8(suggestions.stdout).unwrap();
     let suggestions = suggestions
         .lines()
         .map(|line| {