From 8cffb475fd490785ea44367d91161aba9b98a490 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 7 Jul 2024 17:10:58 +0200 Subject: [PATCH] Remove several usages of the `as_command_mut` method Passes `&Builder<'_>` to additional places, so that they could use the `BootstrapCommand` APIs directly, rather than going through `as_command_mut`. --- src/bootstrap/src/core/build_steps/llvm.rs | 1 + src/bootstrap/src/core/build_steps/setup.rs | 14 ++- .../src/core/build_steps/toolstate.rs | 100 ++++++------------ src/bootstrap/src/lib.rs | 64 ++++++----- src/bootstrap/src/utils/tarball.rs | 14 +-- 5 files changed, 78 insertions(+), 115 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index 876d73dce3ce..888290a0479b 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -125,6 +125,7 @@ pub fn prebuilt_llvm_config(builder: &Builder<'_>, target: TargetSelection) -> L static STAMP_HASH_MEMO: OnceLock = OnceLock::new(); let smart_stamp_hash = STAMP_HASH_MEMO.get_or_init(|| { generate_smart_stamp_hash( + builder, &builder.config.src.join("src/llvm-project"), builder.in_tree_llvm_info.sha().unwrap_or_default(), ) diff --git a/src/bootstrap/src/core/build_steps/setup.rs b/src/bootstrap/src/core/build_steps/setup.rs index e155b3025783..226b3729c10c 100644 --- a/src/bootstrap/src/core/build_steps/setup.rs +++ b/src/bootstrap/src/core/build_steps/setup.rs @@ -474,20 +474,18 @@ impl Step for Hook { if config.dry_run() { return; } - t!(install_git_hook_maybe(config)); + t!(install_git_hook_maybe(builder, config)); } } // install a git hook to automatically run tidy, if they want -fn install_git_hook_maybe(config: &Config) -> io::Result<()> { +fn install_git_hook_maybe(builder: &Builder<'_>, config: &Config) -> io::Result<()> { let git = helpers::git(Some(&config.src)) + .capture() .args(["rev-parse", "--git-common-dir"]) - .as_command_mut() - .output() - .map(|output| { - assert!(output.status.success(), "failed to run `git`"); - PathBuf::from(t!(String::from_utf8(output.stdout)).trim()) - })?; + .run(builder) + .stdout(); + let git = PathBuf::from(git.trim()); let hooks_dir = git.join("hooks"); let dst = hooks_dir.join("pre-push"); if dst.exists() { diff --git a/src/bootstrap/src/core/build_steps/toolstate.rs b/src/bootstrap/src/core/build_steps/toolstate.rs index 9ddd68da59d1..2ab0ce7454b1 100644 --- a/src/bootstrap/src/core/build_steps/toolstate.rs +++ b/src/bootstrap/src/core/build_steps/toolstate.rs @@ -99,24 +99,16 @@ fn print_error(tool: &str, submodule: &str) { crate::exit!(3); } -fn check_changed_files(toolstates: &HashMap, ToolState>) { +fn check_changed_files(builder: &Builder<'_>, toolstates: &HashMap, ToolState>) { // Changed files let output = helpers::git(None) + .capture() .arg("diff") .arg("--name-status") .arg("HEAD") .arg("HEAD^") - .as_command_mut() - .output(); - let output = match output { - Ok(o) => o, - Err(e) => { - eprintln!("Failed to get changed files: {e:?}"); - crate::exit!(1); - } - }; - - let output = t!(String::from_utf8(output.stdout)); + .run(builder) + .stdout(); for (tool, submodule) in STABLE_TOOLS.iter().chain(NIGHTLY_TOOLS.iter()) { let changed = output.lines().any(|l| l.starts_with('M') && l.ends_with(submodule)); @@ -186,8 +178,8 @@ impl Step for ToolStateCheck { crate::exit!(1); } - check_changed_files(&toolstates); - checkout_toolstate_repo(); + check_changed_files(builder, &toolstates); + checkout_toolstate_repo(builder); let old_toolstate = read_old_toolstate(); for (tool, _) in STABLE_TOOLS.iter() { @@ -231,7 +223,7 @@ impl Step for ToolStateCheck { } if builder.config.channel == "nightly" && env::var_os("TOOLSTATE_PUBLISH").is_some() { - commit_toolstate_change(&toolstates); + commit_toolstate_change(builder, &toolstates); } } @@ -315,55 +307,34 @@ fn toolstate_repo() -> String { const TOOLSTATE_DIR: &str = "rust-toolstate"; /// Checks out the toolstate repo into `TOOLSTATE_DIR`. -fn checkout_toolstate_repo() { +fn checkout_toolstate_repo(builder: &Builder<'_>) { if let Ok(token) = env::var("TOOLSTATE_REPO_ACCESS_TOKEN") { - prepare_toolstate_config(&token); + prepare_toolstate_config(builder, &token); } if Path::new(TOOLSTATE_DIR).exists() { eprintln!("Cleaning old toolstate directory..."); t!(fs::remove_dir_all(TOOLSTATE_DIR)); } - let status = helpers::git(None) + helpers::git(None) .arg("clone") .arg("--depth=1") .arg(toolstate_repo()) .arg(TOOLSTATE_DIR) - .as_command_mut() - .status(); - let success = match status { - Ok(s) => s.success(), - Err(_) => false, - }; - if !success { - panic!("git clone unsuccessful (status: {status:?})"); - } + .run(builder); } /// 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) - .as_command_mut() - .status(); - let success = match status { - Ok(s) => s.success(), - Err(_) => false, - }; - if !success { - panic!("git config key={key} value={value} failed (status: {status:?})"); - } +fn prepare_toolstate_config(builder: &Builder<'_>, token: &str) { + fn git_config(builder: &Builder<'_>, key: &str, value: &str) { + helpers::git(None).arg("config").arg("--global").arg(key).arg(value).run(builder); } // If changing anything here, then please check that `src/ci/publish_toolstate.sh` is up to date // as well. - git_config("user.email", "7378925+rust-toolstate-update@users.noreply.github.com"); - git_config("user.name", "Rust Toolstate Update"); - git_config("credential.helper", "store"); + git_config(builder, "user.email", "7378925+rust-toolstate-update@users.noreply.github.com"); + git_config(builder, "user.name", "Rust Toolstate Update"); + git_config(builder, "credential.helper", "store"); let credential = format!("https://{token}:x-oauth-basic@github.com\n",); let git_credential_path = PathBuf::from(t!(env::var("HOME"))).join(".git-credentials"); @@ -403,55 +374,51 @@ fn read_old_toolstate() -> Vec { /// /// * See /// if a private email by GitHub is wanted. -fn commit_toolstate_change(current_toolstate: &ToolstateData) { +fn commit_toolstate_change(builder: &Builder<'_>, current_toolstate: &ToolstateData) { let message = format!("({} CI update)", OS.expect("linux/windows only")); let mut success = false; for _ in 1..=5 { // Upload the test results (the new commit-to-toolstate mapping) to the toolstate repo. // This does *not* change the "current toolstate"; that only happens post-landing // via `src/ci/docker/publish_toolstate.sh`. - publish_test_results(current_toolstate); + publish_test_results(builder, current_toolstate); // `git commit` failing means nothing to commit. - let status = t!(helpers::git(Some(Path::new(TOOLSTATE_DIR))) + let status = helpers::git(Some(Path::new(TOOLSTATE_DIR))) + .allow_failure() .arg("commit") .arg("-a") .arg("-m") .arg(&message) - .as_command_mut() - .status()); - if !status.success() { + .run(builder); + if !status.is_success() { success = true; break; } - let status = t!(helpers::git(Some(Path::new(TOOLSTATE_DIR))) + let status = helpers::git(Some(Path::new(TOOLSTATE_DIR))) + .allow_failure() .arg("push") .arg("origin") .arg("master") - .as_command_mut() - .status()); + .run(builder); // If we successfully push, exit. - if status.success() { + if status.is_success() { success = true; break; } eprintln!("Sleeping for 3 seconds before retrying push"); std::thread::sleep(std::time::Duration::from_secs(3)); - let status = t!(helpers::git(Some(Path::new(TOOLSTATE_DIR))) + helpers::git(Some(Path::new(TOOLSTATE_DIR))) .arg("fetch") .arg("origin") .arg("master") - .as_command_mut() - .status()); - assert!(status.success()); - let status = t!(helpers::git(Some(Path::new(TOOLSTATE_DIR))) + .run(builder); + helpers::git(Some(Path::new(TOOLSTATE_DIR))) .arg("reset") .arg("--hard") .arg("origin/master") - .as_command_mut() - .status()); - assert!(status.success()); + .run(builder); } if !success { @@ -464,9 +431,8 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) { /// These results will later be promoted to `latest.json` by the /// `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").as_command_mut().output()); - let commit = t!(String::from_utf8(commit.stdout)); +fn publish_test_results(builder: &Builder<'_>, current_toolstate: &ToolstateData) { + let commit = helpers::git(None).capture().arg("rev-parse").arg("HEAD").run(builder).stdout(); let toolstate_serialized = t!(serde_json::to_string(¤t_toolstate)); diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index daa916ce0a07..52e28fbd0598 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -23,7 +23,7 @@ use std::fmt::Display; use std::fs::{self, File}; use std::io; use std::path::{Path, PathBuf}; -use std::process::{Command, Stdio}; +use std::process::Command; use std::str; use std::sync::OnceLock; use std::time::SystemTime; @@ -37,7 +37,7 @@ use utils::channel::GitInfo; use utils::helpers::hex_encode; use crate::core::builder; -use crate::core::builder::Kind; +use crate::core::builder::{Builder, Kind}; use crate::core::config::{flags, LldMode}; use crate::core::config::{DryRun, Target}; use crate::core::config::{LlvmLibunwind, TargetSelection}; @@ -490,18 +490,18 @@ impl Build { return; } - let submodule_git = || helpers::git(Some(&absolute_path)); + let submodule_git = || helpers::git(Some(&absolute_path)).capture_stdout(); // Determine commit checked out in submodule. - let checked_out_hash = output(submodule_git().args(["rev-parse", "HEAD"]).as_command_mut()); + let checked_out_hash = submodule_git().args(["rev-parse", "HEAD"]).run(self).stdout(); 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) - .as_command_mut(), - ); + let recorded = helpers::git(Some(&self.src)) + .capture_stdout() + .args(["ls-tree", "HEAD"]) + .arg(relative_path) + .run(self) + .stdout(); let actual_hash = recorded .split_whitespace() .nth(2) @@ -522,21 +522,14 @@ impl Build { let update = |progress: bool| { // Git is buggy and will try to fetch submodules from the tracking branch for *this* repository, // even though that has no relation to the upstream for the submodule. - let current_branch = { - let output = helpers::git(Some(&self.src)) - .args(["symbolic-ref", "--short", "HEAD"]) - .as_command_mut() - .stderr(Stdio::inherit()) - .output(); - let output = t!(output); - if output.status.success() { - Some(String::from_utf8(output.stdout).unwrap().trim().to_owned()) - } else { - None - } - }; + let current_branch = helpers::git(Some(&self.src)) + .capture_stdout() + .args(["symbolic-ref", "--short", "HEAD"]) + .run(self) + .stdout_if_ok() + .map(|s| s.trim().to_owned()); - let mut git = helpers::git(Some(&self.src)); + let mut git = helpers::git(Some(&self.src)).allow_failure(); if let Some(branch) = current_branch { // If there is a tag named after the current branch, git will try to disambiguate by prepending `heads/` to the branch name. // This syntax isn't accepted by `branch.{branch}`. Strip it. @@ -550,8 +543,7 @@ impl Build { git.arg(relative_path); git }; - // NOTE: doesn't use `try_run` because this shouldn't print an error if it fails. - if !update(true).as_command_mut().status().map_or(false, |status| status.success()) { + if !update(true).run(self).is_success() { update(false).run(self); } @@ -1943,22 +1935,28 @@ fn envify(s: &str) -> String { /// /// In case of errors during `git` command execution (e.g., in tarball sources), default values /// are used to prevent panics. -pub fn generate_smart_stamp_hash(dir: &Path, additional_input: &str) -> String { +pub fn generate_smart_stamp_hash( + builder: &Builder<'_>, + dir: &Path, + additional_input: &str, +) -> String { let diff = helpers::git(Some(dir)) + .capture_stdout() + .allow_failure() .arg("diff") - .as_command_mut() - .output() - .map(|o| String::from_utf8(o.stdout).unwrap_or_default()) + .run(builder) + .stdout_if_ok() .unwrap_or_default(); let status = helpers::git(Some(dir)) + .capture_stdout() + .allow_failure() .arg("status") .arg("--porcelain") .arg("-z") .arg("--untracked-files=normal") - .as_command_mut() - .output() - .map(|o| String::from_utf8(o.stdout).unwrap_or_default()) + .run(builder) + .stdout_if_ok() .unwrap_or_default(); let mut hasher = sha2::Sha256::new(); diff --git a/src/bootstrap/src/utils/tarball.rs b/src/bootstrap/src/utils/tarball.rs index f5fc94273c9a..9378c35127f2 100644 --- a/src/bootstrap/src/utils/tarball.rs +++ b/src/bootstrap/src/utils/tarball.rs @@ -369,13 +369,13 @@ impl<'a> Tarball<'a> { // gets the same timestamp. 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") - .as_command_mut(), - ); + let timestamp = helpers::git(Some(&self.builder.src)) + .capture_stdout() + .arg("log") + .arg("-1") + .arg("--format=%ct") + .run(self.builder) + .stdout(); cmd.args(["--override-file-mtime", timestamp.trim()]); }