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] 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 f5e346726868..4583eb1c6cd8 100644 --- a/src/bootstrap/src/core/build_steps/format.rs +++ b/src/bootstrap/src/core/build_steps/format.rs @@ -7,7 +7,7 @@ use build_helper::git::get_git_modified_files; use ignore::WalkBuilder; use std::collections::VecDeque; use std::path::{Path, PathBuf}; -use std::process::{Command, Stdio}; +use std::process::Command; use std::sync::mpsc::SyncSender; use std::sync::Mutex; @@ -160,35 +160,29 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) { override_builder.add(&format!("!{ignore}")).expect(&ignore); } } - let git_available = match helpers::git(None) - .arg("--version") - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - { - Ok(status) => status.success(), - Err(_) => false, - }; + let git_available = build + .run(helpers::git(None).print_on_failure().allow_failure().arg("--version")) + .is_success(); let mut adjective = None; if git_available { - let in_working_tree = match helpers::git(Some(&build.src)) - .arg("rev-parse") - .arg("--is-inside-work-tree") - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - { - Ok(status) => status.success(), - Err(_) => false, - }; + let in_working_tree = build + .run( + helpers::git(Some(&build.src)) + .print_on_failure() + .allow_failure() + .arg("rev-parse") + .arg("--is-inside-work-tree"), + ) + .is_success(); if in_working_tree { let untracked_paths_output = output( - helpers::git(Some(&build.src)) + &mut helpers::git(Some(&build.src)) .arg("status") .arg("--porcelain") .arg("-z") - .arg("--untracked-files=normal"), + .arg("--untracked-files=normal") + .command, ); let untracked_paths: Vec<_> = untracked_paths_output .split_terminator('\0') diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index 8e6795b11bd2..ba21e2ad32d0 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 f41b5fe10f1d..20ab1836e006 100644 --- a/src/bootstrap/src/core/build_steps/perf.rs +++ b/src/bootstrap/src/core/build_steps/perf.rs @@ -2,6 +2,7 @@ use crate::core::build_steps::compile::{Std, Sysroot}; use crate::core::build_steps::tool::{RustcPerf, Tool}; use crate::core::builder::Builder; use crate::core::config::DebuginfoLevel; +use crate::utils::exec::BootstrapCommand; /// Performs profiling using `rustc-perf` on a built version of the compiler. pub fn perf(builder: &Builder<'_>) { diff --git a/src/bootstrap/src/core/build_steps/setup.rs b/src/bootstrap/src/core/build_steps/setup.rs index 947c74f32c99..e6a09e8cb8e8 100644 --- a/src/bootstrap/src/core/build_steps/setup.rs +++ b/src/bootstrap/src/core/build_steps/setup.rs @@ -484,6 +484,7 @@ impl Step for Hook { fn install_git_hook_maybe(config: &Config) -> io::Result<()> { let git = helpers::git(Some(&config.src)) .args(["rev-parse", "--git-common-dir"]) + .command .output() .map(|output| { assert!(output.status.success(), "failed to run `git`"); diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 1460a2290197..918e4e2b9072 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 9e6d03349b5f..e3e7931a5a2a 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 948f97e746f4..10ac6c93e9a4 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -1259,6 +1259,7 @@ impl Config { cmd.arg("rev-parse").arg("--show-cdup"); // Discard stderr because we expect this to fail when building from a tarball. let output = cmd + .command .stderr(std::process::Stdio::null()) .output() .ok() @@ -2141,7 +2142,7 @@ impl Config { let mut git = helpers::git(Some(&self.src)); git.arg("show").arg(format!("{commit}:{}", file.to_str().unwrap())); - output(&mut git) + output(&mut git.command) } /// Bootstrap embeds a version number into the name of shared libraries it uploads in CI. @@ -2445,8 +2446,9 @@ impl Config { }; // Handle running from a directory other than the top level - let top_level = - output(helpers::git(Some(&self.src)).args(["rev-parse", "--show-toplevel"])); + let top_level = output( + &mut helpers::git(Some(&self.src)).args(["rev-parse", "--show-toplevel"]).command, + ); let top_level = top_level.trim_end(); let compiler = format!("{top_level}/compiler/"); let library = format!("{top_level}/library/"); @@ -2454,10 +2456,11 @@ impl Config { // Look for a version to compare to based on the current commit. // Only commits merged by bors will have CI artifacts. let merge_base = output( - helpers::git(Some(&self.src)) + &mut helpers::git(Some(&self.src)) .arg("rev-list") .arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email)) - .args(["-n1", "--first-parent", "HEAD"]), + .args(["-n1", "--first-parent", "HEAD"]) + .command, ); let commit = merge_base.trim_end(); if commit.is_empty() { @@ -2471,6 +2474,7 @@ impl Config { // Warn if there were changes to the compiler or standard library since the ancestor commit. let has_changes = !t!(helpers::git(Some(&self.src)) .args(["diff-index", "--quiet", commit, "--", &compiler, &library]) + .command .status()) .success(); if has_changes { @@ -2542,17 +2546,19 @@ impl Config { if_unchanged: bool, ) -> Option { // Handle running from a directory other than the top level - let top_level = - output(helpers::git(Some(&self.src)).args(["rev-parse", "--show-toplevel"])); + let top_level = output( + &mut helpers::git(Some(&self.src)).args(["rev-parse", "--show-toplevel"]).command, + ); let top_level = top_level.trim_end(); // Look for a version to compare to based on the current commit. // Only commits merged by bors will have CI artifacts. let merge_base = output( - helpers::git(Some(&self.src)) + &mut helpers::git(Some(&self.src)) .arg("rev-list") .arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email)) - .args(["-n1", "--first-parent", "HEAD"]), + .args(["-n1", "--first-parent", "HEAD"]) + .command, ); let commit = merge_base.trim_end(); if commit.is_empty() { @@ -2571,7 +2577,7 @@ impl Config { git.arg(format!("{top_level}/{path}")); } - let has_changes = !t!(git.status()).success(); + let has_changes = !t!(git.command.status()).success(); if has_changes { if if_unchanged { if self.verbose > 0 { diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index c12449fdc4ae..ae2982ea56f0 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -494,11 +494,12 @@ impl Build { let submodule_git = || helpers::git(Some(&absolute_path)); // Determine commit checked out in submodule. - let checked_out_hash = output(submodule_git().args(["rev-parse", "HEAD"])); + let checked_out_hash = output(&mut submodule_git().args(["rev-parse", "HEAD"]).command); let checked_out_hash = checked_out_hash.trim_end(); // Determine commit that the submodule *should* have. - let recorded = - output(helpers::git(Some(&self.src)).args(["ls-tree", "HEAD"]).arg(relative_path)); + let recorded = output( + &mut helpers::git(Some(&self.src)).args(["ls-tree", "HEAD"]).arg(relative_path).command, + ); let actual_hash = recorded .split_whitespace() .nth(2) @@ -521,6 +522,7 @@ impl Build { let current_branch = { let output = helpers::git(Some(&self.src)) .args(["symbolic-ref", "--short", "HEAD"]) + .command .stderr(Stdio::inherit()) .output(); let output = t!(output); @@ -546,7 +548,7 @@ impl Build { git }; // NOTE: doesn't use `try_run` because this shouldn't print an error if it fails. - if !update(true).status().map_or(false, |status| status.success()) { + if !update(true).command.status().map_or(false, |status| status.success()) { self.run(update(false)); } @@ -577,12 +579,15 @@ impl Build { if !self.config.submodules(self.rust_info()) { return; } - let output = output( - helpers::git(Some(&self.src)) - .args(["config", "--file"]) - .arg(self.config.src.join(".gitmodules")) - .args(["--get-regexp", "path"]), - ); + let output = self + .run( + helpers::git(Some(&self.src)) + .quiet() + .args(["config", "--file"]) + .arg(self.config.src.join(".gitmodules")) + .args(["--get-regexp", "path"]), + ) + .stdout(); for line in output.lines() { // Look for `submodule.$name.path = $path` // Sample output: `submodule.src/rust-installer.path src/tools/rust-installer` @@ -950,7 +955,10 @@ impl Build { command.command.status().map(|status| status.into()), matches!(mode, OutputMode::All), ), - OutputMode::OnlyOnFailure => (command.command.output().map(|o| o.into()), true), + mode @ (OutputMode::OnlyOnFailure | OutputMode::Quiet) => ( + command.command.output().map(|o| o.into()), + matches!(mode, OutputMode::OnlyOnFailure), + ), }; let output = match output { @@ -1480,14 +1488,18 @@ impl Build { // Figure out how many merge commits happened since we branched off master. // That's our beta number! // (Note that we use a `..` range, not the `...` symmetric difference.) - output( - helpers::git(Some(&self.src)).arg("rev-list").arg("--count").arg("--merges").arg( - format!( + self.run( + helpers::git(Some(&self.src)) + .quiet() + .arg("rev-list") + .arg("--count") + .arg("--merges") + .arg(format!( "refs/remotes/origin/{}..HEAD", self.config.stage0_metadata.config.nightly_branch - ), - ), + )), ) + .stdout() }); let n = count.trim().parse().unwrap(); self.prerelease_version.set(Some(n)); @@ -1914,6 +1926,7 @@ fn envify(s: &str) -> String { pub fn generate_smart_stamp_hash(dir: &Path, additional_input: &str) -> String { let diff = helpers::git(Some(dir)) .arg("diff") + .command .output() .map(|o| String::from_utf8(o.stdout).unwrap_or_default()) .unwrap_or_default(); @@ -1923,6 +1936,7 @@ pub fn generate_smart_stamp_hash(dir: &Path, additional_input: &str) -> String { .arg("--porcelain") .arg("-z") .arg("--untracked-files=normal") + .command .output() .map(|o| String::from_utf8(o.stdout).unwrap_or_default()) .unwrap_or_default(); diff --git a/src/bootstrap/src/utils/channel.rs b/src/bootstrap/src/utils/channel.rs index ce82c52f049e..2ca86bdb0ed2 100644 --- a/src/bootstrap/src/utils/channel.rs +++ b/src/bootstrap/src/utils/channel.rs @@ -45,7 +45,7 @@ impl GitInfo { } // Make sure git commands work - match helpers::git(Some(dir)).arg("rev-parse").output() { + match helpers::git(Some(dir)).arg("rev-parse").command.output() { Ok(ref out) if out.status.success() => {} _ => return GitInfo::Absent, } @@ -58,15 +58,17 @@ impl GitInfo { // Ok, let's scrape some info let ver_date = output( - helpers::git(Some(dir)) + &mut helpers::git(Some(dir)) .arg("log") .arg("-1") .arg("--date=short") - .arg("--pretty=format:%cd"), + .arg("--pretty=format:%cd") + .command, + ); + let ver_hash = output(&mut helpers::git(Some(dir)).arg("rev-parse").arg("HEAD").command); + let short_ver_hash = output( + &mut helpers::git(Some(dir)).arg("rev-parse").arg("--short=9").arg("HEAD").command, ); - let ver_hash = output(helpers::git(Some(dir)).arg("rev-parse").arg("HEAD")); - let short_ver_hash = - output(helpers::git(Some(dir)).arg("rev-parse").arg("--short=9").arg("HEAD")); GitInfo::Present(Some(Info { commit_date: ver_date.trim().to_string(), sha: ver_hash.trim().to_string(), diff --git a/src/bootstrap/src/utils/exec.rs b/src/bootstrap/src/utils/exec.rs index 87a3a2b0b284..dc30876601c6 100644 --- a/src/bootstrap/src/utils/exec.rs +++ b/src/bootstrap/src/utils/exec.rs @@ -23,6 +23,8 @@ pub enum OutputMode { OnlyOutput, /// Suppress the output if the command succeeds, otherwise print the output. OnlyOnFailure, + /// Suppress the output of the command. + Quiet, } /// Wrapper around `std::process::Command`. @@ -105,10 +107,15 @@ impl BootstrapCommand { } /// Do not print the output of the command, unless it fails. - pub fn quiet(self) -> Self { + pub fn print_on_failure(self) -> Self { self.output_mode(OutputMode::OnlyOnFailure) } + /// Do not print the output of the command. + pub fn quiet(self) -> Self { + self.output_mode(OutputMode::Quiet) + } + pub fn output_mode(self, output_mode: OutputMode) -> Self { Self { output_mode: Some(output_mode), ..self } } @@ -116,15 +123,15 @@ impl BootstrapCommand { /// FIXME: This implementation is temporary, until all `Command` invocations are migrated to /// `BootstrapCommand`. -impl<'a> From<&'a mut Command> for BootstrapCommand { - fn from(command: &'a mut Command) -> Self { +impl<'a> From<&'a mut BootstrapCommand> for BootstrapCommand { + fn from(command: &'a mut BootstrapCommand) -> Self { // This is essentially a manual `Command::clone` - let mut cmd = Command::new(command.get_program()); - if let Some(dir) = command.get_current_dir() { + let mut cmd = Command::new(command.command.get_program()); + if let Some(dir) = command.command.get_current_dir() { cmd.current_dir(dir); } - cmd.args(command.get_args()); - for (key, value) in command.get_envs() { + cmd.args(command.command.get_args()); + for (key, value) in command.command.get_envs() { match value { Some(value) => { cmd.env(key, value); @@ -134,16 +141,11 @@ impl<'a> From<&'a mut Command> for BootstrapCommand { } } } - - cmd.into() - } -} - -/// FIXME: This implementation is temporary, until all `Command` invocations are migrated to -/// `BootstrapCommand`. -impl<'a> From<&'a mut BootstrapCommand> for BootstrapCommand { - fn from(command: &'a mut BootstrapCommand) -> Self { - BootstrapCommand::from(&mut command.command) + Self { + command: cmd, + output_mode: command.output_mode, + failure_behavior: command.failure_behavior, + } } } diff --git a/src/bootstrap/src/utils/helpers.rs b/src/bootstrap/src/utils/helpers.rs index adf18c0ace1b..9c40065dfc4f 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);