Skip to content

Commit

Permalink
unify git command preperation
Browse files Browse the repository at this point in the history
Due to #125954, we had to modify git invocations with
certain flags in #126255. However, because there are so many
instances of `Command::new("git")` in bootstrap, it is difficult to apply these solutions to all of
them.

This PR creates a helper function that unifies the git usage in bootstrap. Meaning, whenever special flags
or hacks are needed, we can apply them to this single function which makes things much simpler for the bootstrap team.

Signed-off-by: onur-ozkan <[email protected]>
  • Loading branch information
onur-ozkan committed Jun 12, 2024
1 parent 76c7382 commit 17a7ab7
Show file tree
Hide file tree
Showing 8 changed files with 84 additions and 95 deletions.
12 changes: 4 additions & 8 deletions src/bootstrap/src/core/build_steps/format.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Runs rustfmt on the repository.
use crate::core::builder::Builder;
use crate::utils::helpers::{output, program_out_of_date, t};
use crate::utils::helpers::{self, output, program_out_of_date, t};
use build_helper::ci::CiEnv;
use build_helper::git::get_git_modified_files;
use ignore::WalkBuilder;
Expand Down Expand Up @@ -160,7 +160,7 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
override_builder.add(&format!("!{ignore}")).expect(&ignore);
}
}
let git_available = match Command::new("git")
let git_available = match helpers::git(None)
.arg("--version")
.stdout(Stdio::null())
.stderr(Stdio::null())
Expand All @@ -172,9 +172,7 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {

let mut adjective = None;
if git_available {
let in_working_tree = match build
.config
.git()
let in_working_tree = match helpers::git(Some(&build.src))
.arg("rev-parse")
.arg("--is-inside-work-tree")
.stdout(Stdio::null())
Expand All @@ -186,9 +184,7 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
};
if in_working_tree {
let untracked_paths_output = output(
build
.config
.git()
helpers::git(Some(&build.src))
.arg("status")
.arg("--porcelain")
.arg("-z")
Expand Down
4 changes: 2 additions & 2 deletions src/bootstrap/src/core/build_steps/llvm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ pub(crate) fn detect_llvm_sha(config: &Config, is_git: bool) -> String {
// in that case.
let closest_upstream = get_git_merge_base(&config.git_config(), Some(&config.src))
.unwrap_or_else(|_| "HEAD".into());
let mut rev_list = config.git();
let mut rev_list = helpers::git(Some(&config.src));
rev_list.args(&[
PathBuf::from("rev-list"),
format!("--author={}", config.stage0_metadata.config.git_merge_commit_email).into(),
Expand Down Expand Up @@ -252,7 +252,7 @@ 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(config.git().arg("rev-parse").arg("HEAD"));
let head_sha = output(helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD"));
let head_sha = head_sha.trim();
llvm_sha == head_sha
}
Expand Down
13 changes: 8 additions & 5 deletions src/bootstrap/src/core/build_steps/setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use crate::core::builder::{Builder, RunConfig, ShouldRun, Step};
use crate::t;
use crate::utils::change_tracker::CONFIG_CHANGE_HISTORY;
use crate::utils::helpers::hex_encode;
use crate::utils::helpers::{self, hex_encode};
use crate::Config;
use sha2::Digest;
use std::env::consts::EXE_SUFFIX;
Expand Down Expand Up @@ -482,10 +482,13 @@ impl Step for Hook {

// install a git hook to automatically run tidy, if they want
fn install_git_hook_maybe(config: &Config) -> io::Result<()> {
let git = config.git().args(["rev-parse", "--git-common-dir"]).output().map(|output| {
assert!(output.status.success(), "failed to run `git`");
PathBuf::from(t!(String::from_utf8(output.stdout)).trim())
})?;
let git = helpers::git(Some(&config.src))
.args(["rev-parse", "--git-common-dir"])
.output()
.map(|output| {
assert!(output.status.success(), "failed to run `git`");
PathBuf::from(t!(String::from_utf8(output.stdout)).trim())
})?;
let hooks_dir = git.join("hooks");
let dst = hooks_dir.join("pre-push");
if dst.exists() {
Expand Down
29 changes: 10 additions & 19 deletions src/bootstrap/src/core/build_steps/toolstate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@
//! [Toolstate]: https://forge.rust-lang.org/infra/toolstate.html
use crate::core::builder::{Builder, RunConfig, ShouldRun, Step};
use crate::utils::helpers::t;
use crate::utils::helpers::{self, t};
use serde_derive::{Deserialize, Serialize};
use std::collections::HashMap;
use std::env;
use std::fmt;
use std::fs;
use std::io::{Seek, SeekFrom};
use std::path::{Path, PathBuf};
use std::process::Command;
use std::time;

// Each cycle is 42 days long (6 weeks); the last week is 35..=42 then.
Expand Down Expand Up @@ -102,12 +101,8 @@ fn print_error(tool: &str, submodule: &str) {

fn check_changed_files(toolstates: &HashMap<Box<str>, ToolState>) {
// Changed files
let output = std::process::Command::new("git")
.arg("diff")
.arg("--name-status")
.arg("HEAD")
.arg("HEAD^")
.output();
let output =
helpers::git(None).arg("diff").arg("--name-status").arg("HEAD").arg("HEAD^").output();
let output = match output {
Ok(o) => o,
Err(e) => {
Expand Down Expand Up @@ -324,7 +319,7 @@ fn checkout_toolstate_repo() {
t!(fs::remove_dir_all(TOOLSTATE_DIR));
}

let status = Command::new("git")
let status = helpers::git(None)
.arg("clone")
.arg("--depth=1")
.arg(toolstate_repo())
Expand All @@ -342,7 +337,7 @@ 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 = Command::new("git").arg("config").arg("--global").arg(key).arg(value).status();
let status = helpers::git(None).arg("config").arg("--global").arg(key).arg(value).status();
let success = match status {
Ok(s) => s.success(),
Err(_) => false,
Expand Down Expand Up @@ -406,8 +401,7 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) {
publish_test_results(current_toolstate);

// `git commit` failing means nothing to commit.
let status = t!(Command::new("git")
.current_dir(TOOLSTATE_DIR)
let status = t!(helpers::git(Some(Path::new(TOOLSTATE_DIR)))
.arg("commit")
.arg("-a")
.arg("-m")
Expand All @@ -418,8 +412,7 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) {
break;
}

let status = t!(Command::new("git")
.current_dir(TOOLSTATE_DIR)
let status = t!(helpers::git(Some(Path::new(TOOLSTATE_DIR)))
.arg("push")
.arg("origin")
.arg("master")
Expand All @@ -431,15 +424,13 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) {
}
eprintln!("Sleeping for 3 seconds before retrying push");
std::thread::sleep(std::time::Duration::from_secs(3));
let status = t!(Command::new("git")
.current_dir(TOOLSTATE_DIR)
let status = t!(helpers::git(Some(Path::new(TOOLSTATE_DIR)))
.arg("fetch")
.arg("origin")
.arg("master")
.status());
assert!(status.success());
let status = t!(Command::new("git")
.current_dir(TOOLSTATE_DIR)
let status = t!(helpers::git(Some(Path::new(TOOLSTATE_DIR)))
.arg("reset")
.arg("--hard")
.arg("origin/master")
Expand All @@ -458,7 +449,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!(std::process::Command::new("git").arg("rev-parse").arg("HEAD").output());
let commit = t!(helpers::git(None).arg("rev-parse").arg("HEAD").output());
let commit = t!(String::from_utf8(commit.stdout));

let toolstate_serialized = t!(serde_json::to_string(&current_toolstate));
Expand Down
30 changes: 11 additions & 19 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::core::build_steps::llvm;
use crate::core::config::flags::{Color, Flags, Warnings};
use crate::utils::cache::{Interned, INTERNER};
use crate::utils::channel::{self, GitInfo};
use crate::utils::helpers::{exe, output, t};
use crate::utils::helpers::{self, exe, output, t};
use build_helper::exit;
use serde::{Deserialize, Deserializer};
use serde_derive::Deserialize;
Expand Down Expand Up @@ -1248,7 +1248,7 @@ impl Config {

// Infer the source directory. This is non-trivial because we want to support a downloaded bootstrap binary,
// running on a completely different machine from where it was compiled.
let mut cmd = Command::new("git");
let mut cmd = helpers::git(None);
// NOTE: we cannot support running from outside the repository because the only other path we have available
// is set at compile time, which can be wrong if bootstrap was downloaded rather than compiled locally.
// We still support running outside the repository if we find we aren't in a git directory.
Expand Down Expand Up @@ -2090,15 +2090,6 @@ impl Config {
build_helper::util::try_run(cmd, self.is_verbose())
}

/// A git invocation which runs inside the source directory.
///
/// Use this rather than `Command::new("git")` in order to support out-of-tree builds.
pub(crate) fn git(&self) -> Command {
let mut git = Command::new("git");
git.current_dir(&self.src);
git
}

pub(crate) fn test_args(&self) -> Vec<&str> {
let mut test_args = match self.cmd {
Subcommand::Test { ref test_args, .. }
Expand Down Expand Up @@ -2130,7 +2121,7 @@ impl Config {
"`Config::read_file_by_commit` is not supported in non-git sources."
);

let mut git = self.git();
let mut git = helpers::git(Some(&self.src));
git.arg("show").arg(format!("{commit}:{}", file.to_str().unwrap()));
output(&mut git)
}
Expand Down Expand Up @@ -2436,15 +2427,16 @@ impl Config {
};

// Handle running from a directory other than the top level
let top_level = output(self.git().args(["rev-parse", "--show-toplevel"]));
let top_level =
output(helpers::git(Some(&self.src)).args(["rev-parse", "--show-toplevel"]));
let top_level = top_level.trim_end();
let compiler = format!("{top_level}/compiler/");
let library = format!("{top_level}/library/");

// 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(
self.git()
helpers::git(Some(&self.src))
.arg("rev-list")
.arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email))
.args(["-n1", "--first-parent", "HEAD"]),
Expand All @@ -2459,8 +2451,7 @@ impl Config {
}

// Warn if there were changes to the compiler or standard library since the ancestor commit.
let has_changes = !t!(self
.git()
let has_changes = !t!(helpers::git(Some(&self.src))
.args(["diff-index", "--quiet", commit, "--", &compiler, &library])
.status())
.success();
Expand Down Expand Up @@ -2533,13 +2524,14 @@ impl Config {
if_unchanged: bool,
) -> Option<String> {
// Handle running from a directory other than the top level
let top_level = output(self.git().args(["rev-parse", "--show-toplevel"]));
let top_level =
output(helpers::git(Some(&self.src)).args(["rev-parse", "--show-toplevel"]));
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(
self.git()
helpers::git(Some(&self.src))
.arg("rev-list")
.arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email))
.args(["-n1", "--first-parent", "HEAD"]),
Expand All @@ -2554,7 +2546,7 @@ impl Config {
}

// Warn if there were changes to the compiler or standard library since the ancestor commit.
let mut git = self.git();
let mut git = helpers::git(Some(&self.src));
git.args(["diff-index", "--quiet", commit, "--"]);

for path in modified_paths {
Expand Down
Loading

0 comments on commit 17a7ab7

Please sign in to comment.