Rollup merge of rust-lang#122354 - clubby789:bootstrap-eager-verbose, r=albertlarsan68

bootstrap: Don't eagerly format verbose messages

We `format!` a lot of messages which are only used when we are at some level of verbosity - do this lazily instead

Author: workingjubilee

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;

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

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

@@ -551,15 +551,15 @@ 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");
             // Output is "<sysroot>\n".
             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

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);
@@ -2103,11 +2104,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()));
         }
 
@@ -2145,7 +2146,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
     }

src/bootstrap/src/core/config/config.rs

@@ -2059,7 +2059,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())
     }
 
@@ -2246,9 +2246,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()
         }
     }
 

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

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,19 +440,19 @@ 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
         // run. This is ok because `setup` never runs any build commands, so it won't fail if commands are missing.
         //
         // 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()
         }
     }
 
@@ -1654,7 +1655,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;
         }
@@ -1745,7 +1746,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());

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

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

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 {