Deduplicate Builder::try_run and mark Config::try_run as deprecated

This does three things:
1. Remove `forward!(Build, fn try_run())`. Having `try_run` behave differently as a free function than an associated function is confusing, and `Builder::try_run` is a very desirable name.
2. Move `test::try_run` and `run::try_run` to `Builder::try_run`. These functions are different than `Config::try_run` - they delay the failure and print it out at the end of the build.
3. Mark `Config::try_run` as deprecated to encourage people to use `Builder::try_run` instead.

Author: jyn514

src/bootstrap/config.rs

@@ -1742,6 +1742,18 @@ impl Config {
         }
     }
 
+    /// 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()`.
+    #[deprecated = "use `Builder::try_run` instead where possible"]
+    pub(crate) fn try_run(&self, cmd: &mut Command) -> Result<(), ()> {
+        if self.dry_run() {
+            return Ok(());
+        }
+        self.verbose(&format!("running: {:?}", cmd));
+        build_helper::util::try_run(cmd, self.is_verbose())
+    }
+
     /// A git invocation which runs inside the source directory.
     ///
     /// Use this rather than `Command::new("git")` in order to support out-of-tree builds.

src/bootstrap/download.rs

@@ -7,7 +7,7 @@ use std::{
     process::{Command, Stdio},
 };
 
-use build_helper::{ci::CiEnv, util::try_run};
+use build_helper::ci::CiEnv;
 use once_cell::sync::OnceCell;
 use xz2::bufread::XzDecoder;
 
@@ -21,6 +21,12 @@ use crate::{
 
 static SHOULD_FIX_BINS_AND_DYLIBS: OnceCell<bool> = OnceCell::new();
 
+/// `Config::try_run` wrapper for this module to avoid warnings on `try_run`, since we don't have access to a `builder` yet.
+fn try_run(config: &Config, cmd: &mut Command) -> Result<(), ()> {
+    #[allow(deprecated)]
+    config.try_run(cmd)
+}
+
 /// Generic helpers that are useful anywhere in bootstrap.
 impl Config {
     pub fn is_verbose(&self) -> bool {
@@ -51,17 +57,6 @@ impl Config {
         tmp
     }
 
-    /// 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()`.
-    pub(crate) fn try_run(&self, cmd: &mut Command) -> Result<(), ()> {
-        if self.dry_run() {
-            return Ok(());
-        }
-        self.verbose(&format!("running: {:?}", cmd));
-        try_run(cmd, self.is_verbose())
-    }
-
     /// Runs a command, printing out nice contextual information if it fails.
     /// Returns false if do not execute at all, otherwise returns its
     /// `status.success()`.
@@ -156,14 +151,16 @@ impl Config {
                 ];
             }
             ";
-            nix_build_succeeded = self
-                .try_run(Command::new("nix-build").args(&[
+            nix_build_succeeded = try_run(
+                self,
+                Command::new("nix-build").args(&[
                     Path::new("-E"),
                     Path::new(NIX_EXPR),
                     Path::new("-o"),
                     &nix_deps_dir,
-                ]))
-                .is_ok();
+                ]),
+            )
+            .is_ok();
             nix_deps_dir
         });
         if !nix_build_succeeded {
@@ -188,7 +185,7 @@ impl Config {
             patchelf.args(&["--set-interpreter", dynamic_linker.trim_end()]);
         }
 
-        let _ = self.try_run(patchelf.arg(fname));
+        let _ = try_run(self, patchelf.arg(fname));
     }
 
     fn download_file(&self, url: &str, dest_path: &Path, help_on_error: &str) {
@@ -236,7 +233,7 @@ impl Config {
             if self.build.contains("windows-msvc") {
                 eprintln!("Fallback to PowerShell");
                 for _ in 0..3 {
-                    if self.try_run(Command::new("PowerShell.exe").args(&[
+                    if try_run(self, Command::new("PowerShell.exe").args(&[
                         "/nologo",
                         "-Command",
                         "[Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12;",

src/bootstrap/lib.rs

@@ -333,7 +333,6 @@ forward! {
     create(path: &Path, s: &str),
     remove(f: &Path),
     tempdir() -> PathBuf,
-    try_run(cmd: &mut Command) -> Result<(), ()>,
     llvm_link_shared() -> bool,
     download_rustc() -> bool,
     initial_rustfmt() -> Option<PathBuf>,
@@ -617,7 +616,9 @@ impl Build {
         }
 
         // Save any local changes, but avoid running `git stash pop` if there are none (since it will exit with an error).
+        #[allow(deprecated)] // diff-index reports the modifications through the exit status
         let has_local_modifications = self
+            .config
             .try_run(
                 Command::new("git")
                     .args(&["diff-index", "--quiet", "HEAD"])
@@ -979,6 +980,21 @@ impl Build {
         try_run_suppressed(cmd)
     }
 
+    /// Runs a command, printing out contextual info if it fails, and delaying errors until the build finishes.
+    pub(crate) fn try_run(&self, cmd: &mut Command) -> bool {
+        if !self.fail_fast {
+            #[allow(deprecated)] // can't use Build::try_run, that's us
+            if self.config.try_run(cmd).is_err() {
+                let mut failures = self.delayed_failures.borrow_mut();
+                failures.push(format!("{:?}", cmd));
+                return false;
+            }
+        } else {
+            self.run(cmd);
+        }
+        true
+    }
+
     pub fn is_verbose_than(&self, level: usize) -> bool {
         self.verbosity > level
     }

src/bootstrap/run.rs

@@ -24,8 +24,7 @@ impl Step for ExpandYamlAnchors {
     /// anchors in them, since GitHub Actions doesn't support them.
     fn run(self, builder: &Builder<'_>) {
         builder.info("Expanding YAML anchors in the GitHub Actions configuration");
-        try_run(
-            builder,
+        builder.try_run(
             &mut builder.tool_cmd(Tool::ExpandYamlAnchors).arg("generate").arg(&builder.src),
         );
     }
@@ -39,19 +38,6 @@ impl Step for ExpandYamlAnchors {
     }
 }
 
-fn try_run(builder: &Builder<'_>, cmd: &mut Command) -> bool {
-    if !builder.fail_fast {
-        if builder.try_run(cmd).is_err() {
-            let mut failures = builder.delayed_failures.borrow_mut();
-            failures.push(format!("{:?}", cmd));
-            return false;
-        }
-    } else {
-        builder.run(cmd);
-    }
-    true
-}
-
 #[derive(Debug, PartialOrd, Ord, Copy, Clone, Hash, PartialEq, Eq)]
 pub struct BuildManifest;
 

src/bootstrap/test.rs

@@ -48,19 +48,6 @@ const MIR_OPT_BLESS_TARGET_MAPPING: &[(&str, &str)] = &[
     // build for, so there is no entry for "aarch64-apple-darwin" here.
 ];
 
-fn try_run(builder: &Builder<'_>, cmd: &mut Command) -> bool {
-    if !builder.fail_fast {
-        if builder.try_run(cmd).is_err() {
-            let mut failures = builder.delayed_failures.borrow_mut();
-            failures.push(format!("{:?}", cmd));
-            return false;
-        }
-    } else {
-        builder.run(cmd);
-    }
-    true
-}
-
 fn try_run_quiet(builder: &Builder<'_>, cmd: &mut Command) -> bool {
     if !builder.fail_fast {
         if !builder.try_run_quiet(cmd) {
@@ -193,7 +180,9 @@ You can skip linkcheck with --exclude src/tools/linkchecker"
         let _guard =
             builder.msg(Kind::Test, compiler.stage, "Linkcheck", bootstrap_host, bootstrap_host);
         let _time = util::timeit(&builder);
-        try_run(builder, linkchecker.arg(builder.out.join(host.triple).join("doc")));
+        builder.try_run(
+            linkchecker.arg(builder.out.join(host.triple).join("doc")),
+        );
     }
 
     fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
@@ -246,7 +235,7 @@ impl Step for HtmlCheck {
         builder.default_doc(&[]);
         builder.ensure(crate::doc::Rustc::new(builder.top_stage, self.target, builder));
 
-        try_run(builder, builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target)));
+        builder.try_run(builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target)));
     }
 }
 
@@ -285,8 +274,7 @@ impl Step for Cargotest {
 
         let _time = util::timeit(&builder);
         let mut cmd = builder.tool_cmd(Tool::CargoTest);
-        try_run(
-            builder,
+        builder.try_run(
             cmd.arg(&cargo)
                 .arg(&out_dir)
                 .args(builder.config.test_args())
@@ -827,7 +815,8 @@ impl Step for Clippy {
 
         let _guard = builder.msg_sysroot_tool(Kind::Test, compiler.stage, "clippy", host, host);
 
-        if builder.try_run(&mut cargo).is_ok() {
+        #[allow(deprecated)] // Clippy reports errors if it blessed the outputs
+        if builder.config.try_run(&mut cargo).is_ok() {
             // The tests succeeded; nothing to do.
             return;
         }
@@ -887,7 +876,7 @@ impl Step for RustdocTheme {
                 util::lld_flag_no_threads(self.compiler.host.contains("windows")),
             );
         }
-        try_run(builder, &mut cmd);
+        builder.try_run(&mut cmd);
     }
 }
 
@@ -1147,7 +1136,7 @@ help: to skip test's attempt to check tidiness, pass `--exclude src/tools/tidy`
         }
 
         builder.info("tidy check");
-        try_run(builder, &mut cmd);
+        builder.try_run(&mut cmd);
 
         builder.ensure(ExpandYamlAnchors);
 
@@ -1192,8 +1181,7 @@ impl Step for ExpandYamlAnchors {
     /// by the user before committing CI changes.
     fn run(self, builder: &Builder<'_>) {
         builder.info("Ensuring the YAML anchors in the GitHub Actions config were expanded");
-        try_run(
-            builder,
+        builder.try_run(
             &mut builder.tool_cmd(Tool::ExpandYamlAnchors).arg("check").arg(&builder.src),
         );
     }
@@ -1982,7 +1970,7 @@ impl BookTest {
             compiler.host,
         );
         let _time = util::timeit(&builder);
-        let toolstate = if try_run(builder, &mut rustbook_cmd) {
+        let toolstate = if builder.try_run(&mut rustbook_cmd) {
             ToolState::TestPass
         } else {
             ToolState::TestFail
@@ -2144,7 +2132,7 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) ->
     cmd.arg("--test-args").arg(test_args);
 
     if builder.config.verbose_tests {
-        try_run(builder, &mut cmd)
+        builder.try_run(&mut cmd)
     } else {
         try_run_quiet(builder, &mut cmd)
     }
@@ -2172,7 +2160,7 @@ impl Step for RustcGuide {
 
         let src = builder.src.join(relative_path);
         let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook);
-        let toolstate = if try_run(builder, rustbook_cmd.arg("linkcheck").arg(&src)) {
+        let toolstate = if builder.try_run(rustbook_cmd.arg("linkcheck").arg(&src)) {
             ToolState::TestPass
         } else {
             ToolState::TestFail
@@ -2725,7 +2713,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.
-        try_run(builder, &mut check_bootstrap);
+        builder.try_run(&mut check_bootstrap);
 
         let mut cmd = Command::new(&builder.initial_cargo);
         cmd.arg("test")
@@ -2801,7 +2789,7 @@ impl Step for TierCheck {
             self.compiler.host,
             self.compiler.host,
         );
-        try_run(builder, &mut cargo.into());
+        builder.try_run(&mut cargo.into());
     }
 }
 
@@ -2887,7 +2875,7 @@ impl Step for RustInstaller {
         cmd.env("CARGO", &builder.initial_cargo);
         cmd.env("RUSTC", &builder.initial_rustc);
         cmd.env("TMP_DIR", &tmpdir);
-        try_run(builder, &mut cmd);
+        builder.try_run(&mut cmd);
     }
 
     fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {

src/bootstrap/tool.rs

@@ -108,7 +108,8 @@ impl Step for ToolBuild {
         );
 
         let mut cargo = Command::from(cargo);
-        let is_expected = builder.try_run(&mut cargo).is_ok();
+        #[allow(deprecated)] // we check this in `is_optional_tool` in a second
+        let is_expected = builder.config.try_run(&mut cargo).is_ok();
 
         builder.save_toolstate(
             tool,