From 9192479dc352c96927f3bfeda746d07c71a1a470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 28 Jun 2024 10:46:52 +0200 Subject: [PATCH 01/11] Improve documentation --- src/bootstrap/src/utils/exec.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index 8bcb2301f1ace..87a3a2b0b284e 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -34,7 +34,8 @@ pub enum OutputMode { /// By default, the command will print its stdout/stderr to stdout/stderr of bootstrap /// ([OutputMode::OnlyOutput]). If bootstrap uses verbose mode, then it will also print the /// command itself in case of failure ([OutputMode::All]). -/// If you want to handle the output programmatically, use `output_mode(OutputMode::OnlyOnFailure)`. +/// If you want to handle the output programmatically, use `output_mode(OutputMode::OnlyOnFailure)`, +/// which will print the output only if the command fails. /// /// [allow_failure]: BootstrapCommand::allow_failure /// [delay_failure]: BootstrapCommand::delay_failure @@ -113,7 +114,7 @@ impl BootstrapCommand { } } -/// This implementation is temporary, until all `Command` invocations are migrated to +/// FIXME: This implementation is temporary, until all `Command` invocations are migrated to /// `BootstrapCommand`. impl<'a> From<&'a mut Command> for BootstrapCommand { fn from(command: &'a mut Command) -> Self { @@ -138,7 +139,7 @@ impl<'a> From<&'a mut Command> for BootstrapCommand { } } -/// This implementation is temporary, until all `Command` invocations are migrated to +/// FIXME: This implementation is temporary, until all `Command` invocations are migrated to /// `BootstrapCommand`. impl<'a> From<&'a mut BootstrapCommand> for BootstrapCommand { fn from(command: &'a mut BootstrapCommand) -> Self { From a34d0a8d5f2abbbe4bf28d829bbf7490f23d5a9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 22 Jun 2024 17:21:33 +0200 Subject: [PATCH 02/11] Make `git` helper return `BootstrapCmd` --- src/bootstrap/src/core/build_steps/format.rs | 38 +++++++-------- src/bootstrap/src/core/build_steps/llvm.rs | 5 +- src/bootstrap/src/core/build_steps/perf.rs | 1 + src/bootstrap/src/core/build_steps/setup.rs | 1 + src/bootstrap/src/core/build_steps/test.rs | 2 +- .../src/core/build_steps/toolstate.rs | 19 ++++++-- src/bootstrap/src/core/config/config.rs | 26 +++++++---- src/bootstrap/src/lib.rs | 46 ++++++++++++------- src/bootstrap/src/utils/channel.rs | 14 +++--- src/bootstrap/src/utils/exec.rs | 36 ++++++++------- src/bootstrap/src/utils/helpers.rs | 4 +- 11 files changed, 112 insertions(+), 80 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/format.rs b/src/bootstrap/src/core/build_steps/format.rs index f5e346726868a..4583eb1c6cd85 100644 --- a/src/bootstrap/src/core/build_steps/format.rs +++ b/src/bootstrap/src/core/build_steps/format.rs @@ -7,7 +7,7 @@ use build_helper::git::get_git_modified_files; use ignore::WalkBuilder; use std::collections::VecDeque; use std::path::{Path, PathBuf}; -use std::process::{Command, Stdio}; +use std::process::Command; use std::sync::mpsc::SyncSender; use std::sync::Mutex; @@ -160,35 +160,29 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) { override_builder.add(&format!("!{ignore}")).expect(&ignore); } } - let git_available = match helpers::git(None) - .arg("--version") - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - { - Ok(status) => status.success(), - Err(_) => false, - }; + let git_available = build + .run(helpers::git(None).print_on_failure().allow_failure().arg("--version")) + .is_success(); let mut adjective = None; if git_available { - let in_working_tree = match helpers::git(Some(&build.src)) - .arg("rev-parse") - .arg("--is-inside-work-tree") - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - { - Ok(status) => status.success(), - Err(_) => false, - }; + let in_working_tree = build + .run( + helpers::git(Some(&build.src)) + .print_on_failure() + .allow_failure() + .arg("rev-parse") + .arg("--is-inside-work-tree"), + ) + .is_success(); if in_working_tree { let untracked_paths_output = output( - helpers::git(Some(&build.src)) + &mut helpers::git(Some(&build.src)) .arg("status") .arg("--porcelain") .arg("-z") - .arg("--untracked-files=normal"), + .arg("--untracked-files=normal") + .command, ); let untracked_paths: Vec<_> = untracked_paths_output .split_terminator('\0') diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index 8e6795b11bd21..ba21e2ad32d05 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -172,7 +172,7 @@ pub(crate) fn detect_llvm_sha(config: &Config, is_git: bool) -> String { // the LLVM shared object file is named `LLVM-12-rust-{version}-nightly` config.src.join("src/version"), ]); - output(&mut rev_list).trim().to_owned() + output(&mut rev_list.command).trim().to_owned() } else if let Some(info) = channel::read_commit_info_file(&config.src) { info.sha.trim().to_owned() } else { @@ -253,7 +253,8 @@ pub(crate) fn is_ci_llvm_modified(config: &Config) -> bool { // We assume we have access to git, so it's okay to unconditionally pass // `true` here. let llvm_sha = detect_llvm_sha(config, true); - let head_sha = output(helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD")); + let head_sha = + output(&mut helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD").command); let head_sha = head_sha.trim(); llvm_sha == head_sha } diff --git a/src/bootstrap/src/core/build_steps/perf.rs b/src/bootstrap/src/core/build_steps/perf.rs index f41b5fe10f1d9..20ab1836e0068 100644 --- a/src/bootstrap/src/core/build_steps/perf.rs +++ b/src/bootstrap/src/core/build_steps/perf.rs @@ -2,6 +2,7 @@ use crate::core::build_steps::compile::{Std, Sysroot}; use crate::core::build_steps::tool::{RustcPerf, Tool}; use crate::core::builder::Builder; use crate::core::config::DebuginfoLevel; +use crate::utils::exec::BootstrapCommand; /// Performs profiling using `rustc-perf` on a built version of the compiler. pub fn perf(builder: &Builder<'_>) { diff --git a/src/bootstrap/src/core/build_steps/setup.rs b/src/bootstrap/src/core/build_steps/setup.rs index 947c74f32c99e..e6a09e8cb8e8c 100644 --- a/src/bootstrap/src/core/build_steps/setup.rs +++ b/src/bootstrap/src/core/build_steps/setup.rs @@ -484,6 +484,7 @@ impl Step for Hook { fn install_git_hook_maybe(config: &Config) -> io::Result<()> { let git = helpers::git(Some(&config.src)) .args(["rev-parse", "--git-common-dir"]) + .command .output() .map(|output| { assert!(output.status.success(), "failed to run `git`"); diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 1460a2290197b..918e4e2b90726 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -2349,7 +2349,7 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) -> cmd = cmd.delay_failure(); if !builder.config.verbose_tests { - cmd = cmd.quiet(); + cmd = cmd.print_on_failure(); } builder.run(cmd).is_success() } diff --git a/src/bootstrap/src/core/build_steps/toolstate.rs b/src/bootstrap/src/core/build_steps/toolstate.rs index 9e6d03349b5fb..e3e7931a5a2ab 100644 --- a/src/bootstrap/src/core/build_steps/toolstate.rs +++ b/src/bootstrap/src/core/build_steps/toolstate.rs @@ -101,8 +101,13 @@ fn print_error(tool: &str, submodule: &str) { fn check_changed_files(toolstates: &HashMap, ToolState>) { // Changed files - let output = - helpers::git(None).arg("diff").arg("--name-status").arg("HEAD").arg("HEAD^").output(); + let output = helpers::git(None) + .arg("diff") + .arg("--name-status") + .arg("HEAD") + .arg("HEAD^") + .command + .output(); let output = match output { Ok(o) => o, Err(e) => { @@ -324,6 +329,7 @@ fn checkout_toolstate_repo() { .arg("--depth=1") .arg(toolstate_repo()) .arg(TOOLSTATE_DIR) + .command .status(); let success = match status { Ok(s) => s.success(), @@ -337,7 +343,8 @@ fn checkout_toolstate_repo() { /// Sets up config and authentication for modifying the toolstate repo. fn prepare_toolstate_config(token: &str) { fn git_config(key: &str, value: &str) { - let status = helpers::git(None).arg("config").arg("--global").arg(key).arg(value).status(); + let status = + helpers::git(None).arg("config").arg("--global").arg(key).arg(value).command.status(); let success = match status { Ok(s) => s.success(), Err(_) => false, @@ -406,6 +413,7 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) { .arg("-a") .arg("-m") .arg(&message) + .command .status()); if !status.success() { success = true; @@ -416,6 +424,7 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) { .arg("push") .arg("origin") .arg("master") + .command .status()); // If we successfully push, exit. if status.success() { @@ -428,12 +437,14 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) { .arg("fetch") .arg("origin") .arg("master") + .command .status()); assert!(status.success()); let status = t!(helpers::git(Some(Path::new(TOOLSTATE_DIR))) .arg("reset") .arg("--hard") .arg("origin/master") + .command .status()); assert!(status.success()); } @@ -449,7 +460,7 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) { /// `publish_toolstate.py` script if the PR passes all tests and is merged to /// master. fn publish_test_results(current_toolstate: &ToolstateData) { - let commit = t!(helpers::git(None).arg("rev-parse").arg("HEAD").output()); + let commit = t!(helpers::git(None).arg("rev-parse").arg("HEAD").command.output()); let commit = t!(String::from_utf8(commit.stdout)); let toolstate_serialized = t!(serde_json::to_string(¤t_toolstate)); diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 948f97e746f42..10ac6c93e9a43 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -1259,6 +1259,7 @@ impl Config { cmd.arg("rev-parse").arg("--show-cdup"); // Discard stderr because we expect this to fail when building from a tarball. let output = cmd + .command .stderr(std::process::Stdio::null()) .output() .ok() @@ -2141,7 +2142,7 @@ impl Config { let mut git = helpers::git(Some(&self.src)); git.arg("show").arg(format!("{commit}:{}", file.to_str().unwrap())); - output(&mut git) + output(&mut git.command) } /// Bootstrap embeds a version number into the name of shared libraries it uploads in CI. @@ -2445,8 +2446,9 @@ impl Config { }; // Handle running from a directory other than the top level - let top_level = - output(helpers::git(Some(&self.src)).args(["rev-parse", "--show-toplevel"])); + let top_level = output( + &mut helpers::git(Some(&self.src)).args(["rev-parse", "--show-toplevel"]).command, + ); let top_level = top_level.trim_end(); let compiler = format!("{top_level}/compiler/"); let library = format!("{top_level}/library/"); @@ -2454,10 +2456,11 @@ impl Config { // Look for a version to compare to based on the current commit. // Only commits merged by bors will have CI artifacts. let merge_base = output( - helpers::git(Some(&self.src)) + &mut helpers::git(Some(&self.src)) .arg("rev-list") .arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email)) - .args(["-n1", "--first-parent", "HEAD"]), + .args(["-n1", "--first-parent", "HEAD"]) + .command, ); let commit = merge_base.trim_end(); if commit.is_empty() { @@ -2471,6 +2474,7 @@ impl Config { // Warn if there were changes to the compiler or standard library since the ancestor commit. let has_changes = !t!(helpers::git(Some(&self.src)) .args(["diff-index", "--quiet", commit, "--", &compiler, &library]) + .command .status()) .success(); if has_changes { @@ -2542,17 +2546,19 @@ impl Config { if_unchanged: bool, ) -> Option { // Handle running from a directory other than the top level - let top_level = - output(helpers::git(Some(&self.src)).args(["rev-parse", "--show-toplevel"])); + let top_level = output( + &mut helpers::git(Some(&self.src)).args(["rev-parse", "--show-toplevel"]).command, + ); let top_level = top_level.trim_end(); // Look for a version to compare to based on the current commit. // Only commits merged by bors will have CI artifacts. let merge_base = output( - helpers::git(Some(&self.src)) + &mut helpers::git(Some(&self.src)) .arg("rev-list") .arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email)) - .args(["-n1", "--first-parent", "HEAD"]), + .args(["-n1", "--first-parent", "HEAD"]) + .command, ); let commit = merge_base.trim_end(); if commit.is_empty() { @@ -2571,7 +2577,7 @@ impl Config { git.arg(format!("{top_level}/{path}")); } - let has_changes = !t!(git.status()).success(); + let has_changes = !t!(git.command.status()).success(); if has_changes { if if_unchanged { if self.verbose > 0 { diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index c12449fdc4ae0..ae2982ea56f09 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -494,11 +494,12 @@ impl Build { let submodule_git = || helpers::git(Some(&absolute_path)); // Determine commit checked out in submodule. - let checked_out_hash = output(submodule_git().args(["rev-parse", "HEAD"])); + let checked_out_hash = output(&mut submodule_git().args(["rev-parse", "HEAD"]).command); let checked_out_hash = checked_out_hash.trim_end(); // Determine commit that the submodule *should* have. - let recorded = - output(helpers::git(Some(&self.src)).args(["ls-tree", "HEAD"]).arg(relative_path)); + let recorded = output( + &mut helpers::git(Some(&self.src)).args(["ls-tree", "HEAD"]).arg(relative_path).command, + ); let actual_hash = recorded .split_whitespace() .nth(2) @@ -521,6 +522,7 @@ impl Build { let current_branch = { let output = helpers::git(Some(&self.src)) .args(["symbolic-ref", "--short", "HEAD"]) + .command .stderr(Stdio::inherit()) .output(); let output = t!(output); @@ -546,7 +548,7 @@ impl Build { git }; // NOTE: doesn't use `try_run` because this shouldn't print an error if it fails. - if !update(true).status().map_or(false, |status| status.success()) { + if !update(true).command.status().map_or(false, |status| status.success()) { self.run(update(false)); } @@ -577,12 +579,15 @@ impl Build { if !self.config.submodules(self.rust_info()) { return; } - let output = output( - helpers::git(Some(&self.src)) - .args(["config", "--file"]) - .arg(self.config.src.join(".gitmodules")) - .args(["--get-regexp", "path"]), - ); + let output = self + .run( + helpers::git(Some(&self.src)) + .quiet() + .args(["config", "--file"]) + .arg(self.config.src.join(".gitmodules")) + .args(["--get-regexp", "path"]), + ) + .stdout(); for line in output.lines() { // Look for `submodule.$name.path = $path` // Sample output: `submodule.src/rust-installer.path src/tools/rust-installer` @@ -950,7 +955,10 @@ impl Build { command.command.status().map(|status| status.into()), matches!(mode, OutputMode::All), ), - OutputMode::OnlyOnFailure => (command.command.output().map(|o| o.into()), true), + mode @ (OutputMode::OnlyOnFailure | OutputMode::Quiet) => ( + command.command.output().map(|o| o.into()), + matches!(mode, OutputMode::OnlyOnFailure), + ), }; let output = match output { @@ -1480,14 +1488,18 @@ impl Build { // Figure out how many merge commits happened since we branched off master. // That's our beta number! // (Note that we use a `..` range, not the `...` symmetric difference.) - output( - helpers::git(Some(&self.src)).arg("rev-list").arg("--count").arg("--merges").arg( - format!( + self.run( + helpers::git(Some(&self.src)) + .quiet() + .arg("rev-list") + .arg("--count") + .arg("--merges") + .arg(format!( "refs/remotes/origin/{}..HEAD", self.config.stage0_metadata.config.nightly_branch - ), - ), + )), ) + .stdout() }); let n = count.trim().parse().unwrap(); self.prerelease_version.set(Some(n)); @@ -1914,6 +1926,7 @@ fn envify(s: &str) -> String { pub fn generate_smart_stamp_hash(dir: &Path, additional_input: &str) -> String { let diff = helpers::git(Some(dir)) .arg("diff") + .command .output() .map(|o| String::from_utf8(o.stdout).unwrap_or_default()) .unwrap_or_default(); @@ -1923,6 +1936,7 @@ pub fn generate_smart_stamp_hash(dir: &Path, additional_input: &str) -> String { .arg("--porcelain") .arg("-z") .arg("--untracked-files=normal") + .command .output() .map(|o| String::from_utf8(o.stdout).unwrap_or_default()) .unwrap_or_default(); diff --git a/src/bootstrap/src/utils/channel.rs b/src/bootstrap/src/utils/channel.rs index ce82c52f049e2..2ca86bdb0ed27 100644 --- a/src/bootstrap/src/utils/channel.rs +++ b/src/bootstrap/src/utils/channel.rs @@ -45,7 +45,7 @@ impl GitInfo { } // Make sure git commands work - match helpers::git(Some(dir)).arg("rev-parse").output() { + match helpers::git(Some(dir)).arg("rev-parse").command.output() { Ok(ref out) if out.status.success() => {} _ => return GitInfo::Absent, } @@ -58,15 +58,17 @@ impl GitInfo { // Ok, let's scrape some info let ver_date = output( - helpers::git(Some(dir)) + &mut helpers::git(Some(dir)) .arg("log") .arg("-1") .arg("--date=short") - .arg("--pretty=format:%cd"), + .arg("--pretty=format:%cd") + .command, + ); + let ver_hash = output(&mut helpers::git(Some(dir)).arg("rev-parse").arg("HEAD").command); + let short_ver_hash = output( + &mut helpers::git(Some(dir)).arg("rev-parse").arg("--short=9").arg("HEAD").command, ); - let ver_hash = output(helpers::git(Some(dir)).arg("rev-parse").arg("HEAD")); - let short_ver_hash = - output(helpers::git(Some(dir)).arg("rev-parse").arg("--short=9").arg("HEAD")); GitInfo::Present(Some(Info { commit_date: ver_date.trim().to_string(), sha: ver_hash.trim().to_string(), diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index 87a3a2b0b284e..dc30876601c6b 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -23,6 +23,8 @@ pub enum OutputMode { OnlyOutput, /// Suppress the output if the command succeeds, otherwise print the output. OnlyOnFailure, + /// Suppress the output of the command. + Quiet, } /// Wrapper around `std::process::Command`. @@ -105,10 +107,15 @@ impl BootstrapCommand { } /// Do not print the output of the command, unless it fails. - pub fn quiet(self) -> Self { + pub fn print_on_failure(self) -> Self { self.output_mode(OutputMode::OnlyOnFailure) } + /// Do not print the output of the command. + pub fn quiet(self) -> Self { + self.output_mode(OutputMode::Quiet) + } + pub fn output_mode(self, output_mode: OutputMode) -> Self { Self { output_mode: Some(output_mode), ..self } } @@ -116,15 +123,15 @@ impl BootstrapCommand { /// FIXME: This implementation is temporary, until all `Command` invocations are migrated to /// `BootstrapCommand`. -impl<'a> From<&'a mut Command> for BootstrapCommand { - fn from(command: &'a mut Command) -> Self { +impl<'a> From<&'a mut BootstrapCommand> for BootstrapCommand { + fn from(command: &'a mut BootstrapCommand) -> Self { // This is essentially a manual `Command::clone` - let mut cmd = Command::new(command.get_program()); - if let Some(dir) = command.get_current_dir() { + let mut cmd = Command::new(command.command.get_program()); + if let Some(dir) = command.command.get_current_dir() { cmd.current_dir(dir); } - cmd.args(command.get_args()); - for (key, value) in command.get_envs() { + cmd.args(command.command.get_args()); + for (key, value) in command.command.get_envs() { match value { Some(value) => { cmd.env(key, value); @@ -134,16 +141,11 @@ impl<'a> From<&'a mut Command> for BootstrapCommand { } } } - - cmd.into() - } -} - -/// FIXME: This implementation is temporary, until all `Command` invocations are migrated to -/// `BootstrapCommand`. -impl<'a> From<&'a mut BootstrapCommand> for BootstrapCommand { - fn from(command: &'a mut BootstrapCommand) -> Self { - BootstrapCommand::from(&mut command.command) + Self { + command: cmd, + output_mode: command.output_mode, + failure_behavior: command.failure_behavior, + } } } diff --git a/src/bootstrap/src/utils/helpers.rs b/src/bootstrap/src/utils/helpers.rs index adf18c0ace1b4..9c40065dfc4fa 100644 --- a/src/bootstrap/src/utils/helpers.rs +++ b/src/bootstrap/src/utils/helpers.rs @@ -498,8 +498,8 @@ pub fn check_cfg_arg(name: &str, values: Option<&[&str]>) -> String { /// manually building a git `Command`. This approach allows us to manage bootstrap-specific /// needs/hacks from a single source, rather than applying them on next to every `Command::new("git")`, /// which is painful to ensure that the required change is applied on each one of them correctly. -pub fn git(source_dir: Option<&Path>) -> Command { - let mut git = Command::new("git"); +pub fn git(source_dir: Option<&Path>) -> BootstrapCommand { + let mut git = BootstrapCommand::new("git"); if let Some(source_dir) = source_dir { git.current_dir(source_dir); From b14ff77c044652246809e197a3b0a0f23f0e1f74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 22 Jun 2024 12:32:55 +0200 Subject: [PATCH 03/11] Remove temporary `BootstrapCommand` trait impls --- src/bootstrap/src/core/build_steps/compile.rs | 2 +- src/bootstrap/src/core/build_steps/doc.rs | 6 +- src/bootstrap/src/core/build_steps/format.rs | 7 +- src/bootstrap/src/core/build_steps/run.rs | 2 +- src/bootstrap/src/core/build_steps/test.rs | 52 +++++------ src/bootstrap/src/core/build_steps/tool.rs | 4 +- src/bootstrap/src/core/builder.rs | 10 +- src/bootstrap/src/lib.rs | 81 ++++++++-------- src/bootstrap/src/utils/exec.rs | 93 +++++++------------ 9 files changed, 114 insertions(+), 143 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index de3b938e42731..c267d3b0c0cf6 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -2080,7 +2080,7 @@ pub fn stream_cargo( tail_args: Vec, cb: &mut dyn FnMut(CargoMessage<'_>), ) -> bool { - let mut cargo = BootstrapCommand::from(cargo).command; + let mut cargo = cargo.into_cmd().command; // Instruct Cargo to give us json messages on stdout, critically leaving // stderr as piped so we can get those pretty colors. let mut message_format = if builder.config.json_output { diff --git a/src/bootstrap/src/core/build_steps/doc.rs b/src/bootstrap/src/core/build_steps/doc.rs index 4a5af25b3b2f8..823e842693e96 100644 --- a/src/bootstrap/src/core/build_steps/doc.rs +++ b/src/bootstrap/src/core/build_steps/doc.rs @@ -738,7 +738,7 @@ fn doc_std( format!("library{} in {} format", crate_description(requested_crates), format.as_str()); let _guard = builder.msg_doc(compiler, description, target); - builder.run(cargo); + builder.run(cargo.into_cmd()); builder.cp_link_r(&out_dir, out); } @@ -863,7 +863,7 @@ impl Step for Rustc { let proc_macro_out_dir = builder.stage_out(compiler, Mode::Rustc).join("doc"); symlink_dir_force(&builder.config, &out, &proc_macro_out_dir); - builder.run(cargo); + builder.run(cargo.into_cmd()); if !builder.config.dry_run() { // Sanity check on linked compiler crates @@ -996,7 +996,7 @@ macro_rules! tool_doc { symlink_dir_force(&builder.config, &out, &proc_macro_out_dir); let _guard = builder.msg_doc(compiler, stringify!($tool).to_lowercase(), target); - builder.run(cargo); + builder.run(cargo.into_cmd()); if !builder.config.dry_run() { // Sanity check on linked doc directories diff --git a/src/bootstrap/src/core/build_steps/format.rs b/src/bootstrap/src/core/build_steps/format.rs index 4583eb1c6cd85..0372360c3cb74 100644 --- a/src/bootstrap/src/core/build_steps/format.rs +++ b/src/bootstrap/src/core/build_steps/format.rs @@ -160,16 +160,15 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) { override_builder.add(&format!("!{ignore}")).expect(&ignore); } } - let git_available = build - .run(helpers::git(None).print_on_failure().allow_failure().arg("--version")) - .is_success(); + let git_available = + build.run(helpers::git(None).capture().allow_failure().arg("--version")).is_success(); let mut adjective = None; if git_available { let in_working_tree = build .run( helpers::git(Some(&build.src)) - .print_on_failure() + .capture() .allow_failure() .arg("rev-parse") .arg("--is-inside-work-tree"), diff --git a/src/bootstrap/src/core/build_steps/run.rs b/src/bootstrap/src/core/build_steps/run.rs index 22d5efa5d95dd..496fe446d724e 100644 --- a/src/bootstrap/src/core/build_steps/run.rs +++ b/src/bootstrap/src/core/build_steps/run.rs @@ -158,7 +158,7 @@ impl Step for Miri { // after another --, so this must be at the end. miri.args(builder.config.args()); - builder.run(miri); + builder.run(miri.into_cmd()); } } diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 918e4e2b90726..9fc337dfd50db 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -26,7 +26,7 @@ use crate::core::builder::{Builder, Compiler, Kind, RunConfig, ShouldRun, Step}; use crate::core::config::flags::get_completion; use crate::core::config::flags::Subcommand; use crate::core::config::TargetSelection; -use crate::utils::exec::{BootstrapCommand, OutputMode}; +use crate::utils::exec::BootstrapCommand; use crate::utils::helpers::{ self, add_link_lib_path, add_rustdoc_cargo_linker_args, dylib_path, dylib_path_var, linker_args, linker_flags, output, t, target_supports_cranelift_backend, up_to_date, @@ -150,16 +150,13 @@ You can skip linkcheck with --skip src/tools/linkchecker" builder.default_doc(&[]); // Build the linkchecker before calling `msg`, since GHA doesn't support nested groups. - let mut linkchecker = builder.tool_cmd(Tool::Linkchecker); + let linkchecker = builder.tool_cmd(Tool::Linkchecker); // Run the linkchecker. let _guard = builder.msg(Kind::Test, compiler.stage, "Linkcheck", bootstrap_host, bootstrap_host); let _time = helpers::timeit(builder); - builder.run( - BootstrapCommand::from(linkchecker.arg(builder.out.join(host.triple).join("doc"))) - .delay_failure(), - ); + builder.run(linkchecker.delay_failure().arg(builder.out.join(host.triple).join("doc"))); } fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { @@ -217,10 +214,7 @@ impl Step for HtmlCheck { )); builder.run( - BootstrapCommand::from( - builder.tool_cmd(Tool::HtmlChecker).arg(builder.doc_out(self.target)), - ) - .delay_failure(), + builder.tool_cmd(Tool::HtmlChecker).delay_failure().arg(builder.doc_out(self.target)), ); } } @@ -260,14 +254,13 @@ impl Step for Cargotest { let _time = helpers::timeit(builder); let mut cmd = builder.tool_cmd(Tool::CargoTest); - let cmd = cmd - .arg(&cargo) + cmd.arg(&cargo) .arg(&out_dir) .args(builder.config.test_args()) .env("RUSTC", builder.rustc(compiler)) .env("RUSTDOC", builder.rustdoc(compiler)); - add_rustdoc_cargo_linker_args(cmd, builder, compiler.host, LldThreads::No); - builder.run(BootstrapCommand::from(cmd).delay_failure()); + add_rustdoc_cargo_linker_args(&mut cmd, builder, compiler.host, LldThreads::No); + builder.run(cmd.delay_failure()); } } @@ -763,12 +756,12 @@ impl Step for Clippy { cargo.env("HOST_LIBS", host_libs); cargo.add_rustc_lib_path(builder); - let mut cargo = prepare_cargo_test(cargo, &[], &[], "clippy", compiler, host, builder); + let cargo = prepare_cargo_test(cargo, &[], &[], "clippy", compiler, host, builder); let _guard = builder.msg_sysroot_tool(Kind::Test, compiler.stage, "clippy", host, host); // Clippy reports errors if it blessed the outputs - if builder.run(BootstrapCommand::from(&mut cargo).allow_failure()).is_success() { + if builder.run(cargo.allow_failure()).is_success() { // The tests succeeded; nothing to do. return; } @@ -821,7 +814,7 @@ impl Step for RustdocTheme { .env("RUSTC_BOOTSTRAP", "1"); cmd.args(linker_args(builder, self.compiler.host, LldThreads::No)); - builder.run(BootstrapCommand::from(&mut cmd).delay_failure()); + builder.run(cmd.delay_failure()); } } @@ -1099,7 +1092,7 @@ HELP: to skip test's attempt to check tidiness, pass `--skip src/tools/tidy` to } builder.info("tidy check"); - builder.run(BootstrapCommand::from(&mut cmd).delay_failure()); + builder.run(cmd.delay_failure()); builder.info("x.py completions check"); let [bash, zsh, fish, powershell] = ["x.py.sh", "x.py.zsh", "x.py.fish", "x.py.ps1"] @@ -1306,7 +1299,7 @@ impl Step for RunMakeSupport { &[], ); - builder.run(cargo); + builder.run(cargo.into_cmd()); let lib_name = "librun_make_support.rlib"; let lib = builder.tools_dir(self.compiler).join(lib_name); @@ -2187,7 +2180,7 @@ impl BookTest { compiler.host, ); let _time = helpers::timeit(builder); - let cmd = BootstrapCommand::from(&mut rustbook_cmd).delay_failure(); + let cmd = rustbook_cmd.delay_failure(); let toolstate = if builder.run(cmd).is_success() { ToolState::TestPass } else { ToolState::TestFail }; builder.save_toolstate(self.name, toolstate); @@ -2318,7 +2311,7 @@ impl Step for ErrorIndex { let guard = builder.msg(Kind::Test, compiler.stage, "error-index", compiler.host, compiler.host); let _time = helpers::timeit(builder); - builder.run(BootstrapCommand::from(&mut tool).output_mode(OutputMode::OnlyOnFailure)); + builder.run(tool.capture()); drop(guard); // The tests themselves need to link to std, so make sure it is // available. @@ -2349,7 +2342,7 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) -> cmd = cmd.delay_failure(); if !builder.config.verbose_tests { - cmd = cmd.print_on_failure(); + cmd = cmd.capture(); } builder.run(cmd).is_success() } @@ -2375,10 +2368,13 @@ impl Step for RustcGuide { builder.update_submodule(&relative_path); let src = builder.src.join(relative_path); - let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook); - let cmd = BootstrapCommand::from(rustbook_cmd.arg("linkcheck").arg(&src)).delay_failure(); - let toolstate = - if builder.run(cmd).is_success() { ToolState::TestPass } else { ToolState::TestFail }; + let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook).delay_failure(); + rustbook_cmd.arg("linkcheck").arg(&src); + let toolstate = if builder.run(rustbook_cmd).is_success() { + ToolState::TestPass + } else { + ToolState::TestFail + }; builder.save_toolstate("rustc-dev-guide", toolstate); } } @@ -3347,7 +3343,7 @@ impl Step for CodegenCranelift { .arg("testsuite.extended_sysroot"); cargo.args(builder.config.test_args()); - builder.run(cargo); + builder.run(cargo.into_cmd()); } } @@ -3472,6 +3468,6 @@ impl Step for CodegenGCC { .arg("--std-tests"); cargo.args(builder.config.test_args()); - builder.run(cargo); + builder.run(cargo.into_cmd()); } } diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index b34640439121a..20c7a30f1a823 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -603,7 +603,7 @@ impl Step for Rustdoc { &self.compiler.host, &target, ); - builder.run(cargo); + builder.run(cargo.into_cmd()); // Cargo adds a number of paths to the dylib search path on windows, which results in // the wrong rustdoc being executed. To avoid the conflicting rustdocs, we name the "tool" @@ -858,7 +858,7 @@ impl Step for LlvmBitcodeLinker { &self.extra_features, ); - builder.run(cargo); + builder.run(cargo.into_cmd()); let tool_out = builder .cargo_out(self.compiler, Mode::ToolRustc, self.target) diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs index 58d6e7a58e308..35de91c41d4f1 100644 --- a/src/bootstrap/src/core/builder.rs +++ b/src/bootstrap/src/core/builder.rs @@ -2398,6 +2398,10 @@ impl Cargo { cargo } + pub fn into_cmd(self) -> BootstrapCommand { + self.into() + } + /// Same as `Cargo::new` except this one doesn't configure the linker with `Cargo::configure_linker` pub fn new_for_mir_opt_tests( builder: &Builder<'_>, @@ -2622,9 +2626,3 @@ impl From for BootstrapCommand { cargo.command } } - -impl From for Command { - fn from(cargo: Cargo) -> Command { - BootstrapCommand::from(cargo).command - } -} diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index ae2982ea56f09..ae3d18e8774bc 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -555,10 +555,7 @@ impl Build { // Save any local changes, but avoid running `git stash pop` if there are none (since it will exit with an error). // diff-index reports the modifications through the exit status let has_local_modifications = self - .run( - BootstrapCommand::from(submodule_git().args(["diff-index", "--quiet", "HEAD"])) - .allow_failure(), - ) + .run(submodule_git().allow_failure().args(["diff-index", "--quiet", "HEAD"])) .is_failure(); if has_local_modifications { self.run(submodule_git().args(["stash", "push"])); @@ -582,7 +579,7 @@ impl Build { let output = self .run( helpers::git(Some(&self.src)) - .quiet() + .capture() .args(["config", "--file"]) .arg(self.config.src.join(".gitmodules")) .args(["--get-regexp", "path"]), @@ -937,69 +934,71 @@ impl Build { /// Execute a command and return its output. /// This method should be used for all command executions in bootstrap. - fn run>(&self, command: C) -> CommandOutput { + fn run>(&self, mut command: C) -> CommandOutput { if self.config.dry_run() { return CommandOutput::default(); } - let mut command = command.into(); + let command = command.as_mut(); self.verbose(|| println!("running: {command:?}")); - let output_mode = command.output_mode.unwrap_or_else(|| match self.is_verbose() { - true => OutputMode::All, - false => OutputMode::OnlyOutput, - }); - let (output, print_error): (io::Result, bool) = match output_mode { - mode @ (OutputMode::All | OutputMode::OnlyOutput) => ( - command.command.status().map(|status| status.into()), - matches!(mode, OutputMode::All), - ), - mode @ (OutputMode::OnlyOnFailure | OutputMode::Quiet) => ( - command.command.output().map(|o| o.into()), - matches!(mode, OutputMode::OnlyOnFailure), - ), + let output: io::Result = match command.output_mode { + OutputMode::Print => command.command.status().map(|status| status.into()), + OutputMode::CaptureAll => command.command.output().map(|o| o.into()), + OutputMode::CaptureStdout => { + command.command.stderr(Stdio::inherit()); + command.command.output().map(|o| o.into()) + } }; let output = match output { Ok(output) => output, - Err(e) => fail(&format!("failed to execute command: {:?}\nerror: {}", command, e)), + Err(e) => fail(&format!("failed to execute command: {command:?}\nerror: {e}")), }; if !output.is_success() { - if print_error { - println!( - "\n\nCommand did not execute successfully.\ - \nExpected success, got: {}", - output.status(), - ); + use std::fmt::Write; + + // Here we build an error message, and below we decide if it should be printed or not. + let mut message = String::new(); + writeln!( + message, + "\n\nCommand {command:?} did not execute successfully.\ + \nExpected success, got: {}", + output.status(), + ) + .unwrap(); - if !self.is_verbose() { - println!("Add `-v` to see more details.\n"); - } + // If the output mode is OutputMode::Print, the output has already been printed to + // stdout/stderr, and we thus don't have anything captured to print anyway. + if matches!(command.output_mode, OutputMode::CaptureAll | OutputMode::CaptureStdout) { + writeln!(message, "\nSTDOUT ----\n{}", output.stdout().trim()).unwrap(); - self.verbose(|| { - println!( - "\nSTDOUT ----\n{}\n\ - STDERR ----\n{}\n", - output.stdout(), - output.stderr(), - ) - }); + // Stderr is added to the message only if it was captured + if matches!(command.output_mode, OutputMode::CaptureAll) { + writeln!(message, "\nSTDERR ----\n{}", output.stderr().trim()).unwrap(); + } } match command.failure_behavior { BehaviorOnFailure::DelayFail => { if self.fail_fast { + println!("{message}"); exit!(1); } let mut failures = self.delayed_failures.borrow_mut(); - failures.push(format!("{command:?}")); + failures.push(message); } BehaviorOnFailure::Exit => { + println!("{message}"); exit!(1); } - BehaviorOnFailure::Ignore => {} + BehaviorOnFailure::Ignore => { + // If failures are allowed, either the error has been printed already + // (OutputMode::Print) or the user used a capture output mode and wants to + // handle the error output on their own. + } } } output @@ -1490,7 +1489,7 @@ impl Build { // (Note that we use a `..` range, not the `...` symmetric difference.) self.run( helpers::git(Some(&self.src)) - .quiet() + .capture() .arg("rev-list") .arg("--count") .arg("--merges") diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index dc30876601c6b..5f9e164816179 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -13,18 +13,19 @@ pub enum BehaviorOnFailure { Ignore, } -/// How should the output of the command be handled. +/// How should the output of the command be handled (whether it should be captured or printed). #[derive(Debug, Copy, Clone)] pub enum OutputMode { - /// Print both the output (by inheriting stdout/stderr) and also the command itself, if it - /// fails. - All, - /// Print the output (by inheriting stdout/stderr). - OnlyOutput, - /// Suppress the output if the command succeeds, otherwise print the output. - OnlyOnFailure, - /// Suppress the output of the command. - Quiet, + /// Prints the stdout/stderr of the command to stdout/stderr of bootstrap (by inheriting these + /// streams). + /// Corresponds to calling `cmd.status()`. + Print, + /// Captures the stdout and stderr of the command into memory. + /// Corresponds to calling `cmd.output()`. + CaptureAll, + /// Captures the stdout of the command into memory, inherits stderr. + /// Corresponds to calling `cmd.output()`. + CaptureStdout, } /// Wrapper around `std::process::Command`. @@ -34,10 +35,10 @@ pub enum OutputMode { /// If you want to delay failures until the end of bootstrap, use [delay_failure]. /// /// By default, the command will print its stdout/stderr to stdout/stderr of bootstrap -/// ([OutputMode::OnlyOutput]). If bootstrap uses verbose mode, then it will also print the -/// command itself in case of failure ([OutputMode::All]). -/// If you want to handle the output programmatically, use `output_mode(OutputMode::OnlyOnFailure)`, -/// which will print the output only if the command fails. +/// ([OutputMode::Print]). +/// If you want to handle the output programmatically, use [BootstrapCommand::capture]. +/// +/// Bootstrap will print a debug log to stdout if the command fails and failure is not allowed. /// /// [allow_failure]: BootstrapCommand::allow_failure /// [delay_failure]: BootstrapCommand::delay_failure @@ -45,12 +46,16 @@ pub enum OutputMode { pub struct BootstrapCommand { pub command: Command, pub failure_behavior: BehaviorOnFailure, - pub output_mode: Option, + pub output_mode: OutputMode, } impl BootstrapCommand { pub fn new>(program: S) -> Self { - Command::new(program).into() + Self { + command: Command::new(program), + failure_behavior: BehaviorOnFailure::Exit, + output_mode: OutputMode::Print, + } } pub fn arg>(&mut self, arg: S) -> &mut Self { @@ -106,52 +111,22 @@ impl BootstrapCommand { Self { failure_behavior: BehaviorOnFailure::Ignore, ..self } } - /// Do not print the output of the command, unless it fails. - pub fn print_on_failure(self) -> Self { - self.output_mode(OutputMode::OnlyOnFailure) - } - - /// Do not print the output of the command. - pub fn quiet(self) -> Self { - self.output_mode(OutputMode::Quiet) + /// Capture the output of the command, do not print it. + pub fn capture(self) -> Self { + Self { output_mode: OutputMode::CaptureAll, ..self } } - pub fn output_mode(self, output_mode: OutputMode) -> Self { - Self { output_mode: Some(output_mode), ..self } + /// Capture stdout of the command, do not print it. + pub fn capture_stdout(self) -> Self { + Self { output_mode: OutputMode::CaptureStdout, ..self } } } -/// FIXME: This implementation is temporary, until all `Command` invocations are migrated to -/// `BootstrapCommand`. -impl<'a> From<&'a mut BootstrapCommand> for BootstrapCommand { - fn from(command: &'a mut BootstrapCommand) -> Self { - // This is essentially a manual `Command::clone` - let mut cmd = Command::new(command.command.get_program()); - if let Some(dir) = command.command.get_current_dir() { - cmd.current_dir(dir); - } - cmd.args(command.command.get_args()); - for (key, value) in command.command.get_envs() { - match value { - Some(value) => { - cmd.env(key, value); - } - None => { - cmd.env_remove(key); - } - } - } - Self { - command: cmd, - output_mode: command.output_mode, - failure_behavior: command.failure_behavior, - } - } -} - -impl From for BootstrapCommand { - fn from(command: Command) -> Self { - Self { command, failure_behavior: BehaviorOnFailure::Exit, output_mode: None } +/// This implementation exists to make it possible to pass both [BootstrapCommand] and +/// `&mut BootstrapCommand` to `Build.run()`. +impl AsMut for BootstrapCommand { + fn as_mut(&mut self) -> &mut BootstrapCommand { + self } } @@ -176,6 +151,10 @@ impl CommandOutput { String::from_utf8(self.0.stdout.clone()).expect("Cannot parse process stdout as UTF-8") } + pub fn stdout_if_ok(&self) -> Option { + if self.is_success() { Some(self.stdout()) } else { None } + } + pub fn stderr(&self) -> String { String::from_utf8(self.0.stderr.clone()).expect("Cannot parse process stderr as UTF-8") } From 3ef77cc42cb1ea1f07c1bd8c2ac514d15291106b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 22 Jun 2024 17:47:11 +0200 Subject: [PATCH 04/11] Remove various usages of the `output` function --- src/bootstrap/src/core/build_steps/compile.rs | 13 ++-- src/bootstrap/src/core/build_steps/dist.rs | 19 ++--- src/bootstrap/src/core/build_steps/format.rs | 33 ++++----- src/bootstrap/src/core/build_steps/llvm.rs | 14 ++-- src/bootstrap/src/core/build_steps/run.rs | 6 +- src/bootstrap/src/core/build_steps/suggest.rs | 25 +++---- .../src/core/build_steps/synthetic_targets.rs | 14 ++-- src/bootstrap/src/core/build_steps/test.rs | 70 ++++++++----------- src/bootstrap/src/core/build_steps/tool.rs | 5 +- src/bootstrap/src/core/builder.rs | 7 +- src/bootstrap/src/core/metadata.rs | 8 +-- src/bootstrap/src/core/sanity.rs | 4 +- src/bootstrap/src/lib.rs | 6 +- src/bootstrap/src/utils/cc_detect.rs | 8 +-- src/bootstrap/src/utils/exec.rs | 12 ++-- src/bootstrap/src/utils/helpers.rs | 8 ++- 16 files changed, 120 insertions(+), 132 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs index c267d3b0c0cf6..01b25224de4cd 100644 --- a/src/bootstrap/src/core/build_steps/compile.rs +++ b/src/bootstrap/src/core/build_steps/compile.rs @@ -29,7 +29,7 @@ use crate::core::builder::{Builder, Kind, PathSet, RunConfig, ShouldRun, Step, T use crate::core::config::{DebuginfoLevel, LlvmLibunwind, RustcLto, TargetSelection}; use crate::utils::exec::BootstrapCommand; use crate::utils::helpers::{ - exe, get_clang_cl_resource_dir, is_debug_info, is_dylib, output, symlink_dir, t, up_to_date, + exe, get_clang_cl_resource_dir, is_debug_info, is_dylib, symlink_dir, t, up_to_date, }; use crate::LLVM_TOOLS; use crate::{CLang, Compiler, DependencyType, GitRepo, Mode}; @@ -1488,10 +1488,10 @@ pub fn compiler_file( if builder.config.dry_run() { return PathBuf::new(); } - let mut cmd = Command::new(compiler); + let mut cmd = BootstrapCommand::new(compiler); cmd.args(builder.cflags(target, GitRepo::Rustc, c)); cmd.arg(format!("-print-file-name={file}")); - let out = output(&mut cmd); + let out = builder.run(cmd.capture_stdout()).stdout(); PathBuf::from(out.trim()) } @@ -1836,7 +1836,9 @@ impl Step for Assemble { let llvm::LlvmResult { llvm_config, .. } = builder.ensure(llvm::Llvm { target: target_compiler.host }); if !builder.config.dry_run() && builder.config.llvm_tools_enabled { - let llvm_bin_dir = output(Command::new(llvm_config).arg("--bindir")); + let llvm_bin_dir = builder + .run(BootstrapCommand::new(llvm_config).capture_stdout().arg("--bindir")) + .stdout(); let llvm_bin_dir = Path::new(llvm_bin_dir.trim()); // Since we've already built the LLVM tools, install them to the sysroot. @@ -2161,8 +2163,7 @@ pub fn strip_debug(builder: &Builder<'_>, target: TargetSelection, path: &Path) } let previous_mtime = FileTime::from_last_modification_time(&path.metadata().unwrap()); - // NOTE: `output` will propagate any errors here. - output(Command::new("strip").arg("--strip-debug").arg(path)); + builder.run(BootstrapCommand::new("strip").capture().arg("--strip-debug").arg(path)); // After running `strip`, we have to set the file modification time to what it was before, // otherwise we risk Cargo invalidating its fingerprint and rebuilding the world next time diff --git a/src/bootstrap/src/core/build_steps/dist.rs b/src/bootstrap/src/core/build_steps/dist.rs index 08795efe5bb4c..3dc871b1ca9dd 100644 --- a/src/bootstrap/src/core/build_steps/dist.rs +++ b/src/bootstrap/src/core/build_steps/dist.rs @@ -14,7 +14,6 @@ use std::ffi::OsStr; use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; -use std::process::Command; use object::read::archive::ArchiveFile; use object::BinaryFormat; @@ -28,7 +27,7 @@ use crate::core::config::TargetSelection; use crate::utils::channel::{self, Info}; use crate::utils::exec::BootstrapCommand; use crate::utils::helpers::{ - exe, is_dylib, move_file, output, t, target_supports_cranelift_backend, timeit, + exe, is_dylib, move_file, t, target_supports_cranelift_backend, timeit, }; use crate::utils::tarball::{GeneratedTarball, OverlayKind, Tarball}; use crate::{Compiler, DependencyType, Mode, LLVM_TOOLS}; @@ -181,9 +180,9 @@ fn make_win_dist( } //Ask gcc where it keeps its stuff - let mut cmd = Command::new(builder.cc(target)); + let mut cmd = BootstrapCommand::new(builder.cc(target)); cmd.arg("-print-search-dirs"); - let gcc_out = output(&mut cmd); + let gcc_out = builder.run(cmd.capture_stdout()).stdout(); let mut bin_path: Vec<_> = env::split_paths(&env::var_os("PATH").unwrap_or_default()).collect(); let mut lib_path = Vec::new(); @@ -1024,7 +1023,7 @@ impl Step for PlainSourceTarball { } // Vendor all Cargo dependencies - let mut cmd = Command::new(&builder.initial_cargo); + let mut cmd = BootstrapCommand::new(&builder.initial_cargo); cmd.arg("vendor") .arg("--versioned-dirs") .arg("--sync") @@ -1062,7 +1061,7 @@ impl Step for PlainSourceTarball { } let config = if !builder.config.dry_run() { - t!(String::from_utf8(t!(cmd.output()).stdout)) + builder.run(cmd.capture()).stdout() } else { String::new() }; @@ -2079,10 +2078,14 @@ fn maybe_install_llvm( } else if let llvm::LlvmBuildStatus::AlreadyBuilt(llvm::LlvmResult { llvm_config, .. }) = llvm::prebuilt_llvm_config(builder, target) { - let mut cmd = Command::new(llvm_config); + let mut cmd = BootstrapCommand::new(llvm_config); cmd.arg("--libfiles"); builder.verbose(|| println!("running {cmd:?}")); - let files = if builder.config.dry_run() { "".into() } else { output(&mut cmd) }; + let files = if builder.config.dry_run() { + "".into() + } else { + builder.run(cmd.capture_stdout()).stdout() + }; let build_llvm_out = &builder.llvm_out(builder.config.build); let target_llvm_out = &builder.llvm_out(target); for file in files.trim_end().split(' ') { diff --git a/src/bootstrap/src/core/build_steps/format.rs b/src/bootstrap/src/core/build_steps/format.rs index 0372360c3cb74..66286a52813f4 100644 --- a/src/bootstrap/src/core/build_steps/format.rs +++ b/src/bootstrap/src/core/build_steps/format.rs @@ -1,7 +1,8 @@ //! Runs rustfmt on the repository. use crate::core::builder::Builder; -use crate::utils::helpers::{self, output, program_out_of_date, t}; +use crate::utils::exec::BootstrapCommand; +use crate::utils::helpers::{self, program_out_of_date, t}; use build_helper::ci::CiEnv; use build_helper::git::get_git_modified_files; use ignore::WalkBuilder; @@ -53,19 +54,17 @@ fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl F fn get_rustfmt_version(build: &Builder<'_>) -> Option<(String, PathBuf)> { let stamp_file = build.out.join("rustfmt.stamp"); - let mut cmd = Command::new(match build.initial_rustfmt() { + let mut cmd = BootstrapCommand::new(match build.initial_rustfmt() { Some(p) => p, None => return None, }); cmd.arg("--version"); - let output = match cmd.output() { - Ok(status) => status, - Err(_) => return None, - }; - if !output.status.success() { + + let output = build.run(cmd.capture().allow_failure()); + if output.is_failure() { return None; } - Some((String::from_utf8(output.stdout).unwrap(), stamp_file)) + Some((output.stdout(), stamp_file)) } /// Return whether the format cache can be reused. @@ -175,14 +174,16 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) { ) .is_success(); if in_working_tree { - let untracked_paths_output = output( - &mut helpers::git(Some(&build.src)) - .arg("status") - .arg("--porcelain") - .arg("-z") - .arg("--untracked-files=normal") - .command, - ); + let untracked_paths_output = build + .run( + helpers::git(Some(&build.src)) + .capture_stdout() + .arg("status") + .arg("--porcelain") + .arg("-z") + .arg("--untracked-files=normal"), + ) + .stdout(); let untracked_paths: Vec<_> = untracked_paths_output .split_terminator('\0') .filter_map( diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index ba21e2ad32d05..4f1c1f87d1c6c 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -14,7 +14,6 @@ use std::ffi::{OsStr, OsString}; use std::fs::{self, File}; use std::io; use std::path::{Path, PathBuf}; -use std::process::Command; use std::sync::OnceLock; use crate::core::builder::{Builder, RunConfig, ShouldRun, Step}; @@ -25,6 +24,7 @@ use crate::utils::helpers::{ }; use crate::{generate_smart_stamp_hash, CLang, GitRepo, Kind}; +use crate::utils::exec::BootstrapCommand; use build_helper::ci::CiEnv; use build_helper::git::get_git_merge_base; @@ -478,7 +478,9 @@ impl Step for Llvm { let LlvmResult { llvm_config, .. } = builder.ensure(Llvm { target: builder.config.build }); if !builder.config.dry_run() { - let llvm_bindir = output(Command::new(&llvm_config).arg("--bindir")); + let llvm_bindir = builder + .run(BootstrapCommand::new(&llvm_config).capture_stdout().arg("--bindir")) + .stdout(); let host_bin = Path::new(llvm_bindir.trim()); cfg.define( "LLVM_TABLEGEN", @@ -528,8 +530,8 @@ impl Step for Llvm { // Helper to find the name of LLVM's shared library on darwin and linux. let find_llvm_lib_name = |extension| { - let mut cmd = Command::new(&res.llvm_config); - let version = output(cmd.arg("--version")); + let cmd = BootstrapCommand::new(&res.llvm_config); + let version = builder.run(cmd.capture_stdout().arg("--version")).stdout(); let major = version.split('.').next().unwrap(); match &llvm_version_suffix { @@ -585,8 +587,8 @@ fn check_llvm_version(builder: &Builder<'_>, llvm_config: &Path) { return; } - let mut cmd = Command::new(llvm_config); - let version = output(cmd.arg("--version")); + let cmd = BootstrapCommand::new(llvm_config); + let version = builder.run(cmd.capture_stdout().arg("--version")).stdout(); let mut parts = version.split('.').take(2).filter_map(|s| s.parse::().ok()); if let (Some(major), Some(_minor)) = (parts.next(), parts.next()) { if major >= 17 { diff --git a/src/bootstrap/src/core/build_steps/run.rs b/src/bootstrap/src/core/build_steps/run.rs index 496fe446d724e..316a03a2171b5 100644 --- a/src/bootstrap/src/core/build_steps/run.rs +++ b/src/bootstrap/src/core/build_steps/run.rs @@ -4,7 +4,6 @@ //! If it can be reached from `./x.py run` it can go here. use std::path::PathBuf; -use std::process::Command; use crate::core::build_steps::dist::distdir; use crate::core::build_steps::test; @@ -12,7 +11,7 @@ use crate::core::build_steps::tool::{self, SourceType, Tool}; use crate::core::builder::{Builder, RunConfig, ShouldRun, Step}; use crate::core::config::flags::get_completion; use crate::core::config::TargetSelection; -use crate::utils::helpers::output; +use crate::utils::exec::BootstrapCommand; use crate::Mode; #[derive(Debug, PartialOrd, Ord, Clone, Hash, PartialEq, Eq)] @@ -41,7 +40,8 @@ impl Step for BuildManifest { panic!("\n\nfailed to specify `dist.upload-addr` in `config.toml`\n\n") }); - let today = output(Command::new("date").arg("+%Y-%m-%d")); + let today = + builder.run(BootstrapCommand::new("date").capture_stdout().arg("+%Y-%m-%d")).stdout(); cmd.arg(sign); cmd.arg(distdir(builder)); diff --git a/src/bootstrap/src/core/build_steps/suggest.rs b/src/bootstrap/src/core/build_steps/suggest.rs index 7c5e0d4e13ebb..5412b3c807b62 100644 --- a/src/bootstrap/src/core/build_steps/suggest.rs +++ b/src/bootstrap/src/core/build_steps/suggest.rs @@ -13,24 +13,15 @@ use crate::core::builder::Builder; pub fn suggest(builder: &Builder<'_>, run: bool) { let git_config = builder.config.git_config(); let suggestions = builder - .tool_cmd(Tool::SuggestTests) - .env("SUGGEST_TESTS_GIT_REPOSITORY", git_config.git_repository) - .env("SUGGEST_TESTS_NIGHTLY_BRANCH", git_config.nightly_branch) - .command - .output() - .expect("failed to run `suggest-tests` tool"); + .run( + builder + .tool_cmd(Tool::SuggestTests) + .capture_stdout() + .env("SUGGEST_TESTS_GIT_REPOSITORY", git_config.git_repository) + .env("SUGGEST_TESTS_NIGHTLY_BRANCH", git_config.nightly_branch), + ) + .stdout(); - if !suggestions.status.success() { - println!("failed to run `suggest-tests` tool ({})", suggestions.status); - println!( - "`suggest_tests` stdout:\n{}`suggest_tests` stderr:\n{}", - String::from_utf8(suggestions.stdout).unwrap(), - String::from_utf8(suggestions.stderr).unwrap() - ); - panic!("failed to run `suggest-tests`"); - } - - let suggestions = String::from_utf8(suggestions.stdout).unwrap(); let suggestions = suggestions .lines() .map(|line| { diff --git a/src/bootstrap/src/core/build_steps/synthetic_targets.rs b/src/bootstrap/src/core/build_steps/synthetic_targets.rs index 281a9b093b90b..a115ba2d8b27a 100644 --- a/src/bootstrap/src/core/build_steps/synthetic_targets.rs +++ b/src/bootstrap/src/core/build_steps/synthetic_targets.rs @@ -9,8 +9,8 @@ use crate::core::builder::{Builder, ShouldRun, Step}; use crate::core::config::TargetSelection; +use crate::utils::exec::BootstrapCommand; use crate::Compiler; -use std::process::{Command, Stdio}; #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub(crate) struct MirOptPanicAbortSyntheticTarget { @@ -56,7 +56,7 @@ fn create_synthetic_target( return TargetSelection::create_synthetic(&name, path.to_str().unwrap()); } - let mut cmd = Command::new(builder.rustc(compiler)); + let mut cmd = BootstrapCommand::new(builder.rustc(compiler)); cmd.arg("--target").arg(base.rustc_target_arg()); cmd.args(["-Zunstable-options", "--print", "target-spec-json"]); @@ -64,14 +64,8 @@ fn create_synthetic_target( // we cannot use nightly features. So `RUSTC_BOOTSTRAP` is needed here. cmd.env("RUSTC_BOOTSTRAP", "1"); - cmd.stdout(Stdio::piped()); - - let output = cmd.spawn().unwrap().wait_with_output().unwrap(); - if !output.status.success() { - panic!("failed to gather the target spec for {base}"); - } - - let mut spec: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + let output = builder.run(cmd.capture()).stdout(); + let mut spec: serde_json::Value = serde_json::from_slice(output.as_bytes()).unwrap(); let spec_map = spec.as_object_mut().unwrap(); // The `is-builtin` attribute of a spec needs to be removed, otherwise rustc will complain. diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 9fc337dfd50db..b063d0aaf4f16 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -29,8 +29,7 @@ use crate::core::config::TargetSelection; use crate::utils::exec::BootstrapCommand; use crate::utils::helpers::{ self, add_link_lib_path, add_rustdoc_cargo_linker_args, dylib_path, dylib_path_var, - linker_args, linker_flags, output, t, target_supports_cranelift_backend, up_to_date, - LldThreads, + linker_args, linker_flags, t, target_supports_cranelift_backend, up_to_date, LldThreads, }; use crate::utils::render_tests::{add_flags_and_try_run_tests, try_run_tests}; use crate::{envify, CLang, DocTests, GitRepo, Mode}; @@ -470,19 +469,12 @@ impl Miri { // We re-use the `cargo` from above. cargo.arg("--print-sysroot"); - // FIXME: Is there a way in which we can re-use the usual `run` helpers? if builder.config.dry_run() { String::new() } else { builder.verbose(|| println!("running: {cargo:?}")); - let out = cargo - .command - .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"); + let stdout = builder.run(cargo.capture_stdout()).stdout(); // Output is "\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(|| println!("`cargo miri setup --print-sysroot` said: {sysroot:?}")); sysroot.to_owned() @@ -911,25 +903,26 @@ impl Step for RustdocJSNotStd { } } -fn get_browser_ui_test_version_inner(npm: &Path, global: bool) -> Option { - let mut command = Command::new(npm); +fn get_browser_ui_test_version_inner( + builder: &Builder<'_>, + npm: &Path, + global: bool, +) -> Option { + let mut command = BootstrapCommand::new(npm).capture(); command.arg("list").arg("--parseable").arg("--long").arg("--depth=0"); if global { command.arg("--global"); } - let lines = command - .output() - .map(|output| String::from_utf8_lossy(&output.stdout).into_owned()) - .unwrap_or_default(); + let lines = builder.run(command.allow_failure()).stdout(); lines .lines() .find_map(|l| l.split(':').nth(1)?.strip_prefix("browser-ui-test@")) .map(|v| v.to_owned()) } -fn get_browser_ui_test_version(npm: &Path) -> Option { - get_browser_ui_test_version_inner(npm, false) - .or_else(|| get_browser_ui_test_version_inner(npm, true)) +fn get_browser_ui_test_version(builder: &Builder<'_>, npm: &Path) -> Option { + get_browser_ui_test_version_inner(builder, npm, false) + .or_else(|| get_browser_ui_test_version_inner(builder, npm, true)) } #[derive(Debug, Clone, Hash, PartialEq, Eq)] @@ -953,7 +946,7 @@ impl Step for RustdocGUI { .config .npm .as_ref() - .map(|p| get_browser_ui_test_version(p).is_some()) + .map(|p| get_browser_ui_test_version(builder, p).is_some()) .unwrap_or(false) })) } @@ -1811,26 +1804,15 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the } let lldb_exe = builder.config.lldb.clone().unwrap_or_else(|| PathBuf::from("lldb")); - let lldb_version = Command::new(&lldb_exe) - .arg("--version") - .output() - .map(|output| { - (String::from_utf8_lossy(&output.stdout).to_string(), output.status.success()) - }) - .ok() - .and_then(|(output, success)| if success { Some(output) } else { None }); + let lldb_version = builder + .run(BootstrapCommand::new(&lldb_exe).capture().allow_failure().arg("--version")) + .stdout_if_ok(); if let Some(ref vers) = lldb_version { - let run = |cmd: &mut Command| { - cmd.output().map(|output| { - String::from_utf8_lossy(&output.stdout) - .lines() - .next() - .unwrap_or_else(|| panic!("{:?} failed {:?}", cmd, output)) - .to_string() - }) - }; cmd.arg("--lldb-version").arg(vers); - let lldb_python_dir = run(Command::new(&lldb_exe).arg("-P")).ok(); + let lldb_python_dir = builder + .run(BootstrapCommand::new(&lldb_exe).allow_failure().capture_stdout().arg("-P")) + .stdout_if_ok() + .map(|p| p.lines().next().expect("lldb Python dir not found").to_string()); if let Some(ref dir) = lldb_python_dir { cmd.arg("--lldb-python-dir").arg(dir); } @@ -1886,8 +1868,12 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the let llvm::LlvmResult { llvm_config, .. } = builder.ensure(llvm::Llvm { target: builder.config.build }); if !builder.config.dry_run() { - let llvm_version = output(Command::new(&llvm_config).arg("--version")); - let llvm_components = output(Command::new(&llvm_config).arg("--components")); + let llvm_version = builder + .run(BootstrapCommand::new(&llvm_config).capture_stdout().arg("--version")) + .stdout(); + let llvm_components = builder + .run(BootstrapCommand::new(&llvm_config).capture_stdout().arg("--components")) + .stdout(); // Remove trailing newline from llvm-config output. cmd.arg("--llvm-version") .arg(llvm_version.trim()) @@ -1906,7 +1892,9 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the // separate compilations. We can add LLVM's library path to the // platform-specific environment variable as a workaround. if !builder.config.dry_run() && suite.ends_with("fulldeps") { - let llvm_libdir = output(Command::new(&llvm_config).arg("--libdir")); + let llvm_libdir = builder + .run(BootstrapCommand::new(&llvm_config).capture_stdout().arg("--libdir")) + .stdout(); add_link_lib_path(vec![llvm_libdir.trim().into()], &mut cmd); } diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index 20c7a30f1a823..40184e016a634 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -1,7 +1,6 @@ use std::env; use std::fs; use std::path::{Path, PathBuf}; -use std::process::Command; use crate::core::build_steps::compile; use crate::core::build_steps::toolstate::ToolState; @@ -10,7 +9,6 @@ use crate::core::builder::{Builder, Cargo as CargoCommand, RunConfig, ShouldRun, use crate::core::config::TargetSelection; use crate::utils::channel::GitInfo; use crate::utils::exec::BootstrapCommand; -use crate::utils::helpers::output; use crate::utils::helpers::{add_dylib_path, exe, t}; use crate::Compiler; use crate::Mode; @@ -926,7 +924,8 @@ impl Step for LibcxxVersionTool { } } - let version_output = output(&mut Command::new(executable)); + let version_output = + builder.run(BootstrapCommand::new(executable).capture_stdout()).stdout(); let version_str = version_output.split_once("version:").unwrap().1; let version = version_str.trim().parse::().unwrap(); diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs index 35de91c41d4f1..4da912994c3ee 100644 --- a/src/bootstrap/src/core/builder.rs +++ b/src/bootstrap/src/core/builder.rs @@ -8,7 +8,6 @@ use std::fs; use std::hash::Hash; use std::ops::Deref; use std::path::{Path, PathBuf}; -use std::process::Command; use std::time::{Duration, Instant}; use crate::core::build_steps::tool::{self, SourceType}; @@ -20,7 +19,7 @@ use crate::core::config::{DryRun, SplitDebuginfo, TargetSelection}; use crate::prepare_behaviour_dump_dir; use crate::utils::cache::Cache; use crate::utils::helpers::{self, add_dylib_path, add_link_lib_path, exe, linker_args}; -use crate::utils::helpers::{check_cfg_arg, libdir, linker_flags, output, t, LldThreads}; +use crate::utils::helpers::{check_cfg_arg, libdir, linker_flags, t, LldThreads}; use crate::EXTRA_CHECK_CFGS; use crate::{Build, CLang, Crate, DocTests, GitRepo, Mode}; @@ -1919,7 +1918,9 @@ impl<'a> Builder<'a> { // platform-specific environment variable as a workaround. if mode == Mode::ToolRustc || mode == Mode::Codegen { if let Some(llvm_config) = self.llvm_config(target) { - let llvm_libdir = output(Command::new(llvm_config).arg("--libdir")); + let llvm_libdir = self + .run(BootstrapCommand::new(llvm_config).capture_stdout().arg("--libdir")) + .stdout(); add_link_lib_path(vec![llvm_libdir.trim().into()], &mut cargo); } } diff --git a/src/bootstrap/src/core/metadata.rs b/src/bootstrap/src/core/metadata.rs index 220eb5ba126e4..da269959e7b43 100644 --- a/src/bootstrap/src/core/metadata.rs +++ b/src/bootstrap/src/core/metadata.rs @@ -1,9 +1,8 @@ use std::path::PathBuf; -use std::process::Command; use serde_derive::Deserialize; -use crate::utils::helpers::output; +use crate::utils::exec::BootstrapCommand; use crate::{t, Build, Crate}; /// For more information, see the output of @@ -71,7 +70,7 @@ pub fn build(build: &mut Build) { /// particular crate (e.g., `x build sysroot` to build library/sysroot). fn workspace_members(build: &Build) -> Vec { let collect_metadata = |manifest_path| { - let mut cargo = Command::new(&build.initial_cargo); + let mut cargo = BootstrapCommand::new(&build.initial_cargo); cargo // Will read the libstd Cargo.toml // which uses the unstable `public-dependency` feature. @@ -82,7 +81,8 @@ fn workspace_members(build: &Build) -> Vec { .arg("--no-deps") .arg("--manifest-path") .arg(build.src.join(manifest_path)); - let metadata_output = output(&mut cargo); + // FIXME: fix stderr + let metadata_output = build.run(cargo.capture()).stdout(); let Output { packages, .. } = t!(serde_json::from_str(&metadata_output)); packages }; diff --git a/src/bootstrap/src/core/sanity.rs b/src/bootstrap/src/core/sanity.rs index 5a0be2948a19e..d64923e69cbd0 100644 --- a/src/bootstrap/src/core/sanity.rs +++ b/src/bootstrap/src/core/sanity.rs @@ -24,6 +24,7 @@ use std::collections::HashSet; use crate::builder::Kind; use crate::core::config::Target; +use crate::utils::exec::BootstrapCommand; use crate::utils::helpers::output; use crate::Build; @@ -352,7 +353,8 @@ than building it. // There are three builds of cmake on windows: MSVC, MinGW, and // Cygwin. The Cygwin build does not have generators for Visual // Studio, so detect that here and error. - let out = output(Command::new("cmake").arg("--help")); + let out = + build.run(BootstrapCommand::new("cmake").capture_stdout().arg("--help")).stdout(); if !out.contains("Visual Studio") { panic!( " diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index ae3d18e8774bc..dfaa1e5eb1294 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -861,14 +861,16 @@ impl Build { if let Some(s) = target_config.and_then(|c| c.llvm_filecheck.as_ref()) { s.to_path_buf() } else if let Some(s) = target_config.and_then(|c| c.llvm_config.as_ref()) { - let llvm_bindir = output(Command::new(s).arg("--bindir")); + let llvm_bindir = + self.run(BootstrapCommand::new(s).capture_stdout().arg("--bindir")).stdout(); let filecheck = Path::new(llvm_bindir.trim()).join(exe("FileCheck", target)); if filecheck.exists() { filecheck } else { // On Fedora the system LLVM installs FileCheck in the // llvm subdirectory of the libdir. - let llvm_libdir = output(Command::new(s).arg("--libdir")); + let llvm_libdir = + self.run(BootstrapCommand::new(s).capture_stdout().arg("--libdir")).stdout(); let lib_filecheck = Path::new(llvm_libdir.trim()).join("llvm").join(exe("FileCheck", target)); if lib_filecheck.exists() { diff --git a/src/bootstrap/src/utils/cc_detect.rs b/src/bootstrap/src/utils/cc_detect.rs index 540b8671333a9..b80df54520264 100644 --- a/src/bootstrap/src/utils/cc_detect.rs +++ b/src/bootstrap/src/utils/cc_detect.rs @@ -23,11 +23,10 @@ use std::collections::HashSet; use std::path::{Path, PathBuf}; -use std::process::Command; use std::{env, iter}; use crate::core::config::TargetSelection; -use crate::utils::helpers::output; +use crate::utils::exec::BootstrapCommand; use crate::{Build, CLang, GitRepo}; // The `cc` crate doesn't provide a way to obtain a path to the detected archiver, @@ -183,14 +182,15 @@ fn default_compiler( return None; } - let output = output(c.to_command().arg("--version")); + let cmd = BootstrapCommand::from(c.to_command()); + let output = build.run(cmd.capture_stdout().arg("--version")).stdout(); let i = output.find(" 4.")?; match output[i + 3..].chars().next().unwrap() { '0'..='6' => {} _ => return None, } let alternative = format!("e{gnu_compiler}"); - if Command::new(&alternative).output().is_ok() { + if build.run(BootstrapCommand::new(&alternative).capture()).is_success() { Some(PathBuf::from(alternative)) } else { None diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index 5f9e164816179..bda6e0bfbac02 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -51,11 +51,7 @@ pub struct BootstrapCommand { impl BootstrapCommand { pub fn new>(program: S) -> Self { - Self { - command: Command::new(program), - failure_behavior: BehaviorOnFailure::Exit, - output_mode: OutputMode::Print, - } + Command::new(program).into() } pub fn arg>(&mut self, arg: S) -> &mut Self { @@ -130,6 +126,12 @@ impl AsMut for BootstrapCommand { } } +impl From for BootstrapCommand { + fn from(command: Command) -> Self { + Self { command, failure_behavior: BehaviorOnFailure::Exit, output_mode: OutputMode::Print } + } +} + /// Represents the output of an executed process. #[allow(unused)] pub struct CommandOutput(Output); diff --git a/src/bootstrap/src/utils/helpers.rs b/src/bootstrap/src/utils/helpers.rs index 9c40065dfc4fa..bbd4185874f4f 100644 --- a/src/bootstrap/src/utils/helpers.rs +++ b/src/bootstrap/src/utils/helpers.rs @@ -360,7 +360,7 @@ pub fn get_clang_cl_resource_dir(clang_cl_path: &str) -> PathBuf { /// Returns a flag that configures LLD to use only a single thread. /// If we use an external LLD, we need to find out which version is it to know which flag should we /// pass to it (LLD older than version 10 had a different flag). -fn lld_flag_no_threads(lld_mode: LldMode, is_windows: bool) -> &'static str { +fn lld_flag_no_threads(builder: &Builder<'_>, lld_mode: LldMode, is_windows: bool) -> &'static str { static LLD_NO_THREADS: OnceLock<(&'static str, &'static str)> = OnceLock::new(); let new_flags = ("/threads:1", "--threads=1"); @@ -369,7 +369,9 @@ fn lld_flag_no_threads(lld_mode: LldMode, is_windows: bool) -> &'static str { let (windows_flag, other_flag) = LLD_NO_THREADS.get_or_init(|| { let newer_version = match lld_mode { LldMode::External => { - let out = output(Command::new("lld").arg("-flavor").arg("ld").arg("--version")); + let mut cmd = BootstrapCommand::new("lld").capture_stdout(); + cmd.arg("-flavor").arg("ld").arg("--version"); + let out = builder.run(cmd).stdout(); match (out.find(char::is_numeric), out.find('.')) { (Some(b), Some(e)) => out.as_str()[b..e].parse::().ok().unwrap_or(14) > 10, _ => true, @@ -431,7 +433,7 @@ pub fn linker_flags( if matches!(lld_threads, LldThreads::No) { args.push(format!( "-Clink-arg=-Wl,{}", - lld_flag_no_threads(builder.config.lld_mode, target.is_windows()) + lld_flag_no_threads(builder, builder.config.lld_mode, target.is_windows()) )); } } From e8c8860142a999d086efe579e8e17df8437a9730 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 22 Jun 2024 17:54:44 +0200 Subject: [PATCH 05/11] Allow unused `Tool` variants --- src/bootstrap/src/core/build_steps/tool.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/bootstrap/src/core/build_steps/tool.rs b/src/bootstrap/src/core/build_steps/tool.rs index 40184e016a634..5fb282db90a7c 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -244,6 +244,7 @@ macro_rules! bootstrap_tool { ; )+) => { #[derive(PartialEq, Eq, Clone)] + #[allow(dead_code)] pub enum Tool { $( $name, From 60c20bfe0c6e34d36d9a40d835ce6946a10f0238 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 29 Jun 2024 16:00:23 +0200 Subject: [PATCH 06/11] Refactor command outcome handling To handle the case of failing to start a `BootstrapCommand`. --- src/bootstrap/src/core/sanity.rs | 15 ++++--- src/bootstrap/src/lib.rs | 77 +++++++++++++++++++------------- src/bootstrap/src/utils/exec.rs | 52 +++++++++++++++------ 3 files changed, 93 insertions(+), 51 deletions(-) diff --git a/src/bootstrap/src/core/sanity.rs b/src/bootstrap/src/core/sanity.rs index d64923e69cbd0..78862ccb8cd68 100644 --- a/src/bootstrap/src/core/sanity.rs +++ b/src/bootstrap/src/core/sanity.rs @@ -13,7 +13,6 @@ use std::env; use std::ffi::{OsStr, OsString}; use std::fs; use std::path::PathBuf; -use std::process::Command; #[cfg(not(feature = "bootstrap-self-test"))] use crate::builder::Builder; @@ -25,7 +24,6 @@ use std::collections::HashSet; use crate::builder::Kind; use crate::core::config::Target; use crate::utils::exec::BootstrapCommand; -use crate::utils::helpers::output; use crate::Build; pub struct Finder { @@ -210,11 +208,14 @@ than building it. .or_else(|| cmd_finder.maybe_have("reuse")); #[cfg(not(feature = "bootstrap-self-test"))] - let stage0_supported_target_list: HashSet = - output(Command::new(&build.config.initial_rustc).args(["--print", "target-list"])) - .lines() - .map(|s| s.to_string()) - .collect(); + let stage0_supported_target_list: HashSet = crate::utils::helpers::output( + &mut BootstrapCommand::new(&build.config.initial_rustc) + .args(["--print", "target-list"]) + .command, + ) + .lines() + .map(|s| s.to_string()) + .collect(); // We're gonna build some custom C code here and there, host triples // also build some C++ shims for LLVM so we need a C++ compiler. diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index dfaa1e5eb1294..309fec474a14b 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -23,14 +23,13 @@ use std::fmt::Display; use std::fs::{self, File}; use std::io; use std::path::{Path, PathBuf}; -use std::process::{Command, Stdio}; +use std::process::{Command, Output, Stdio}; use std::str; use std::sync::OnceLock; use std::time::SystemTime; use build_helper::ci::{gha, CiEnv}; use build_helper::exit; -use build_helper::util::fail; use filetime::FileTime; use sha2::digest::Digest; use termcolor::{ColorChoice, StandardStream, WriteColor}; @@ -945,43 +944,61 @@ impl Build { self.verbose(|| println!("running: {command:?}")); - let output: io::Result = match command.output_mode { - OutputMode::Print => command.command.status().map(|status| status.into()), - OutputMode::CaptureAll => command.command.output().map(|o| o.into()), + let output: io::Result = match command.output_mode { + OutputMode::Print => command.command.status().map(|status| Output { + status, + stdout: vec![], + stderr: vec![], + }), + OutputMode::CaptureAll => command.command.output(), OutputMode::CaptureStdout => { command.command.stderr(Stdio::inherit()); - command.command.output().map(|o| o.into()) + command.command.output() } }; - let output = match output { - Ok(output) => output, - Err(e) => fail(&format!("failed to execute command: {command:?}\nerror: {e}")), - }; - if !output.is_success() { - use std::fmt::Write; - - // Here we build an error message, and below we decide if it should be printed or not. - let mut message = String::new(); - writeln!( - message, - "\n\nCommand {command:?} did not execute successfully.\ + use std::fmt::Write; + + let mut message = String::new(); + let output: CommandOutput = match output { + // Command has succeeded + Ok(output) if output.status.success() => output.into(), + // Command has started, but then it failed + Ok(output) => { + writeln!( + message, + "\n\nCommand {command:?} did not execute successfully.\ \nExpected success, got: {}", - output.status(), - ) - .unwrap(); - - // If the output mode is OutputMode::Print, the output has already been printed to - // stdout/stderr, and we thus don't have anything captured to print anyway. - if matches!(command.output_mode, OutputMode::CaptureAll | OutputMode::CaptureStdout) { - writeln!(message, "\nSTDOUT ----\n{}", output.stdout().trim()).unwrap(); + output.status, + ) + .unwrap(); + + let output: CommandOutput = output.into(); + // If the output mode is OutputMode::Print, the output has already been printed to + // stdout/stderr, and we thus don't have anything captured to print anyway. + if matches!(command.output_mode, OutputMode::CaptureAll | OutputMode::CaptureStdout) + { + writeln!(message, "\nSTDOUT ----\n{}", output.stdout().trim()).unwrap(); - // Stderr is added to the message only if it was captured - if matches!(command.output_mode, OutputMode::CaptureAll) { - writeln!(message, "\nSTDERR ----\n{}", output.stderr().trim()).unwrap(); + // Stderr is added to the message only if it was captured + if matches!(command.output_mode, OutputMode::CaptureAll) { + writeln!(message, "\nSTDERR ----\n{}", output.stderr().trim()).unwrap(); + } } + output } - + // The command did not even start + Err(e) => { + writeln!( + message, + "\n\nCommand {command:?} did not execute successfully.\ + \nIt was not possible to execute the command: {e:?}" + ) + .unwrap(); + CommandOutput::did_not_start() + } + }; + if !output.is_success() { match command.failure_behavior { BehaviorOnFailure::DelayFail => { if self.fail_fast { diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index bda6e0bfbac02..78a3b917e459a 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -132,25 +132,47 @@ impl From for BootstrapCommand { } } +/// Represents the outcome of starting a command. +enum CommandOutcome { + /// The command has started and finished with some status. + Finished(ExitStatus), + /// It was not even possible to start the command. + DidNotStart, +} + /// Represents the output of an executed process. #[allow(unused)] -pub struct CommandOutput(Output); +pub struct CommandOutput { + outcome: CommandOutcome, + stdout: Vec, + stderr: Vec, +} impl CommandOutput { + pub fn did_not_start() -> Self { + Self { outcome: CommandOutcome::DidNotStart, stdout: vec![], stderr: vec![] } + } + pub fn is_success(&self) -> bool { - self.0.status.success() + match self.outcome { + CommandOutcome::Finished(status) => status.success(), + CommandOutcome::DidNotStart => false, + } } pub fn is_failure(&self) -> bool { !self.is_success() } - pub fn status(&self) -> ExitStatus { - self.0.status + pub fn status(&self) -> Option { + match self.outcome { + CommandOutcome::Finished(status) => Some(status), + CommandOutcome::DidNotStart => None, + } } pub fn stdout(&self) -> String { - String::from_utf8(self.0.stdout.clone()).expect("Cannot parse process stdout as UTF-8") + String::from_utf8(self.stdout.clone()).expect("Cannot parse process stdout as UTF-8") } pub fn stdout_if_ok(&self) -> Option { @@ -158,24 +180,26 @@ impl CommandOutput { } pub fn stderr(&self) -> String { - String::from_utf8(self.0.stderr.clone()).expect("Cannot parse process stderr as UTF-8") + String::from_utf8(self.stderr.clone()).expect("Cannot parse process stderr as UTF-8") } } impl Default for CommandOutput { fn default() -> Self { - Self(Output { status: Default::default(), stdout: vec![], stderr: vec![] }) + Self { + outcome: CommandOutcome::Finished(ExitStatus::default()), + stdout: vec![], + stderr: vec![], + } } } impl From for CommandOutput { fn from(output: Output) -> Self { - Self(output) - } -} - -impl From for CommandOutput { - fn from(status: ExitStatus) -> Self { - Self(Output { status, stdout: vec![], stderr: vec![] }) + Self { + outcome: CommandOutcome::Finished(output.status), + stdout: output.stdout, + stderr: output.stderr, + } } } From 70b6e044523b0a2bc89b748a389c04277b5b9da1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 29 Jun 2024 22:07:59 +0200 Subject: [PATCH 07/11] Handle execution of dry run commands --- src/bootstrap/src/core/build_steps/perf.rs | 1 - src/bootstrap/src/core/build_steps/test.rs | 8 +++++++- src/bootstrap/src/core/metadata.rs | 3 +-- src/bootstrap/src/lib.rs | 5 ++--- src/bootstrap/src/utils/exec.rs | 14 +++++++++++++- 5 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/perf.rs b/src/bootstrap/src/core/build_steps/perf.rs index 20ab1836e0068..f41b5fe10f1d9 100644 --- a/src/bootstrap/src/core/build_steps/perf.rs +++ b/src/bootstrap/src/core/build_steps/perf.rs @@ -2,7 +2,6 @@ use crate::core::build_steps::compile::{Std, Sysroot}; use crate::core::build_steps::tool::{RustcPerf, Tool}; use crate::core::builder::Builder; use crate::core::config::DebuginfoLevel; -use crate::utils::exec::BootstrapCommand; /// Performs profiling using `rustc-perf` on a built version of the compiler. pub fn perf(builder: &Builder<'_>) { diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index b063d0aaf4f16..998e45848a1c3 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -1805,7 +1805,13 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the let lldb_exe = builder.config.lldb.clone().unwrap_or_else(|| PathBuf::from("lldb")); let lldb_version = builder - .run(BootstrapCommand::new(&lldb_exe).capture().allow_failure().arg("--version")) + .run( + BootstrapCommand::new(&lldb_exe) + .capture() + .allow_failure() + .run_always() + .arg("--version"), + ) .stdout_if_ok(); if let Some(ref vers) = lldb_version { cmd.arg("--lldb-version").arg(vers); diff --git a/src/bootstrap/src/core/metadata.rs b/src/bootstrap/src/core/metadata.rs index da269959e7b43..49d17107125aa 100644 --- a/src/bootstrap/src/core/metadata.rs +++ b/src/bootstrap/src/core/metadata.rs @@ -81,8 +81,7 @@ fn workspace_members(build: &Build) -> Vec { .arg("--no-deps") .arg("--manifest-path") .arg(build.src.join(manifest_path)); - // FIXME: fix stderr - let metadata_output = build.run(cargo.capture()).stdout(); + let metadata_output = build.run(cargo.capture_stdout().run_always()).stdout(); let Output { packages, .. } = t!(serde_json::from_str(&metadata_output)); packages }; diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 309fec474a14b..ce139a0bc59df 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -936,12 +936,11 @@ impl Build { /// Execute a command and return its output. /// This method should be used for all command executions in bootstrap. fn run>(&self, mut command: C) -> CommandOutput { - if self.config.dry_run() { + let command = command.as_mut(); + if self.config.dry_run() && !command.run_always { return CommandOutput::default(); } - let command = command.as_mut(); - self.verbose(|| println!("running: {command:?}")); let output: io::Result = match command.output_mode { diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index 78a3b917e459a..98d194e64d3ab 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -47,6 +47,8 @@ pub struct BootstrapCommand { pub command: Command, pub failure_behavior: BehaviorOnFailure, pub output_mode: OutputMode, + // Run the command even during dry run + pub run_always: bool, } impl BootstrapCommand { @@ -107,6 +109,11 @@ impl BootstrapCommand { Self { failure_behavior: BehaviorOnFailure::Ignore, ..self } } + pub fn run_always(&mut self) -> &mut Self { + self.run_always = true; + self + } + /// Capture the output of the command, do not print it. pub fn capture(self) -> Self { Self { output_mode: OutputMode::CaptureAll, ..self } @@ -128,7 +135,12 @@ impl AsMut for BootstrapCommand { impl From for BootstrapCommand { fn from(command: Command) -> Self { - Self { command, failure_behavior: BehaviorOnFailure::Exit, output_mode: OutputMode::Print } + Self { + command, + failure_behavior: BehaviorOnFailure::Exit, + output_mode: OutputMode::Print, + run_always: false, + } } } From b90129dd21edb7492c7c4fc58883e849be8deff7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Wed, 3 Jul 2024 11:33:43 +0200 Subject: [PATCH 08/11] Review changes --- src/bootstrap/src/utils/exec.rs | 27 +++++++++++++-------------- src/bootstrap/src/utils/helpers.rs | 9 +++++---- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index 98d194e64d3ab..74fb483773e3a 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -34,8 +34,7 @@ pub enum OutputMode { /// If you want to allow failures, use [allow_failure]. /// If you want to delay failures until the end of bootstrap, use [delay_failure]. /// -/// By default, the command will print its stdout/stderr to stdout/stderr of bootstrap -/// ([OutputMode::Print]). +/// By default, the command will print its stdout/stderr to stdout/stderr of bootstrap ([OutputMode::Print]). /// If you want to handle the output programmatically, use [BootstrapCommand::capture]. /// /// Bootstrap will print a debug log to stdout if the command fails and failure is not allowed. @@ -144,8 +143,8 @@ impl From for BootstrapCommand { } } -/// Represents the outcome of starting a command. -enum CommandOutcome { +/// Represents the current status of `BootstrapCommand`. +enum CommandStatus { /// The command has started and finished with some status. Finished(ExitStatus), /// It was not even possible to start the command. @@ -155,20 +154,20 @@ enum CommandOutcome { /// Represents the output of an executed process. #[allow(unused)] pub struct CommandOutput { - outcome: CommandOutcome, + status: CommandStatus, stdout: Vec, stderr: Vec, } impl CommandOutput { pub fn did_not_start() -> Self { - Self { outcome: CommandOutcome::DidNotStart, stdout: vec![], stderr: vec![] } + Self { status: CommandStatus::DidNotStart, stdout: vec![], stderr: vec![] } } pub fn is_success(&self) -> bool { - match self.outcome { - CommandOutcome::Finished(status) => status.success(), - CommandOutcome::DidNotStart => false, + match self.status { + CommandStatus::Finished(status) => status.success(), + CommandStatus::DidNotStart => false, } } @@ -177,9 +176,9 @@ impl CommandOutput { } pub fn status(&self) -> Option { - match self.outcome { - CommandOutcome::Finished(status) => Some(status), - CommandOutcome::DidNotStart => None, + match self.status { + CommandStatus::Finished(status) => Some(status), + CommandStatus::DidNotStart => None, } } @@ -199,7 +198,7 @@ impl CommandOutput { impl Default for CommandOutput { fn default() -> Self { Self { - outcome: CommandOutcome::Finished(ExitStatus::default()), + status: CommandStatus::Finished(ExitStatus::default()), stdout: vec![], stderr: vec![], } @@ -209,7 +208,7 @@ impl Default for CommandOutput { impl From for CommandOutput { fn from(output: Output) -> Self { Self { - outcome: CommandOutcome::Finished(output.status), + status: CommandStatus::Finished(output.status), stdout: output.stdout, stderr: output.stderr, } diff --git a/src/bootstrap/src/utils/helpers.rs b/src/bootstrap/src/utils/helpers.rs index bbd4185874f4f..2575b7939b44c 100644 --- a/src/bootstrap/src/utils/helpers.rs +++ b/src/bootstrap/src/utils/helpers.rs @@ -494,12 +494,13 @@ pub fn check_cfg_arg(name: &str, values: Option<&[&str]>) -> String { format!("--check-cfg=cfg({name}{next})") } -/// Prepares `Command` that runs git inside the source directory if given. +/// Prepares `BootstrapCommand` that runs git inside the source directory if given. /// /// Whenever a git invocation is needed, this function should be preferred over -/// manually building a git `Command`. This approach allows us to manage bootstrap-specific -/// needs/hacks from a single source, rather than applying them on next to every `Command::new("git")`, -/// which is painful to ensure that the required change is applied on each one of them correctly. +/// manually building a git `BootstrapCommand`. This approach allows us to manage +/// bootstrap-specific needs/hacks from a single source, rather than applying them on next to every +/// `BootstrapCommand::new("git")`, which is painful to ensure that the required change is applied +/// on each one of them correctly. pub fn git(source_dir: Option<&Path>) -> BootstrapCommand { let mut git = BootstrapCommand::new("git"); From b618fea358147080a27f3984a3be078f8b49e519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Wed, 3 Jul 2024 21:04:02 +0200 Subject: [PATCH 09/11] Simplify and generalize implementation of output mode --- src/bootstrap/src/lib.rs | 35 ++++++++++--------------- src/bootstrap/src/utils/exec.rs | 45 +++++++++++++++++++++------------ 2 files changed, 42 insertions(+), 38 deletions(-) diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index ce139a0bc59df..ffdd9bc885a62 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -23,7 +23,7 @@ use std::fmt::Display; use std::fs::{self, File}; use std::io; use std::path::{Path, PathBuf}; -use std::process::{Command, Output, Stdio}; +use std::process::{Command, Stdio}; use std::str; use std::sync::OnceLock; use std::time::SystemTime; @@ -41,7 +41,7 @@ use crate::core::builder::Kind; use crate::core::config::{flags, LldMode}; use crate::core::config::{DryRun, Target}; use crate::core::config::{LlvmLibunwind, TargetSelection}; -use crate::utils::exec::{BehaviorOnFailure, BootstrapCommand, CommandOutput, OutputMode}; +use crate::utils::exec::{BehaviorOnFailure, BootstrapCommand, CommandOutput}; use crate::utils::helpers::{self, dir_is_empty, exe, libdir, mtime, output, symlink_dir}; mod core; @@ -943,18 +943,10 @@ impl Build { self.verbose(|| println!("running: {command:?}")); - let output: io::Result = match command.output_mode { - OutputMode::Print => command.command.status().map(|status| Output { - status, - stdout: vec![], - stderr: vec![], - }), - OutputMode::CaptureAll => command.command.output(), - OutputMode::CaptureStdout => { - command.command.stderr(Stdio::inherit()); - command.command.output() - } - }; + command.command.stdout(command.stdout.stdio()); + command.command.stderr(command.stderr.stdio()); + + let output = command.command.output(); use std::fmt::Write; @@ -973,16 +965,15 @@ impl Build { .unwrap(); let output: CommandOutput = output.into(); - // If the output mode is OutputMode::Print, the output has already been printed to + + // If the output mode is OutputMode::Capture, we can now print the output. + // If it is OutputMode::Print, then the output has already been printed to // stdout/stderr, and we thus don't have anything captured to print anyway. - if matches!(command.output_mode, OutputMode::CaptureAll | OutputMode::CaptureStdout) - { + if command.stdout.captures() { writeln!(message, "\nSTDOUT ----\n{}", output.stdout().trim()).unwrap(); - - // Stderr is added to the message only if it was captured - if matches!(command.output_mode, OutputMode::CaptureAll) { - writeln!(message, "\nSTDERR ----\n{}", output.stderr().trim()).unwrap(); - } + } + if command.stderr.captures() { + writeln!(message, "\nSTDERR ----\n{}", output.stderr().trim()).unwrap(); } output } diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index 74fb483773e3a..086d10c63511a 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -1,6 +1,6 @@ use std::ffi::OsStr; use std::path::Path; -use std::process::{Command, CommandArgs, CommandEnvs, ExitStatus, Output}; +use std::process::{Command, CommandArgs, CommandEnvs, ExitStatus, Output, Stdio}; /// What should be done when the command fails. #[derive(Debug, Copy, Clone)] @@ -13,19 +13,30 @@ pub enum BehaviorOnFailure { Ignore, } -/// How should the output of the command be handled (whether it should be captured or printed). +/// How should the output of a specific stream of the command (stdout/stderr) be handled +/// (whether it should be captured or printed). #[derive(Debug, Copy, Clone)] pub enum OutputMode { - /// Prints the stdout/stderr of the command to stdout/stderr of bootstrap (by inheriting these - /// streams). - /// Corresponds to calling `cmd.status()`. + /// Prints the stream by inheriting it from the bootstrap process. Print, - /// Captures the stdout and stderr of the command into memory. - /// Corresponds to calling `cmd.output()`. - CaptureAll, - /// Captures the stdout of the command into memory, inherits stderr. - /// Corresponds to calling `cmd.output()`. - CaptureStdout, + /// Captures the stream into memory. + Capture, +} + +impl OutputMode { + pub fn captures(&self) -> bool { + match self { + OutputMode::Print => false, + OutputMode::Capture => true, + } + } + + pub fn stdio(&self) -> Stdio { + match self { + OutputMode::Print => Stdio::inherit(), + OutputMode::Capture => Stdio::piped(), + } + } } /// Wrapper around `std::process::Command`. @@ -45,7 +56,8 @@ pub enum OutputMode { pub struct BootstrapCommand { pub command: Command, pub failure_behavior: BehaviorOnFailure, - pub output_mode: OutputMode, + pub stdout: OutputMode, + pub stderr: OutputMode, // Run the command even during dry run pub run_always: bool, } @@ -113,14 +125,14 @@ impl BootstrapCommand { self } - /// Capture the output of the command, do not print it. + /// Capture all output of the command, do not print it. pub fn capture(self) -> Self { - Self { output_mode: OutputMode::CaptureAll, ..self } + Self { stdout: OutputMode::Capture, stderr: OutputMode::Capture, ..self } } /// Capture stdout of the command, do not print it. pub fn capture_stdout(self) -> Self { - Self { output_mode: OutputMode::CaptureStdout, ..self } + Self { stdout: OutputMode::Capture, ..self } } } @@ -137,7 +149,8 @@ impl From for BootstrapCommand { Self { command, failure_behavior: BehaviorOnFailure::Exit, - output_mode: OutputMode::Print, + stdout: OutputMode::Print, + stderr: OutputMode::Print, run_always: false, } } From 54952b444566956b12058162eae383c692da0147 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Wed, 3 Jul 2024 21:14:46 +0200 Subject: [PATCH 10/11] Rebase on master --- src/bootstrap/src/utils/tarball.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/src/utils/tarball.rs b/src/bootstrap/src/utils/tarball.rs index 3c15bb296f39a..1a35dc1fd3896 100644 --- a/src/bootstrap/src/utils/tarball.rs +++ b/src/bootstrap/src/utils/tarball.rs @@ -370,7 +370,11 @@ impl<'a> Tarball<'a> { if self.builder.rust_info().is_managed_git_subrepository() { // %ct means committer date let timestamp = helpers::output( - helpers::git(Some(&self.builder.src)).arg("log").arg("-1").arg("--format=%ct"), + &mut helpers::git(Some(&self.builder.src)) + .arg("log") + .arg("-1") + .arg("--format=%ct") + .command, ); cmd.args(["--override-file-mtime", timestamp.trim()]); } From c198c0e6d0556cec74d10aec998f4e229137c701 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 4 Jul 2024 10:10:30 +0200 Subject: [PATCH 11/11] Do not consider LLDB version to be valid if it is empty When dry run is enabled, the command for finding LLDB version would succeed, but return an empty string. This was inadvertently enabling a code path that should only be executed when the LLDB is actually present and its version is valid. This commit makes sure that if the version is empty, LLDB will be considered not found. --- src/bootstrap/src/core/build_steps/test.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 998e45848a1c3..dc53bd3cfa785 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -1805,14 +1805,9 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the let lldb_exe = builder.config.lldb.clone().unwrap_or_else(|| PathBuf::from("lldb")); let lldb_version = builder - .run( - BootstrapCommand::new(&lldb_exe) - .capture() - .allow_failure() - .run_always() - .arg("--version"), - ) - .stdout_if_ok(); + .run(BootstrapCommand::new(&lldb_exe).capture().allow_failure().arg("--version")) + .stdout_if_ok() + .and_then(|v| if v.trim().is_empty() { None } else { Some(v) }); if let Some(ref vers) = lldb_version { cmd.arg("--lldb-version").arg(vers); let lldb_python_dir = builder