From 63d7992353b7250e150a377d1ca499e869de55f7 Mon Sep 17 00:00:00 2001 From: jyn Date: Thu, 13 Jul 2023 02:12:57 -0500 Subject: [PATCH 1/3] 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. --- src/bootstrap/config.rs | 12 +++++++++++ src/bootstrap/download.rs | 33 +++++++++++++---------------- src/bootstrap/lib.rs | 18 +++++++++++++++- src/bootstrap/run.rs | 16 +------------- src/bootstrap/test.rs | 44 ++++++++++++++------------------------- src/bootstrap/tool.rs | 3 ++- 6 files changed, 63 insertions(+), 63 deletions(-) diff --git a/src/bootstrap/config.rs b/src/bootstrap/config.rs index 348537433231f..ba78a1869e41f 100644 --- a/src/bootstrap/config.rs +++ b/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. diff --git a/src/bootstrap/download.rs b/src/bootstrap/download.rs index 8ee5ed83529a5..4f7dd6370e751 100644 --- a/src/bootstrap/download.rs +++ b/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 = 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;", diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index 32b6697356777..7c53d1a92f7eb 100644 --- a/src/bootstrap/lib.rs +++ b/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, @@ -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 } diff --git a/src/bootstrap/run.rs b/src/bootstrap/run.rs index 70b9170004339..128916b03c67c 100644 --- a/src/bootstrap/run.rs +++ b/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; diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 9ceb2714262ea..eaf3f3460aca1 100644 --- a/src/bootstrap/test.rs +++ b/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<'_> { diff --git a/src/bootstrap/tool.rs b/src/bootstrap/tool.rs index 8b3e8ca9b908e..5de212ab763d1 100644 --- a/src/bootstrap/tool.rs +++ b/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, From 78f51a4be00381b0a6dd578247107962dae7f3c7 Mon Sep 17 00:00:00 2001 From: jyn Date: Thu, 13 Jul 2023 02:19:39 -0500 Subject: [PATCH 2/3] Rename `Builder::try_run` to `run_delaying_failure` --- src/bootstrap/lib.rs | 2 +- src/bootstrap/run.rs | 2 +- src/bootstrap/test.rs | 28 ++++++++++++++-------------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index 7c53d1a92f7eb..e64cbc0152bb0 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -981,7 +981,7 @@ impl Build { } /// 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 { + pub(crate) fn run_delaying_failure(&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() { diff --git a/src/bootstrap/run.rs b/src/bootstrap/run.rs index 128916b03c67c..4082f5bb9b1ee 100644 --- a/src/bootstrap/run.rs +++ b/src/bootstrap/run.rs @@ -24,7 +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"); - builder.try_run( + builder.run_delaying_failure( &mut builder.tool_cmd(Tool::ExpandYamlAnchors).arg("generate").arg(&builder.src), ); } diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index eaf3f3460aca1..3852c92d7284d 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -180,9 +180,7 @@ 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); - builder.try_run( - linkchecker.arg(builder.out.join(host.triple).join("doc")), - ); + builder.run_delaying_failure(linkchecker.arg(builder.out.join(host.triple).join("doc"))); } fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { @@ -235,7 +233,9 @@ impl Step for HtmlCheck { builder.default_doc(&[]); builder.ensure(crate::doc::Rustc::new(builder.top_stage, self.target, builder)); - builder.try_run(builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target))); + builder.run_delaying_failure( + builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target)), + ); } } @@ -274,7 +274,7 @@ impl Step for Cargotest { let _time = util::timeit(&builder); let mut cmd = builder.tool_cmd(Tool::CargoTest); - builder.try_run( + builder.run_delaying_failure( cmd.arg(&cargo) .arg(&out_dir) .args(builder.config.test_args()) @@ -876,7 +876,7 @@ impl Step for RustdocTheme { util::lld_flag_no_threads(self.compiler.host.contains("windows")), ); } - builder.try_run(&mut cmd); + builder.run_delaying_failure(&mut cmd); } } @@ -1136,7 +1136,7 @@ help: to skip test's attempt to check tidiness, pass `--exclude src/tools/tidy` } builder.info("tidy check"); - builder.try_run(&mut cmd); + builder.run_delaying_failure(&mut cmd); builder.ensure(ExpandYamlAnchors); @@ -1181,7 +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"); - builder.try_run( + builder.run_delaying_failure( &mut builder.tool_cmd(Tool::ExpandYamlAnchors).arg("check").arg(&builder.src), ); } @@ -1970,7 +1970,7 @@ impl BookTest { compiler.host, ); let _time = util::timeit(&builder); - let toolstate = if builder.try_run(&mut rustbook_cmd) { + let toolstate = if builder.run_delaying_failure(&mut rustbook_cmd) { ToolState::TestPass } else { ToolState::TestFail @@ -2132,7 +2132,7 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) -> cmd.arg("--test-args").arg(test_args); if builder.config.verbose_tests { - builder.try_run(&mut cmd) + builder.run_delaying_failure(&mut cmd) } else { try_run_quiet(builder, &mut cmd) } @@ -2160,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 builder.try_run(rustbook_cmd.arg("linkcheck").arg(&src)) { + let toolstate = if builder.run_delaying_failure(rustbook_cmd.arg("linkcheck").arg(&src)) { ToolState::TestPass } else { ToolState::TestFail @@ -2713,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. - builder.try_run(&mut check_bootstrap); + builder.run_delaying_failure(&mut check_bootstrap); let mut cmd = Command::new(&builder.initial_cargo); cmd.arg("test") @@ -2789,7 +2789,7 @@ impl Step for TierCheck { self.compiler.host, self.compiler.host, ); - builder.try_run(&mut cargo.into()); + builder.run_delaying_failure(&mut cargo.into()); } } @@ -2875,7 +2875,7 @@ impl Step for RustInstaller { cmd.env("CARGO", &builder.initial_cargo); cmd.env("RUSTC", &builder.initial_rustc); cmd.env("TMP_DIR", &tmpdir); - builder.try_run(&mut cmd); + builder.run_delaying_failure(&mut cmd); } fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { From c0c6a24f89119848c27eb5267a6cf25dd468c1cb Mon Sep 17 00:00:00 2001 From: jyn Date: Thu, 13 Jul 2023 02:23:13 -0500 Subject: [PATCH 3/3] Replace `builder::try_run_quiet` with `run_quiet_delaying_failure` It was only used when a `builder` is available, and I want to encourage using the version that supports `--no-fail-fast`. --- src/bootstrap/lib.rs | 15 ++++++++++++--- src/bootstrap/test.rs | 15 +-------------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index e64cbc0152bb0..518567cc587d9 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -972,12 +972,21 @@ impl Build { /// 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()`. - fn try_run_quiet(&self, cmd: &mut Command) -> bool { + fn run_quiet_delaying_failure(&self, cmd: &mut Command) -> bool { if self.config.dry_run() { return true; } - self.verbose(&format!("running: {:?}", cmd)); - try_run_suppressed(cmd) + if !self.fail_fast { + self.verbose(&format!("running: {:?}", cmd)); + if !try_run_suppressed(cmd) { + let mut failures = self.delayed_failures.borrow_mut(); + failures.push(format!("{:?}", cmd)); + return false; + } + } else { + self.run_quiet(cmd); + } + true } /// Runs a command, printing out contextual info if it fails, and delaying errors until the build finishes. diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 3852c92d7284d..f63f75aca5bb3 100644 --- a/src/bootstrap/test.rs +++ b/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_quiet(builder: &Builder<'_>, cmd: &mut Command) -> bool { - if !builder.fail_fast { - if !builder.try_run_quiet(cmd) { - let mut failures = builder.delayed_failures.borrow_mut(); - failures.push(format!("{:?}", cmd)); - return false; - } - } else { - builder.run_quiet(cmd); - } - true -} - #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct CrateBootstrap { path: Interned, @@ -2134,7 +2121,7 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) -> if builder.config.verbose_tests { builder.run_delaying_failure(&mut cmd) } else { - try_run_quiet(builder, &mut cmd) + builder.run_quiet_delaying_failure(&mut cmd) } }