Auto merge of rust-lang#127799 - Kobzol:bootstrap-cmd-refactor-7, r=<…

…try>

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

Continuation of rust-lang#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: rust-lang#126819

r? `@onur-ozkan`

try-job: x86_64-msvc

src/bootstrap/src/bin/main.rs

@@ -18,8 +18,14 @@ use bootstrap::{
     find_recent_config_change_ids, human_readable_changes, t, Build, Config, Subcommand,
     CONFIG_CHANGE_HISTORY,
 };
+use build_helper::ci::CiEnv;
 
 fn main() {
+    // Enable verbose panics on CI
+    if CiEnv::is_ci() {
+        std::env::set_var("RUST_BACKTRACE", "1");
+    }
+
     let args = env::args().skip(1).collect::<Vec<_>>();
     let config = Config::parse(&args);
 

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();