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 8bcb2301f1a..87a3a2b0b28 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 @@ pub fn output_mode(self, output_mode: OutputMode) -> Self { } } -/// 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 @@ fn from(command: &'a mut Command) -> Self { } } -/// 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 f5e34672686..4583eb1c6cd 100644 --- a/src/bootstrap/src/core/build_steps/format.rs +++ b/src/bootstrap/src/core/build_steps/format.rs @@ -7,7 +7,7 @@ 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 8e6795b11bd..ba21e2ad32d 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 f41b5fe10f1..20ab1836e00 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::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 947c74f32c9..e6a09e8cb8e 100644 --- a/src/bootstrap/src/core/build_steps/setup.rs +++ b/src/bootstrap/src/core/build_steps/setup.rs @@ -484,6 +484,7 @@ fn run(self, builder: &Builder<'_>) -> Self::Output { 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 1460a229019..918e4e2b907 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 9e6d03349b5..e3e7931a5a2 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 948f97e746f..10ac6c93e9a 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -1259,6 +1259,7 @@ pub(crate) fn parse_inner(args: &[String], get_toml: impl Fn(&Path) -> TomlConfi 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 @@ pub(crate) fn read_file_by_commit(&self, file: &Path, commit: &str) -> String { 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 @@ fn download_ci_rustc_commit(&self, download_rustc: Option) -> Opti }; // 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 @@ fn download_ci_rustc_commit(&self, download_rustc: Option) -> Opti // 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 @@ fn download_ci_rustc_commit(&self, download_rustc: Option) -> Opti // 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 @@ pub fn last_modified_commit( 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 @@ pub fn last_modified_commit( 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 c12449fdc4a..ae2982ea56f 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -494,11 +494,12 @@ pub(crate) fn update_submodule(&self, relative_path: &Path) { 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 @@ pub(crate) fn update_submodule(&self, relative_path: &Path) { 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 @@ pub(crate) fn update_submodule(&self, relative_path: &Path) { 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 @@ pub fn update_existing_submodules(&self) { 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 @@ fn run>(&self, command: C) -> CommandOutput { 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 @@ fn extract_beta_rev_from_file>(version_file: P) -> Option // 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 ce82c52f049..2ca86bdb0ed 100644 --- a/src/bootstrap/src/utils/channel.rs +++ b/src/bootstrap/src/utils/channel.rs @@ -45,7 +45,7 @@ pub fn new(omit_git_hash: bool, dir: &Path) -> 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 @@ pub fn new(omit_git_hash: bool, dir: &Path) -> 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 87a3a2b0b28..dc30876601c 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 @@ pub fn allow_failure(self) -> Self { } /// 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 @@ pub fn output_mode(self, output_mode: OutputMode) -> Self { /// 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 @@ fn from(command: &'a mut Command) -> Self { } } } - - 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 adf18c0ace1..9c40065dfc4 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 de3b938e427..c267d3b0c0c 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 4a5af25b3b2..823e842693e 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 @@ fn run(self, builder: &Builder<'_>) { 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 @@ fn run(self, builder: &Builder<'_>) { 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 4583eb1c6cd..0372360c3cb 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 22d5efa5d95..496fe446d72 100644 --- a/src/bootstrap/src/core/build_steps/run.rs +++ b/src/bootstrap/src/core/build_steps/run.rs @@ -158,7 +158,7 @@ fn run(self, builder: &Builder<'_>) { // 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 918e4e2b907..9fc337dfd50 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::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 @@ fn run(self, builder: &Builder<'_>) { 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 @@ fn run(self, builder: &Builder<'_>) { )); 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 @@ fn run(self, builder: &Builder<'_>) { 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 @@ fn run(self, builder: &Builder<'_>) { 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 @@ fn run(self, builder: &Builder<'_>) { .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 @@ fn run(self, builder: &Builder<'_>) { } 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 @@ fn run(self, builder: &Builder<'_>) -> PathBuf { &[], ); - 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 @@ fn run_ext_doc(self, builder: &Builder<'_>) { 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 @@ fn run(self, builder: &Builder<'_>) { 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 @@ fn run(self, builder: &Builder<'_>) { 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 @@ fn run(self, builder: &Builder<'_>) { .arg("testsuite.extended_sysroot"); cargo.args(builder.config.test_args()); - builder.run(cargo); + builder.run(cargo.into_cmd()); } } @@ -3472,6 +3468,6 @@ fn run(self, builder: &Builder<'_>) { .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 b3464043912..20c7a30f1a8 100644 --- a/src/bootstrap/src/core/build_steps/tool.rs +++ b/src/bootstrap/src/core/build_steps/tool.rs @@ -603,7 +603,7 @@ fn run(self, builder: &Builder<'_>) -> PathBuf { &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 @@ fn run(self, builder: &Builder<'_>) -> PathBuf { &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 58d6e7a58e3..35de91c41d4 100644 --- a/src/bootstrap/src/core/builder.rs +++ b/src/bootstrap/src/core/builder.rs @@ -2398,6 +2398,10 @@ pub fn new( 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 @@ fn from(mut cargo: Cargo) -> 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 ae2982ea56f..ae3d18e8774 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -555,10 +555,7 @@ pub(crate) fn update_submodule(&self, relative_path: &Path) { // 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 @@ pub fn update_existing_submodules(&self) { 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 @@ fn rustc_snapshot_sysroot(&self) -> &Path { /// 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; - if !self.is_verbose() { - println!("Add `-v` to see more details.\n"); + // 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 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(); } - - self.verbose(|| { - println!( - "\nSTDOUT ----\n{}\n\ - STDERR ----\n{}\n", - output.stdout(), - output.stderr(), - ) - }); } 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 @@ fn extract_beta_rev_from_file>(version_file: P) -> Option // (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 dc30876601c..5f9e1648161 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 @@ pub fn allow_failure(self) -> Self { 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) + /// Capture the output of the command, do not print it. + pub fn capture(self) -> Self { + Self { output_mode: OutputMode::CaptureAll, ..self } } - /// 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 } + /// 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 @@ pub fn stdout(&self) -> String { 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 c267d3b0c0c..01b25224de4 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::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 @@ fn run(self, builder: &Builder<'_>) -> Compiler { 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 08795efe5bb..3dc871b1ca9 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::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::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 @@ fn run(self, builder: &Builder<'_>) -> GeneratedTarball { } // 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 @@ fn run(self, builder: &Builder<'_>) -> GeneratedTarball { } 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 0372360c3cb..66286a52813 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 ba21e2ad32d..4f1c1f87d1c 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::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::{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 @@ fn run(self, builder: &Builder<'_>) -> LlvmResult { 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 @@ fn run(self, builder: &Builder<'_>) -> LlvmResult { // 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 496fe446d72..316a03a2171 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::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 @@ fn run(self, builder: &Builder<'_>) { 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 7c5e0d4e13e..5412b3c807b 100644 --- a/src/bootstrap/src/core/build_steps/suggest.rs +++ b/src/bootstrap/src/core/build_steps/suggest.rs @@ -13,24 +13,15 @@ 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 281a9b093b9..a115ba2d8b2 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 9fc337dfd50..b063d0aaf4f 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::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 @@ pub fn build_miri_sysroot( // 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 @@ fn run(self, builder: &Builder<'_>) { } } -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 @@ fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { .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 @@ fn run(self, builder: &Builder<'_>) { } 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 @@ fn run(self, builder: &Builder<'_>) { 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 @@ fn run(self, builder: &Builder<'_>) { // 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 20c7a30f1a8..40184e016a6 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::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 @@ fn run(self, builder: &Builder<'_>) -> LibcxxVersion { } } - 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 35de91c41d4..4da912994c3 100644 --- a/src/bootstrap/src/core/builder.rs +++ b/src/bootstrap/src/core/builder.rs @@ -8,7 +8,6 @@ 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::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 @@ fn cargo( // 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 220eb5ba126..da269959e7b 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 5a0be2948a1..d64923e69cb 100644 --- a/src/bootstrap/src/core/sanity.rs +++ b/src/bootstrap/src/core/sanity.rs @@ -24,6 +24,7 @@ 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 @@ pub fn check(build: &mut Build) { // 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 ae3d18e8774..dfaa1e5eb12 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -861,14 +861,16 @@ fn llvm_filecheck(&self, target: TargetSelection) -> PathBuf { 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 540b8671333..b80df545202 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 5f9e1648161..bda6e0bfbac 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 @@ fn as_mut(&mut self) -> &mut 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 9c40065dfc4..bbd4185874f 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 40184e016a6..5fb282db90a 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 | 81 +++++++++++++++++++------------- src/bootstrap/src/utils/exec.rs | 52 ++++++++++++++------ 3 files changed, 95 insertions(+), 53 deletions(-) diff --git a/src/bootstrap/src/core/sanity.rs b/src/bootstrap/src/core/sanity.rs index d64923e69cb..78862ccb8cd 100644 --- a/src/bootstrap/src/core/sanity.rs +++ b/src/bootstrap/src/core/sanity.rs @@ -13,7 +13,6 @@ 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 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 @@ pub fn check(build: &mut Build) { .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 dfaa1e5eb12..309fec474a1 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -23,14 +23,13 @@ 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 @@ fn run>(&self, mut command: C) -> CommandOutput { 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}")), + 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(); + + 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(); + } + } + 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() { - 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 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(); - } - } - 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 bda6e0bfbac..78a3b917e45 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -132,25 +132,47 @@ fn from(command: Command) -> Self { } } +/// 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 @@ pub fn stdout_if_ok(&self) -> Option { } 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 20ab1836e00..f41b5fe10f1 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::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 b063d0aaf4f..998e45848a1 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -1805,7 +1805,13 @@ fn run(self, builder: &Builder<'_>) { 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 da269959e7b..49d17107125 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 309fec474a1..ce139a0bc59 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -936,12 +936,11 @@ fn rustc_snapshot_sysroot(&self) -> &Path { /// 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 78a3b917e45..98d194e64d3 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 @@ pub fn allow_failure(self) -> Self { 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 @@ fn as_mut(&mut self) -> &mut 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 98d194e64d3..74fb483773e 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 @@ fn from(command: Command) -> Self { } } -/// 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 @@ pub fn is_failure(&self) -> bool { } 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 @@ pub fn stderr(&self) -> String { 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 @@ fn default() -> Self { 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 bbd4185874f..2575b7939b4 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 | 37 ++++++++++----------------- src/bootstrap/src/utils/exec.rs | 45 +++++++++++++++++++++------------ 2 files changed, 43 insertions(+), 39 deletions(-) diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index ce139a0bc59..ffdd9bc885a 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -23,7 +23,7 @@ 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::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 @@ fn run>(&self, mut command: C) -> CommandOutput { 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 @@ fn run>(&self, mut command: C) -> CommandOutput { .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(); - } + // 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 command.stdout.captures() { + writeln!(message, "\nSTDOUT ----\n{}", output.stdout().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 74fb483773e..086d10c6351 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 @@ pub fn run_always(&mut self) -> &mut Self { 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 @@ fn from(command: Command) -> Self { 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 3c15bb296f3..1a35dc1fd38 100644 --- a/src/bootstrap/src/utils/tarball.rs +++ b/src/bootstrap/src/utils/tarball.rs @@ -370,7 +370,11 @@ fn run(self, build_cli: impl FnOnce(&Tarball<'a>, &mut BootstrapCommand)) -> Gen 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 998e45848a1..dc53bd3cfa7 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -1805,14 +1805,9 @@ fn run(self, builder: &Builder<'_>) { 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