Auto merge of #127799 - Kobzol:bootstrap-cmd-refactor-7, r=onur-ozkan

Bootstrap command refactoring: make command output API more bulletproof (step 7)

Continuation of https://github.com/rust-lang/rust/pull/127680.

This PR modifies the API of running commands to make it more explicit when a command is expected to produce programmatically handled output. Now if you call just `run`, you cannot access the stdout/stderr by accident, because it will not be returned to the caller.

This API change might be seen as overkill, let me know what do you think. In any case, I'd like to land the second commit, to make it harder to accidentally read stdout/stderr of commands that did not capture output (now you'd get an empty string as a result, but you should probably get a panic instead, if you try to read uncaptured stdout/stderr).

Tracking issue: https://github.com/rust-lang/rust/issues/126819

r? `@onur-ozkan`

try-job: x86_64-msvc

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

@@ -1481,7 +1481,7 @@ pub fn compiler_file(
     let mut cmd = command(compiler);
     cmd.args(builder.cflags(target, GitRepo::Rustc, c));
     cmd.arg(format!("-print-file-name={file}"));
-    let out = cmd.capture_stdout().run(builder).stdout();
+    let out = cmd.run_capture_stdout(builder).stdout();
     PathBuf::from(out.trim())
 }
 
@@ -1845,7 +1845,7 @@ impl Step for Assemble {
                 builder.ensure(llvm::Llvm { target: target_compiler.host });
             if !builder.config.dry_run() && builder.config.llvm_tools_enabled {
                 let llvm_bin_dir =
-                    command(llvm_config).capture_stdout().arg("--bindir").run(builder).stdout();
+                    command(llvm_config).arg("--bindir").run_capture_stdout(builder).stdout();
                 let llvm_bin_dir = Path::new(llvm_bin_dir.trim());
 
                 // Since we've already built the LLVM tools, install them to the sysroot.
@@ -2171,7 +2171,7 @@ pub fn strip_debug(builder: &Builder<'_>, target: TargetSelection, path: &Path)
     }
 
     let previous_mtime = t!(t!(path.metadata()).modified());
-    command("strip").capture().arg("--strip-debug").arg(path).run(builder);
+    command("strip").arg("--strip-debug").arg(path).run_capture(builder);
 
     let file = t!(fs::File::open(path));
 

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

@@ -182,7 +182,7 @@ fn make_win_dist(
     //Ask gcc where it keeps its stuff
     let mut cmd = command(builder.cc(target));
     cmd.arg("-print-search-dirs");
-    let gcc_out = cmd.capture_stdout().run(builder).stdout();
+    let gcc_out = cmd.run_capture_stdout(builder).stdout();
 
     let mut bin_path: Vec<_> = env::split_paths(&env::var_os("PATH").unwrap_or_default()).collect();
     let mut lib_path = Vec::new();
@@ -1067,7 +1067,7 @@ impl Step for PlainSourceTarball {
                 cmd.arg("--sync").arg(manifest_path);
             }
 
-            let config = cmd.capture().run(builder).stdout();
+            let config = cmd.run_capture(builder).stdout();
 
             let cargo_config_dir = plain_dst_src.join(".cargo");
             builder.create_dir(&cargo_config_dir);
@@ -2075,7 +2075,7 @@ fn maybe_install_llvm(
         let mut cmd = command(llvm_config);
         cmd.arg("--libfiles");
         builder.verbose(|| println!("running {cmd:?}"));
-        let files = cmd.capture_stdout().run(builder).stdout();
+        let files = cmd.run_capture_stdout(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/format.rs

@@ -60,7 +60,7 @@ fn get_rustfmt_version(build: &Builder<'_>) -> Option<(String, PathBuf)> {
     });
     cmd.arg("--version");
 
-    let output = cmd.capture().allow_failure().run(build);
+    let output = cmd.allow_failure().run_capture(build);
     if output.is_failure() {
         return None;
     }
@@ -160,25 +160,23 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
         }
     }
     let git_available =
-        helpers::git(None).capture().allow_failure().arg("--version").run(build).is_success();
+        helpers::git(None).allow_failure().arg("--version").run_capture(build).is_success();
 
     let mut adjective = None;
     if git_available {
         let in_working_tree = helpers::git(Some(&build.src))
-            .capture()
             .allow_failure()
             .arg("rev-parse")
             .arg("--is-inside-work-tree")
-            .run(build)
+            .run_capture(build)
             .is_success();
         if in_working_tree {
             let untracked_paths_output = helpers::git(Some(&build.src))
-                .capture_stdout()
                 .arg("status")
                 .arg("--porcelain")
                 .arg("-z")
                 .arg("--untracked-files=normal")
-                .run(build)
+                .run_capture_stdout(build)
                 .stdout();
             let untracked_paths: Vec<_> = untracked_paths_output
                 .split_terminator('\0')

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

@@ -471,7 +471,7 @@ impl Step for Llvm {
                 builder.ensure(Llvm { target: builder.config.build });
             if !builder.config.dry_run() {
                 let llvm_bindir =
-                    command(&llvm_config).capture_stdout().arg("--bindir").run(builder).stdout();
+                    command(&llvm_config).arg("--bindir").run_capture_stdout(builder).stdout();
                 let host_bin = Path::new(llvm_bindir.trim());
                 cfg.define(
                     "LLVM_TABLEGEN",
@@ -522,7 +522,7 @@ 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 version =
-                command(&res.llvm_config).capture_stdout().arg("--version").run(builder).stdout();
+                command(&res.llvm_config).arg("--version").run_capture_stdout(builder).stdout();
             let major = version.split('.').next().unwrap();
 
             match &llvm_version_suffix {
@@ -578,7 +578,7 @@ fn check_llvm_version(builder: &Builder<'_>, llvm_config: &Path) {
         return;
     }
 
-    let version = command(llvm_config).capture_stdout().arg("--version").run(builder).stdout();
+    let version = command(llvm_config).arg("--version").run_capture_stdout(builder).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

@@ -40,7 +40,7 @@ impl Step for BuildManifest {
             panic!("\n\nfailed to specify `dist.upload-addr` in `config.toml`\n\n")
         });
 
-        let today = command("date").capture_stdout().arg("+%Y-%m-%d").run(builder).stdout();
+        let today = command("date").arg("+%Y-%m-%d").run_capture_stdout(builder).stdout();
 
         cmd.arg(sign);
         cmd.arg(distdir(builder));

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

@@ -275,7 +275,7 @@ impl Step for Link {
 }
 
 fn rustup_installed(builder: &Builder<'_>) -> bool {
-    command("rustup").capture_stdout().arg("--version").run(builder).is_success()
+    command("rustup").arg("--version").run_capture_stdout(builder).is_success()
 }
 
 fn stage_dir_exists(stage_path: &str) -> bool {
@@ -313,10 +313,9 @@ fn attempt_toolchain_link(builder: &Builder<'_>, stage_path: &str) {
 
 fn toolchain_is_linked(builder: &Builder<'_>) -> bool {
     match command("rustup")
-        .capture_stdout()
         .allow_failure()
         .args(["toolchain", "list"])
-        .run(builder)
+        .run_capture_stdout(builder)
         .stdout_if_ok()
     {
         Some(toolchain_list) => {
@@ -341,9 +340,8 @@ fn toolchain_is_linked(builder: &Builder<'_>) -> bool {
 
 fn try_link_toolchain(builder: &Builder<'_>, stage_path: &str) -> bool {
     command("rustup")
-        .capture_stdout()
         .args(["toolchain", "link", "stage1", stage_path])
-        .run(builder)
+        .run_capture_stdout(builder)
         .is_success()
 }
 
@@ -481,9 +479,8 @@ impl Step for Hook {
 // install a git hook to automatically run tidy, if they want
 fn install_git_hook_maybe(builder: &Builder<'_>, config: &Config) -> io::Result<()> {
     let git = helpers::git(Some(&config.src))
-        .capture()
         .args(["rev-parse", "--git-common-dir"])
-        .run(builder)
+        .run_capture(builder)
         .stdout();
     let git = PathBuf::from(git.trim());
     let hooks_dir = git.join("hooks");

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

@@ -14,10 +14,9 @@ pub fn suggest(builder: &Builder<'_>, run: bool) {
     let git_config = builder.config.git_config();
     let suggestions = 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)
-        .run(builder)
+        .run_capture_stdout(builder)
         .stdout();
 
     let suggestions = suggestions

src/bootstrap/src/core/build_steps/synthetic_targets.rs

@@ -64,7 +64,7 @@ fn create_synthetic_target(
     // we cannot use nightly features. So `RUSTC_BOOTSTRAP` is needed here.
     cmd.env("RUSTC_BOOTSTRAP", "1");
 
-    let output = cmd.capture().run(builder).stdout();
+    let output = cmd.run_capture(builder).stdout();
     let mut spec: serde_json::Value = serde_json::from_slice(output.as_bytes()).unwrap();
     let spec_map = spec.as_object_mut().unwrap();
 

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

@@ -169,7 +169,7 @@ You can skip linkcheck with --skip src/tools/linkchecker"
 }
 
 fn check_if_tidy_is_installed(builder: &Builder<'_>) -> bool {
-    command("tidy").capture_stdout().allow_failure().arg("--version").run(builder).is_success()
+    command("tidy").allow_failure().arg("--version").run_capture_stdout(builder).is_success()
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -468,7 +468,7 @@ impl Miri {
         cargo.arg("--print-sysroot");
 
         builder.verbose(|| println!("running: {cargo:?}"));
-        let stdout = cargo.capture_stdout().run(builder).stdout();
+        let stdout = cargo.run_capture_stdout(builder).stdout();
         // Output is "<sysroot>\n".
         let sysroot = stdout.trim_end();
         builder.verbose(|| println!("`cargo miri setup --print-sysroot` said: {sysroot:?}"));
@@ -749,7 +749,7 @@ impl Step for Clippy {
         let _guard = builder.msg_sysroot_tool(Kind::Test, compiler.stage, "clippy", host, host);
 
         // Clippy reports errors if it blessed the outputs
-        if cargo.allow_failure().run(builder).is_success() {
+        if cargo.allow_failure().run(builder) {
             // The tests succeeded; nothing to do.
             return;
         }
@@ -904,12 +904,12 @@ fn get_browser_ui_test_version_inner(
     npm: &Path,
     global: bool,
 ) -> Option<String> {
-    let mut command = command(npm).capture();
+    let mut command = command(npm);
     command.arg("list").arg("--parseable").arg("--long").arg("--depth=0");
     if global {
         command.arg("--global");
     }
-    let lines = command.allow_failure().run(builder).stdout();
+    let lines = command.allow_failure().run_capture(builder).stdout();
     lines
         .lines()
         .find_map(|l| l.split(':').nth(1)?.strip_prefix("browser-ui-test@"))
@@ -1846,19 +1846,17 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
 
         let lldb_exe = builder.config.lldb.clone().unwrap_or_else(|| PathBuf::from("lldb"));
         let lldb_version = command(&lldb_exe)
-            .capture()
             .allow_failure()
             .arg("--version")
-            .run(builder)
+            .run_capture(builder)
             .stdout_if_ok()
             .and_then(|v| if v.trim().is_empty() { None } else { Some(v) });
         if let Some(ref vers) = lldb_version {
             cmd.arg("--lldb-version").arg(vers);
             let lldb_python_dir = command(&lldb_exe)
                 .allow_failure()
-                .capture_stdout()
                 .arg("-P")
-                .run(builder)
+                .run_capture_stdout(builder)
                 .stdout_if_ok()
                 .map(|p| p.lines().next().expect("lldb Python dir not found").to_string());
             if let Some(ref dir) = lldb_python_dir {
@@ -1917,10 +1915,9 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
                 builder.ensure(llvm::Llvm { target: builder.config.build });
             if !builder.config.dry_run() {
                 let llvm_version =
-                    builder.run(command(&llvm_config).capture_stdout().arg("--version")).stdout();
+                    command(&llvm_config).arg("--version").run_capture_stdout(builder).stdout();
-                let llvm_components = builder
+                let llvm_components =
-                    .run(command(&llvm_config).capture_stdout().arg("--components"))
+                    command(&llvm_config).arg("--components").run_capture_stdout(builder).stdout();
-                    .stdout();
                 // Remove trailing newline from llvm-config output.
                 cmd.arg("--llvm-version")
                     .arg(llvm_version.trim())
@@ -1940,7 +1937,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
             // platform-specific environment variable as a workaround.
             if !builder.config.dry_run() && suite.ends_with("fulldeps") {
                 let llvm_libdir =
-                    builder.run(command(&llvm_config).capture_stdout().arg("--libdir")).stdout();
+                    command(&llvm_config).arg("--libdir").run_capture_stdout(builder).stdout();
                 add_link_lib_path(vec![llvm_libdir.trim().into()], &mut cmd);
             }
 
@@ -2212,7 +2209,7 @@ impl BookTest {
             compiler.host,
         );
         let _time = helpers::timeit(builder);
-        let toolstate = if rustbook_cmd.delay_failure().run(builder).is_success() {
+        let toolstate = if rustbook_cmd.delay_failure().run(builder) {
             ToolState::TestPass
         } else {
             ToolState::TestFail
@@ -2345,7 +2342,7 @@ impl Step for ErrorIndex {
         let guard =
             builder.msg(Kind::Test, compiler.stage, "error-index", compiler.host, compiler.host);
         let _time = helpers::timeit(builder);
-        tool.capture().run(builder);
+        tool.run_capture(builder);
         drop(guard);
         // The tests themselves need to link to std, so make sure it is
         // available.
@@ -2376,9 +2373,10 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) ->
 
     cmd = cmd.delay_failure();
     if !builder.config.verbose_tests {
-        cmd = cmd.capture();
+        cmd.run_capture(builder).is_success()
+    } else {
+        cmd.run(builder)
     }
-    cmd.run(builder).is_success()
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -2404,11 +2402,8 @@ impl Step for RustcGuide {
         let src = builder.src.join(relative_path);
         let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook).delay_failure();
         rustbook_cmd.arg("linkcheck").arg(&src);
-        let toolstate = if rustbook_cmd.run(builder).is_success() {
+        let toolstate =
-            ToolState::TestPass
+            if rustbook_cmd.run(builder) { ToolState::TestPass } else { ToolState::TestFail };
-        } else {
-            ToolState::TestFail
-        };
         builder.save_toolstate("rustc-dev-guide", toolstate);
     }
 }
@@ -2920,7 +2915,7 @@ impl Step for RemoteCopyLibs {
             let f = t!(f);
             let name = f.file_name().into_string().unwrap();
             if helpers::is_dylib(&name) {
-                builder.run(command(&tool).arg("push").arg(f.path()));
+                command(&tool).arg("push").arg(f.path()).run(builder);
             }
         }
     }
@@ -2951,21 +2946,21 @@ impl Step for Distcheck {
         builder.ensure(dist::PlainSourceTarball);
         builder.ensure(dist::Src);
 
-        let mut cmd = command("tar");
+        command("tar")
-        cmd.arg("-xf")
+            .arg("-xf")
             .arg(builder.ensure(dist::PlainSourceTarball).tarball())
             .arg("--strip-components=1")
-            .current_dir(&dir);
+            .current_dir(&dir)
-        cmd.run(builder);
+            .run(builder);
-        builder.run(
+        command("./configure")
-            command("./configure")
+            .args(&builder.config.configure_args)
-                .args(&builder.config.configure_args)
+            .arg("--enable-vendor")
-                .arg("--enable-vendor")
+            .current_dir(&dir)
-                .current_dir(&dir),
+            .run(builder);
-        );
+        command(helpers::make(&builder.config.build.triple))
-        builder.run(
+            .arg("check")
-            command(helpers::make(&builder.config.build.triple)).arg("check").current_dir(&dir),
+            .current_dir(&dir)
-        );
+            .run(builder);
 
         // Now make sure that rust-src has all of libstd's dependencies
         builder.info("Distcheck rust-src");
@@ -2973,24 +2968,23 @@ impl Step for Distcheck {
         let _ = fs::remove_dir_all(&dir);
         t!(fs::create_dir_all(&dir));
 
-        let mut cmd = command("tar");
+        command("tar")
-        cmd.arg("-xf")
+            .arg("-xf")
             .arg(builder.ensure(dist::Src).tarball())
             .arg("--strip-components=1")
-            .current_dir(&dir);
+            .current_dir(&dir)
-        cmd.run(builder);
+            .run(builder);
 
         let toml = dir.join("rust-src/lib/rustlib/src/rust/library/std/Cargo.toml");
-        builder.run(
+        command(&builder.initial_cargo)
-            command(&builder.initial_cargo)
+            // Will read the libstd Cargo.toml
-                // Will read the libstd Cargo.toml
+            // which uses the unstable `public-dependency` feature.
-                // which uses the unstable `public-dependency` feature.
+            .env("RUSTC_BOOTSTRAP", "1")
-                .env("RUSTC_BOOTSTRAP", "1")
+            .arg("generate-lockfile")
-                .arg("generate-lockfile")
+            .arg("--manifest-path")
-                .arg("--manifest-path")
+            .arg(&toml)
-                .arg(&toml)
+            .current_dir(&dir)
-                .current_dir(&dir),
+            .run(builder);
-        );
     }
 }
 

src/bootstrap/src/core/build_steps/tool.rs

@@ -589,8 +589,7 @@ impl Step for Rustdoc {
                 .arg("--")
                 .arg(librustdoc_src)
                 .arg(rustdoc_src)
-                .run(builder)
+                .run(builder);
-                .is_success();
 
             if !has_changes {
                 let precompiled_rustdoc = builder
@@ -982,7 +981,7 @@ impl Step for LibcxxVersionTool {
             }
         }
 
-        let version_output = command(executable).capture_stdout().run(builder).stdout();
+        let version_output = command(executable).run_capture_stdout(builder).stdout();
 
         let version_str = version_output.split_once("version:").unwrap().1;
         let version = version_str.trim().parse::<usize>().unwrap();

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

@@ -102,12 +102,11 @@ fn print_error(tool: &str, submodule: &str) {
 fn check_changed_files(builder: &Builder<'_>, toolstates: &HashMap<Box<str>, ToolState>) {
     // Changed files
     let output = helpers::git(None)
-        .capture()
         .arg("diff")
         .arg("--name-status")
         .arg("HEAD")
         .arg("HEAD^")
-        .run(builder)
+        .run_capture(builder)
         .stdout();
 
     for (tool, submodule) in STABLE_TOOLS.iter().chain(NIGHTLY_TOOLS.iter()) {
@@ -391,7 +390,7 @@ fn commit_toolstate_change(builder: &Builder<'_>, current_toolstate: &ToolstateD
             .arg("-m")
             .arg(&message)
             .run(builder);
-        if !status.is_success() {
+        if !status {
             success = true;
             break;
         }
@@ -403,7 +402,7 @@ fn commit_toolstate_change(builder: &Builder<'_>, current_toolstate: &ToolstateD
             .arg("master")
             .run(builder);
         // If we successfully push, exit.
-        if status.is_success() {
+        if status {
             success = true;
             break;
         }
@@ -432,7 +431,7 @@ fn commit_toolstate_change(builder: &Builder<'_>, current_toolstate: &ToolstateD
 /// `publish_toolstate.py` script if the PR passes all tests and is merged to
 /// master.
 fn publish_test_results(builder: &Builder<'_>, current_toolstate: &ToolstateData) {
-    let commit = helpers::git(None).capture().arg("rev-parse").arg("HEAD").run(builder).stdout();
+    let commit = helpers::git(None).arg("rev-parse").arg("HEAD").run_capture(builder).stdout();
 
     let toolstate_serialized = t!(serde_json::to_string(&current_toolstate));
 

src/bootstrap/src/core/builder.rs

@@ -1954,7 +1954,7 @@ impl<'a> Builder<'a> {
         if mode == Mode::ToolRustc || mode == Mode::Codegen {
             if let Some(llvm_config) = self.llvm_config(target) {
                 let llvm_libdir =
-                    command(llvm_config).capture_stdout().arg("--libdir").run(self).stdout();
+                    command(llvm_config).arg("--libdir").run_capture_stdout(self).stdout();
                 add_link_lib_path(vec![llvm_libdir.trim().into()], &mut cargo);
             }
         }

src/bootstrap/src/core/metadata.rs

@@ -81,7 +81,7 @@ fn workspace_members(build: &Build) -> Vec<Package> {
             .arg("--no-deps")
             .arg("--manifest-path")
             .arg(build.src.join(manifest_path));
-        let metadata_output = cargo.capture_stdout().run_always().run(build).stdout();
+        let metadata_output = cargo.run_always().run_capture_stdout(build).stdout();
         let Output { packages, .. } = t!(serde_json::from_str(&metadata_output));
         packages
     };

src/bootstrap/src/core/sanity.rs

@@ -352,7 +352,7 @@ than building it.
             // There are three builds of cmake on windows: MSVC, MinGW, and
             // Cygwin. The Cygwin build does not have generators for Visual
             // Studio, so detect that here and error.
-            let out = command("cmake").capture_stdout().arg("--help").run(build).stdout();
+            let out = command("cmake").arg("--help").run_capture_stdout(build).stdout();
             if !out.contains("Visual Studio") {
                 panic!(
                     "

src/bootstrap/src/lib.rs

@@ -40,7 +40,7 @@ use crate::core::builder::{Builder, Kind};
 use crate::core::config::{flags, LldMode};
 use crate::core::config::{DryRun, Target};
 use crate::core::config::{LlvmLibunwind, TargetSelection};
-use crate::utils::exec::{command, BehaviorOnFailure, BootstrapCommand, CommandOutput};
+use crate::utils::exec::{command, BehaviorOnFailure, BootstrapCommand, CommandOutput, OutputMode};
 use crate::utils::helpers::{self, dir_is_empty, exe, libdir, mtime, output, symlink_dir};
 
 mod core;
@@ -496,21 +496,21 @@ impl Build {
         // Therefore, all commands below are marked with `run_always()`, so that they also run in
         // dry run mode.
         let submodule_git = || {
-            let mut cmd = helpers::git(Some(&absolute_path)).capture_stdout();
+            let mut cmd = helpers::git(Some(&absolute_path));
             cmd.run_always();
             cmd
         };
 
         // Determine commit checked out in submodule.
-        let checked_out_hash = submodule_git().args(["rev-parse", "HEAD"]).run(self).stdout();
+        let checked_out_hash =
+            submodule_git().args(["rev-parse", "HEAD"]).run_capture_stdout(self).stdout();
         let checked_out_hash = checked_out_hash.trim_end();
         // Determine commit that the submodule *should* have.
         let recorded = helpers::git(Some(&self.src))
-            .capture_stdout()
             .run_always()
             .args(["ls-tree", "HEAD"])
             .arg(relative_path)
-            .run(self)
+            .run_capture_stdout(self)
             .stdout();
         let actual_hash = recorded
             .split_whitespace()
@@ -534,11 +534,10 @@ impl Build {
             // Git is buggy and will try to fetch submodules from the tracking branch for *this* repository,
             // even though that has no relation to the upstream for the submodule.
             let current_branch = helpers::git(Some(&self.src))
-                .capture_stdout()
                 .allow_failure()
                 .run_always()
                 .args(["symbolic-ref", "--short", "HEAD"])
-                .run(self)
+                .run_capture_stdout(self)
                 .stdout_if_ok()
                 .map(|s| s.trim().to_owned());
 
@@ -557,17 +556,14 @@ impl Build {
             git.arg(relative_path);
             git
         };
-        if !update(true).run(self).is_success() {
+        if !update(true).run(self) {
             update(false).run(self);
         }
 
         // 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 = submodule_git()
+        let has_local_modifications =
-            .allow_failure()
+            !submodule_git().allow_failure().args(["diff-index", "--quiet", "HEAD"]).run(self);
-            .args(["diff-index", "--quiet", "HEAD"])
-            .run(self)
-            .is_failure();
         if has_local_modifications {
             submodule_git().args(["stash", "push"]).run(self);
         }
@@ -588,11 +584,10 @@ impl Build {
             return;
         }
         let output = helpers::git(Some(&self.src))
-            .capture()
             .args(["config", "--file"])
             .arg(self.config.src.join(".gitmodules"))
             .args(["--get-regexp", "path"])
-            .run(self)
+            .run_capture(self)
             .stdout();
         for line in output.lines() {
             // Look for `submodule.$name.path = $path`
@@ -870,14 +865,14 @@ impl Build {
         if let Some(s) = target_config.and_then(|c| c.llvm_filecheck.as_ref()) {
             s.to_path_buf()
         } else if let Some(s) = target_config.and_then(|c| c.llvm_config.as_ref()) {
-            let llvm_bindir = command(s).capture_stdout().arg("--bindir").run(self).stdout();
+            let llvm_bindir = command(s).arg("--bindir").run_capture_stdout(self).stdout();
             let filecheck = Path::new(llvm_bindir.trim()).join(exe("FileCheck", target));
             if filecheck.exists() {
                 filecheck
             } else {
                 // On Fedora the system LLVM installs FileCheck in the
                 // llvm subdirectory of the libdir.
-                let llvm_libdir = command(s).capture_stdout().arg("--libdir").run(self).stdout();
+                let llvm_libdir = command(s).arg("--libdir").run_capture_stdout(self).stdout();
                 let lib_filecheck =
                     Path::new(llvm_libdir.trim()).join("llvm").join(exe("FileCheck", target));
                 if lib_filecheck.exists() {
@@ -944,7 +939,12 @@ impl Build {
     /// Execute a command and return its output.
     /// This method should be used for all command executions in bootstrap.
     #[track_caller]
-    fn run(&self, command: &mut BootstrapCommand) -> CommandOutput {
+    fn run(
+        &self,
+        command: &mut BootstrapCommand,
+        stdout: OutputMode,
+        stderr: OutputMode,
+    ) -> CommandOutput {
         command.mark_as_executed();
         if self.config.dry_run() && !command.run_always {
             return CommandOutput::default();
@@ -957,19 +957,20 @@ impl Build {
             println!("running: {command:?} (created at {created_at}, executed at {executed_at})")
         });
 
-        let stdout = command.stdout.stdio();
+        let cmd = command.as_command_mut();
-        command.as_command_mut().stdout(stdout);
+        cmd.stdout(stdout.stdio());
-        let stderr = command.stderr.stdio();
+        cmd.stderr(stderr.stdio());
-        command.as_command_mut().stderr(stderr);
 
-        let output = command.as_command_mut().output();
+        let output = cmd.output();
 
         use std::fmt::Write;
 
         let mut message = String::new();
         let output: CommandOutput = match output {
             // Command has succeeded
-            Ok(output) if output.status.success() => output.into(),
+            Ok(output) if output.status.success() => {
+                CommandOutput::from_output(output, stdout, stderr)
+            }
             // Command has started, but then it failed
             Ok(output) => {
                 writeln!(
@@ -983,15 +984,15 @@ Executed at: {executed_at}"#,
                 )
                 .unwrap();
 
-                let output: CommandOutput = output.into();
+                let output: CommandOutput = CommandOutput::from_output(output, stdout, stderr);
 
                 // If the output mode is OutputMode::Capture, we can now print the output.
                 // If it is OutputMode::Print, then the output has already been printed to
                 // stdout/stderr, and we thus don't have anything captured to print anyway.
-                if command.stdout.captures() {
+                if stdout.captures() {
                     writeln!(message, "\nSTDOUT ----\n{}", output.stdout().trim()).unwrap();
                 }
-                if command.stderr.captures() {
+                if stderr.captures() {
                     writeln!(message, "\nSTDERR ----\n{}", output.stderr().trim()).unwrap();
                 }
                 output
@@ -1004,7 +1005,7 @@ Executed at: {executed_at}"#,
             \nIt was not possible to execute the command: {e:?}"
                 )
                 .unwrap();
-                CommandOutput::did_not_start()
+                CommandOutput::did_not_start(stdout, stderr)
             }
         };
         if !output.is_success() {
@@ -1529,7 +1530,6 @@ Executed at: {executed_at}"#,
             // That's our beta number!
             // (Note that we use a `..` range, not the `...` symmetric difference.)
             helpers::git(Some(&self.src))
-                .capture()
                 .arg("rev-list")
                 .arg("--count")
                 .arg("--merges")
@@ -1537,7 +1537,7 @@ Executed at: {executed_at}"#,
                     "refs/remotes/origin/{}..HEAD",
                     self.config.stage0_metadata.config.nightly_branch
                 ))
-                .run(self)
+                .run_capture(self)
                 .stdout()
         });
         let n = count.trim().parse().unwrap();
@@ -1972,21 +1972,19 @@ pub fn generate_smart_stamp_hash(
     additional_input: &str,
 ) -> String {
     let diff = helpers::git(Some(dir))
-        .capture_stdout()
         .allow_failure()
         .arg("diff")
-        .run(builder)
+        .run_capture_stdout(builder)
         .stdout_if_ok()
         .unwrap_or_default();
 
     let status = helpers::git(Some(dir))
-        .capture_stdout()
         .allow_failure()
         .arg("status")
         .arg("--porcelain")
         .arg("-z")
         .arg("--untracked-files=normal")
-        .run(builder)
+        .run_capture_stdout(builder)
         .stdout_if_ok()
         .unwrap_or_default();
 

src/bootstrap/src/utils/cc_detect.rs

@@ -182,15 +182,15 @@ fn default_compiler(
                 return None;
             }
 
-            let cmd = BootstrapCommand::from(c.to_command());
+            let mut cmd = BootstrapCommand::from(c.to_command());
-            let output = cmd.capture_stdout().arg("--version").run(build).stdout();
+            let output = cmd.arg("--version").run_capture_stdout(build).stdout();
             let i = output.find(" 4.")?;
             match output[i + 3..].chars().next().unwrap() {
                 '0'..='6' => {}
                 _ => return None,
             }
             let alternative = format!("e{gnu_compiler}");
-            if command(&alternative).capture().run(build).is_success() {
+            if command(&alternative).run_capture(build).is_success() {
                 Some(PathBuf::from(alternative))
             } else {
                 None

src/bootstrap/src/utils/exec.rs

@@ -50,7 +50,7 @@ impl 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::Print]).
-/// If you want to handle the output programmatically, use [BootstrapCommand::capture].
+/// If you want to handle the output programmatically, use [BootstrapCommand::run_capture].
 ///
 /// Bootstrap will print a debug log to stdout if the command fails and failure is not allowed.
 ///
@@ -59,8 +59,6 @@ impl OutputMode {
 pub struct BootstrapCommand {
     command: Command,
     pub failure_behavior: BehaviorOnFailure,
-    pub stdout: OutputMode,
-    pub stderr: OutputMode,
     // Run the command even during dry run
     pub run_always: bool,
     // This field makes sure that each command is executed (or disarmed) before it is dropped,
@@ -135,22 +133,23 @@ impl BootstrapCommand {
         self
     }
 
-    /// Capture all output of the command, do not print it.
+    /// Run the command, while printing stdout and stderr.
-    #[must_use]
+    /// Returns true if the command has succeeded.
-    pub fn capture(self) -> Self {
-        Self { stdout: OutputMode::Capture, stderr: OutputMode::Capture, ..self }
-    }
-
-    /// Capture stdout of the command, do not print it.
-    #[must_use]
-    pub fn capture_stdout(self) -> Self {
-        Self { stdout: OutputMode::Capture, ..self }
-    }
-
-    /// Run the command, returning its output.
     #[track_caller]
-    pub fn run(&mut self, builder: &Build) -> CommandOutput {
+    pub fn run(&mut self, builder: &Build) -> bool {
-        builder.run(self)
+        builder.run(self, OutputMode::Print, OutputMode::Print).is_success()
+    }
+
+    /// Run the command, while capturing and returning all its output.
+    #[track_caller]
+    pub fn run_capture(&mut self, builder: &Build) -> CommandOutput {
+        builder.run(self, OutputMode::Capture, OutputMode::Capture)
+    }
+
+    /// Run the command, while capturing and returning stdout, and printing stderr.
+    #[track_caller]
+    pub fn run_capture_stdout(&mut self, builder: &Build) -> CommandOutput {
+        builder.run(self, OutputMode::Capture, OutputMode::Print)
     }
 
     /// Provides access to the stdlib Command inside.
@@ -189,11 +188,7 @@ impl BootstrapCommand {
 impl Debug for BootstrapCommand {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         write!(f, "{:?}", self.command)?;
-        write!(
+        write!(f, " (failure_mode={:?})", self.failure_behavior)
-            f,
-            " (failure_mode={:?}, stdout_mode={:?}, stderr_mode={:?})",
-            self.failure_behavior, self.stdout, self.stderr
-        )
     }
 }
 
@@ -205,8 +200,6 @@ impl From<Command> for BootstrapCommand {
         Self {
             command,
             failure_behavior: BehaviorOnFailure::Exit,
-            stdout: OutputMode::Print,
-            stderr: OutputMode::Print,
             run_always: false,
             drop_bomb: DropBomb::arm(program),
         }
@@ -230,17 +223,41 @@ pub fn command<S: AsRef<OsStr>>(program: S) -> BootstrapCommand {
 }
 
 /// Represents the output of an executed process.
-#[allow(unused)]
 pub struct CommandOutput {
     status: CommandStatus,
-    stdout: Vec<u8>,
+    stdout: Option<Vec<u8>>,
-    stderr: Vec<u8>,
+    stderr: Option<Vec<u8>>,
 }
 
 impl CommandOutput {
     #[must_use]
-    pub fn did_not_start() -> Self {
+    pub fn did_not_start(stdout: OutputMode, stderr: OutputMode) -> Self {
-        Self { status: CommandStatus::DidNotStart, stdout: vec![], stderr: vec![] }
+        Self {
+            status: CommandStatus::DidNotStart,
+            stdout: match stdout {
+                OutputMode::Print => None,
+                OutputMode::Capture => Some(vec![]),
+            },
+            stderr: match stderr {
+                OutputMode::Print => None,
+                OutputMode::Capture => Some(vec![]),
+            },
+        }
+    }
+
+    #[must_use]
+    pub fn from_output(output: Output, stdout: OutputMode, stderr: OutputMode) -> Self {
+        Self {
+            status: CommandStatus::Finished(output.status),
+            stdout: match stdout {
+                OutputMode::Print => None,
+                OutputMode::Capture => Some(output.stdout),
+            },
+            stderr: match stderr {
+                OutputMode::Print => None,
+                OutputMode::Capture => Some(output.stderr),
+            },
+        }
     }
 
     #[must_use]
@@ -266,7 +283,10 @@ impl CommandOutput {
 
     #[must_use]
     pub fn stdout(&self) -> String {
-        String::from_utf8(self.stdout.clone()).expect("Cannot parse process stdout as UTF-8")
+        String::from_utf8(
+            self.stdout.clone().expect("Accessing stdout of a command that did not capture stdout"),
+        )
+        .expect("Cannot parse process stdout as UTF-8")
     }
 
     #[must_use]
@@ -276,7 +296,10 @@ impl CommandOutput {
 
     #[must_use]
     pub fn stderr(&self) -> String {
-        String::from_utf8(self.stderr.clone()).expect("Cannot parse process stderr as UTF-8")
+        String::from_utf8(
+            self.stderr.clone().expect("Accessing stderr of a command that did not capture stderr"),
+        )
+        .expect("Cannot parse process stderr as UTF-8")
     }
 }
 
@@ -284,18 +307,8 @@ impl Default for CommandOutput {
     fn default() -> Self {
         Self {
             status: CommandStatus::Finished(ExitStatus::default()),
-            stdout: vec![],
+            stdout: Some(vec![]),
-            stderr: vec![],
+            stderr: Some(vec![]),
-        }
-    }
-}
-
-impl From<Output> for CommandOutput {
-    fn from(output: Output) -> Self {
-        Self {
-            status: CommandStatus::Finished(output.status),
-            stdout: output.stdout,
-            stderr: output.stderr,
         }
     }
 }

src/bootstrap/src/utils/helpers.rs

@@ -347,7 +347,7 @@ pub fn get_clang_cl_resource_dir(builder: &Builder<'_>, clang_cl_path: &str) ->
     let mut builtins_locator = command(clang_cl_path);
     builtins_locator.args(["/clang:-print-libgcc-file-name", "/clang:--rtlib=compiler-rt"]);
 
-    let clang_rt_builtins = builtins_locator.capture_stdout().run(builder).stdout();
+    let clang_rt_builtins = builtins_locator.run_capture_stdout(builder).stdout();
     let clang_rt_builtins = Path::new(clang_rt_builtins.trim());
     assert!(
         clang_rt_builtins.exists(),
@@ -372,9 +372,9 @@ fn lld_flag_no_threads(builder: &Builder<'_>, lld_mode: LldMode, is_windows: boo
     let (windows_flag, other_flag) = LLD_NO_THREADS.get_or_init(|| {
         let newer_version = match lld_mode {
             LldMode::External => {
-                let mut cmd = command("lld").capture_stdout();
+                let mut cmd = command("lld");
                 cmd.arg("-flavor").arg("ld").arg("--version");
-                let out = cmd.run(builder).stdout();
+                let out = cmd.run_capture_stdout(builder).stdout();
                 match (out.find(char::is_numeric), out.find('.')) {
                     (Some(b), Some(e)) => out.as_str()[b..e].parse::<i32>().ok().unwrap_or(14) > 10,
                     _ => true,
@@ -534,7 +534,7 @@ pub fn get_closest_merge_base_commit(
     author: &str,
     target_paths: &[PathBuf],
 ) -> Result<String, String> {
-    let mut git = git(source_dir).capture_stdout();
+    let mut git = git(source_dir);
 
     let merge_base = get_git_merge_base(config, source_dir).unwrap_or_else(|_| "HEAD".into());
 

src/bootstrap/src/utils/tarball.rs

@@ -370,11 +370,10 @@ impl<'a> Tarball<'a> {
         if self.builder.rust_info().is_managed_git_subrepository() {
             // %ct means committer date
             let timestamp = helpers::git(Some(&self.builder.src))
-                .capture_stdout()
                 .arg("log")
                 .arg("-1")
                 .arg("--format=%ct")
-                .run(self.builder)
+                .run_capture_stdout(self.builder)
                 .stdout();
             cmd.args(["--override-file-mtime", timestamp.trim()]);
         }