Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bootstrap: Don't eagerly format verbose messages #122354

Merged
merged 1 commit into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/bootstrap/src/core/build_steps/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/src/core/build_steps/dist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}
Expand Down Expand Up @@ -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
Expand Down
25 changes: 13 additions & 12 deletions src/bootstrap/src/core/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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()));
}

Expand Down Expand Up @@ -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
}
Expand Down
7 changes: 4 additions & 3 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}

Expand Down Expand Up @@ -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()
}
}

Expand Down
26 changes: 15 additions & 11 deletions src/bootstrap/src/core/download.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 ??");
}
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}
Expand Down
25 changes: 13 additions & 12 deletions src/bootstrap/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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
Expand All @@ -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);
}

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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) => (
Expand Down Expand Up @@ -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()
}
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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());
Expand Down
10 changes: 5 additions & 5 deletions src/bootstrap/src/utils/cc_detect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/src/utils/render_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
4 changes: 3 additions & 1 deletion src/bootstrap/src/utils/tarball.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading