From 7f1d08e575a6578c9a591f9a78ab13deb33704f8 Mon Sep 17 00:00:00 2001 From: clubby789 Date: Mon, 11 Mar 2024 23:49:15 +0000 Subject: [PATCH] bootstrap: Don't eagerly format verbose messages --- src/bootstrap/src/core/build_steps/compile.rs | 7 ++--- src/bootstrap/src/core/build_steps/dist.rs | 2 +- src/bootstrap/src/core/build_steps/test.rs | 6 ++--- src/bootstrap/src/core/builder.rs | 25 +++++++++--------- src/bootstrap/src/core/config/config.rs | 7 ++--- src/bootstrap/src/core/download.rs | 26 +++++++++++-------- src/bootstrap/src/lib.rs | 25 +++++++++--------- src/bootstrap/src/utils/cc_detect.rs | 10 +++---- src/bootstrap/src/utils/render_tests.rs | 2 +- src/bootstrap/src/utils/tarball.rs | 4 ++- 10 files changed, 62 insertions(+), 52 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index 242fe3c12b998..94ea2a01a4057 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -1536,7 +1536,8 @@ impl Step for Sysroot { }; let sysroot = sysroot_dir(compiler.stage); - builder.verbose(&format!("Removing sysroot {} to avoid caching bugs", sysroot.display())); + builder + .verbose(|| println!("Removing sysroot {} to avoid caching bugs", sysroot.display())); let _ = fs::remove_dir_all(&sysroot); t!(fs::create_dir_all(&sysroot)); @@ -1606,7 +1607,7 @@ impl Step for Sysroot { return true; } if !filtered_files.iter().all(|f| f != path.file_name().unwrap()) { - builder.verbose_than(1, &format!("ignoring {}", path.display())); + builder.verbose_than(1, || println!("ignoring {}", path.display())); false } else { true @@ -2085,7 +2086,7 @@ pub fn stream_cargo( cargo.arg(arg); } - builder.verbose(&format!("running: {cargo:?}")); + builder.verbose(|| println!("running: {cargo:?}")); if builder.config.dry_run() { return true; diff --git a/src/bootstrap/src/core/build_steps/dist.rs b/src/bootstrap/src/core/build_steps/dist.rs index 613c58252d3bd..3efdfc324b86c 100644 --- a/src/bootstrap/src/core/build_steps/dist.rs +++ b/src/bootstrap/src/core/build_steps/dist.rs @@ -2107,7 +2107,7 @@ fn maybe_install_llvm( { let mut cmd = Command::new(llvm_config); cmd.arg("--libfiles"); - builder.verbose(&format!("running {cmd:?}")); + builder.verbose(|| println!("running {cmd:?}")); let files = if builder.config.dry_run() { "".into() } else { output(&mut cmd) }; let build_llvm_out = &builder.llvm_out(builder.config.build); let target_llvm_out = &builder.llvm_out(target); diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 248d831b6e30e..2c63cde093a95 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -551,7 +551,7 @@ impl Miri { if builder.config.dry_run() { String::new() } else { - builder.verbose(&format!("running: {cargo:?}")); + builder.verbose(|| println!("running: {cargo:?}")); let out = cargo.output().expect("We already ran `cargo miri setup` before and that worked"); assert!(out.status.success(), "`cargo miri setup` returned with non-0 exit code"); @@ -559,7 +559,7 @@ impl Miri { let stdout = String::from_utf8(out.stdout) .expect("`cargo miri setup` stdout is not valid UTF-8"); let sysroot = stdout.trim_end(); - builder.verbose(&format!("`cargo miri setup --print-sysroot` said: {sysroot:?}")); + builder.verbose(|| println!("`cargo miri setup --print-sysroot` said: {sysroot:?}")); sysroot.to_owned() } } @@ -2326,7 +2326,7 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) -> } } - builder.verbose(&format!("doc tests for: {}", markdown.display())); + builder.verbose(|| println!("doc tests for: {}", markdown.display())); let mut cmd = builder.rustdoc_cmd(compiler); builder.add_rust_test_threads(&mut cmd); // allow for unstable options such as new editions diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs index 5e5d6d024ee82..db1f0bc082aba 100644 --- a/src/bootstrap/src/core/builder.rs +++ b/src/bootstrap/src/core/builder.rs @@ -382,10 +382,12 @@ impl StepDescription { } if !builder.config.skip.is_empty() && !matches!(builder.config.dry_run, DryRun::SelfCheck) { - builder.verbose(&format!( - "{:?} not skipped for {:?} -- not in {:?}", - pathset, self.name, builder.config.skip - )); + builder.verbose(|| { + println!( + "{:?} not skipped for {:?} -- not in {:?}", + pathset, self.name, builder.config.skip + ) + }); } false } @@ -1093,10 +1095,9 @@ impl<'a> Builder<'a> { // Avoid deleting the rustlib/ directory we just copied // (in `impl Step for Sysroot`). if !builder.download_rustc() { - builder.verbose(&format!( - "Removing sysroot {} to avoid caching bugs", - sysroot.display() - )); + builder.verbose(|| { + println!("Removing sysroot {} to avoid caching bugs", sysroot.display()) + }); let _ = fs::remove_dir_all(&sysroot); t!(fs::create_dir_all(&sysroot)); } @@ -1436,7 +1437,7 @@ impl<'a> Builder<'a> { let sysroot_str = sysroot.as_os_str().to_str().expect("sysroot should be UTF-8"); if !matches!(self.config.dry_run, DryRun::SelfCheck) { - self.verbose_than(0, &format!("using sysroot {sysroot_str}")); + self.verbose_than(0, || println!("using sysroot {sysroot_str}")); } let mut rustflags = Rustflags::new(target); @@ -2102,11 +2103,11 @@ impl<'a> Builder<'a> { panic!("{}", out); } if let Some(out) = self.cache.get(&step) { - self.verbose_than(1, &format!("{}c {:?}", " ".repeat(stack.len()), step)); + self.verbose_than(1, || println!("{}c {:?}", " ".repeat(stack.len()), step)); return out; } - self.verbose_than(1, &format!("{}> {:?}", " ".repeat(stack.len()), step)); + self.verbose_than(1, || println!("{}> {:?}", " ".repeat(stack.len()), step)); stack.push(Box::new(step.clone())); } @@ -2144,7 +2145,7 @@ impl<'a> Builder<'a> { let cur_step = stack.pop().expect("step stack empty"); assert_eq!(cur_step.downcast_ref(), Some(&step)); } - self.verbose_than(1, &format!("{}< {:?}", " ".repeat(self.stack.borrow().len()), step)); + self.verbose_than(1, || println!("{}< {:?}", " ".repeat(self.stack.borrow().len()), step)); self.cache.put(step, out.clone()); out } diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index ae5169e938390..d317e92a18196 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -2043,7 +2043,7 @@ impl Config { if self.dry_run() { return Ok(()); } - self.verbose(&format!("running: {cmd:?}")); + self.verbose(|| println!("running: {cmd:?}")); build_helper::util::try_run(cmd, self.is_verbose()) } @@ -2230,9 +2230,10 @@ impl Config { } } - pub fn verbose(&self, msg: &str) { + /// Runs a function if verbosity is greater than 0 + pub fn verbose(&self, f: impl Fn()) { if self.verbose > 0 { - println!("{msg}"); + f() } } diff --git a/src/bootstrap/src/core/download.rs b/src/bootstrap/src/core/download.rs index 27829eab9379d..251138388cafd 100644 --- a/src/bootstrap/src/core/download.rs +++ b/src/bootstrap/src/core/download.rs @@ -61,7 +61,7 @@ impl Config { if self.dry_run() { return true; } - self.verbose(&format!("running: {cmd:?}")); + self.verbose(|| println!("running: {cmd:?}")); check_run(cmd, self.is_verbose()) } @@ -195,7 +195,7 @@ impl Config { } fn download_file(&self, url: &str, dest_path: &Path, help_on_error: &str) { - self.verbose(&format!("download {url}")); + self.verbose(|| println!("download {url}")); // Use a temporary file in case we crash while downloading, to avoid a corrupt download in cache/. let tempfile = self.tempdir().join(dest_path.file_name().unwrap()); // While bootstrap itself only supports http and https downloads, downstream forks might @@ -300,7 +300,9 @@ impl Config { } short_path = t!(short_path.strip_prefix(pattern)); let dst_path = dst.join(short_path); - self.verbose(&format!("extracting {} to {}", original_path.display(), dst.display())); + self.verbose(|| { + println!("extracting {} to {}", original_path.display(), dst.display()) + }); if !t!(member.unpack_in(dst)) { panic!("path traversal attack ??"); } @@ -323,7 +325,7 @@ impl Config { pub(crate) fn verify(&self, path: &Path, expected: &str) -> bool { use sha2::Digest; - self.verbose(&format!("verifying {}", path.display())); + self.verbose(|| println!("verifying {}", path.display())); if self.dry_run() { return false; @@ -379,7 +381,7 @@ enum DownloadSource { /// Functions that are only ever called once, but named for clarify and to avoid thousand-line functions. impl Config { pub(crate) fn download_clippy(&self) -> PathBuf { - self.verbose("downloading stage0 clippy artifacts"); + self.verbose(|| println!("downloading stage0 clippy artifacts")); let date = &self.stage0_metadata.compiler.date; let version = &self.stage0_metadata.compiler.version; @@ -469,7 +471,7 @@ impl Config { } pub(crate) fn download_ci_rustc(&self, commit: &str) { - self.verbose(&format!("using downloaded stage2 artifacts from CI (commit {commit})")); + self.verbose(|| println!("using downloaded stage2 artifacts from CI (commit {commit})")); let version = self.artifact_version_part(commit); // download-rustc doesn't need its own cargo, it can just use beta's. But it does need the @@ -486,7 +488,7 @@ impl Config { } pub(crate) fn download_beta_toolchain(&self) { - self.verbose("downloading stage0 beta artifacts"); + self.verbose(|| println!("downloading stage0 beta artifacts")); let date = &self.stage0_metadata.compiler.date; let version = &self.stage0_metadata.compiler.version; @@ -625,10 +627,12 @@ impl Config { self.unpack(&tarball, &bin_root, prefix); return; } else { - self.verbose(&format!( - "ignoring cached file {} due to failed verification", - tarball.display() - )); + self.verbose(|| { + println!( + "ignoring cached file {} due to failed verification", + tarball.display() + ) + }); self.remove(&tarball); } } diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 6520b7ed08988..b3e6989b33d16 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -288,7 +288,7 @@ macro_rules! forward { } forward! { - verbose(msg: &str), + verbose(f: impl Fn()), is_verbose() -> bool, create(path: &Path, s: &str), remove(f: &Path), @@ -440,11 +440,11 @@ impl Build { .unwrap() .trim(); if local_release.split('.').take(2).eq(version.split('.').take(2)) { - build.verbose(&format!("auto-detected local-rebuild {local_release}")); + build.verbose(|| println!("auto-detected local-rebuild {local_release}")); build.local_rebuild = true; } - build.verbose("finding compilers"); + build.verbose(|| println!("finding compilers")); utils::cc_detect::find(&build); // When running `setup`, the profile is about to change, so any requirements we have now may // be different on the next invocation. Don't check for them until the next time x.py is @@ -452,7 +452,7 @@ impl Build { // // Similarly, for `setup` we don't actually need submodules or cargo metadata. if !matches!(build.config.cmd, Subcommand::Setup { .. }) { - build.verbose("running sanity check"); + build.verbose(|| println!("running sanity check")); crate::core::sanity::check(&mut build); // Make sure we update these before gathering metadata so we don't get an error about missing @@ -464,7 +464,7 @@ impl Build { // Now, update all existing submodules. build.update_existing_submodules(); - build.verbose("learning about cargo"); + build.verbose(|| println!("learning about cargo")); crate::core::metadata::build(&mut build); } @@ -693,7 +693,7 @@ impl Build { let stamp = dir.join(".stamp"); let mut cleared = false; if mtime(&stamp) < mtime(input) { - self.verbose(&format!("Dirty - {}", dir.display())); + self.verbose(|| println!("Dirty - {}", dir.display())); let _ = fs::remove_dir_all(dir); cleared = true; } else if stamp.exists() { @@ -986,7 +986,7 @@ impl Build { } let command = cmd.into(); - self.verbose(&format!("running: {command:?}")); + self.verbose(|| println!("running: {command:?}")); let (output, print_error) = match command.output_mode { mode @ (OutputMode::PrintAll | OutputMode::PrintOutput) => ( @@ -1044,14 +1044,15 @@ impl Build { } } + /// Check if verbosity is greater than the `level` pub fn is_verbose_than(&self, level: usize) -> bool { self.verbosity > level } - /// Prints a message if this build is configured in more verbose mode than `level`. - fn verbose_than(&self, level: usize, msg: &str) { + /// Runs a function if verbosity is greater than `level`. + fn verbose_than(&self, level: usize, f: impl Fn()) { if self.is_verbose_than(level) { - println!("{msg}"); + f() } } @@ -1627,7 +1628,7 @@ impl Build { if self.config.dry_run() { return; } - self.verbose_than(1, &format!("Copy {src:?} to {dst:?}")); + self.verbose_than(1, || println!("Copy {src:?} to {dst:?}")); if src == dst { return; } @@ -1718,7 +1719,7 @@ impl Build { return; } let dst = dstdir.join(src.file_name().unwrap()); - self.verbose_than(1, &format!("Install {src:?} to {dst:?}")); + self.verbose_than(1, || println!("Install {src:?} to {dst:?}")); t!(fs::create_dir_all(dstdir)); if !src.exists() { panic!("ERROR: File \"{}\" not found!", src.display()); diff --git a/src/bootstrap/src/utils/cc_detect.rs b/src/bootstrap/src/utils/cc_detect.rs index ff2992bc896ec..3ba4e0cb686e6 100644 --- a/src/bootstrap/src/utils/cc_detect.rs +++ b/src/bootstrap/src/utils/cc_detect.rs @@ -145,15 +145,15 @@ pub fn find_target(build: &Build, target: TargetSelection) { build.cxx.borrow_mut().insert(target, compiler); } - build.verbose(&format!("CC_{} = {:?}", &target.triple, build.cc(target))); - build.verbose(&format!("CFLAGS_{} = {:?}", &target.triple, cflags)); + build.verbose(|| println!("CC_{} = {:?}", &target.triple, build.cc(target))); + build.verbose(|| println!("CFLAGS_{} = {:?}", &target.triple, cflags)); if let Ok(cxx) = build.cxx(target) { let cxxflags = build.cflags(target, GitRepo::Rustc, CLang::Cxx); - build.verbose(&format!("CXX_{} = {:?}", &target.triple, cxx)); - build.verbose(&format!("CXXFLAGS_{} = {:?}", &target.triple, cxxflags)); + build.verbose(|| println!("CXX_{} = {:?}", &target.triple, cxx)); + build.verbose(|| println!("CXXFLAGS_{} = {:?}", &target.triple, cxxflags)); } if let Some(ar) = ar { - build.verbose(&format!("AR_{} = {:?}", &target.triple, ar)); + build.verbose(|| println!("AR_{} = {:?}", &target.triple, ar)); build.ar.borrow_mut().insert(target, ar); } diff --git a/src/bootstrap/src/utils/render_tests.rs b/src/bootstrap/src/utils/render_tests.rs index cbd01606a895a..70f25b2cc87c1 100644 --- a/src/bootstrap/src/utils/render_tests.rs +++ b/src/bootstrap/src/utils/render_tests.rs @@ -44,7 +44,7 @@ pub(crate) fn try_run_tests(builder: &Builder<'_>, cmd: &mut Command, stream: bo fn run_tests(builder: &Builder<'_>, cmd: &mut Command, stream: bool) -> bool { cmd.stdout(Stdio::piped()); - builder.verbose(&format!("running: {cmd:?}")); + builder.verbose(|| println!("running: {cmd:?}")); let mut process = cmd.spawn().unwrap(); diff --git a/src/bootstrap/src/utils/tarball.rs b/src/bootstrap/src/utils/tarball.rs index a14dfd1ca1234..03f56cba29d8d 100644 --- a/src/bootstrap/src/utils/tarball.rs +++ b/src/bootstrap/src/utils/tarball.rs @@ -328,7 +328,9 @@ impl<'a> Tarball<'a> { // For `x install` tarball files aren't needed, so we can speed up the process by not producing them. let compression_profile = if self.builder.kind == Kind::Install { - self.builder.verbose("Forcing dist.compression-profile = 'no-op' for `x install`."); + self.builder.verbose(|| { + println!("Forcing dist.compression-profile = 'no-op' for `x install`.") + }); // "no-op" indicates that the rust-installer won't produce compressed tarball sources. "no-op" } else {