Skip to content

Commit

Permalink
Rollup merge of #130161 - onur-ozkan:fmt-changed-files, r=Kobzol,Ralf…
Browse files Browse the repository at this point in the history
…Jung

refactor merge base logic and fix `x fmt`

When remote upstream is not configured, using [get_git_modified_files](https://github.com/rust-lang/rust/blob/38e3a5771cefc9362976a605549f8b04d5707311/src/tools/build_helper/src/git.rs#L114) to find modified files fails because [get_rust_lang_rust_remote](https://github.com/rust-lang/rust/blob/38e3a5771cefc9362976a605549f8b04d5707311/src/tools/build_helper/src/git.rs#L46-L48) can not resolve "rust-lang/rust" from the git output. The changes in this PR makes bootstrap to find the latest bors commit, treating it as the "closest upstream commit" so that the change tracker logic can use it to find the diffs.

In addition, [skips formatting](e392454) if there are no modified files.

Fixes #130147
  • Loading branch information
matthiaskrgr authored Sep 11, 2024
2 parents 5107ff4 + 5f32717 commit ff4b3d4
Show file tree
Hide file tree
Showing 13 changed files with 79 additions and 58 deletions.
14 changes: 5 additions & 9 deletions src/bootstrap/src/core/build_steps/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use std::path::{Path, PathBuf};
use std::process::Stdio;
use std::{env, fs, str};

use build_helper::git::get_closest_merge_commit;
use serde_derive::Deserialize;

use crate::core::build_steps::tool::SourceType;
Expand All @@ -26,8 +27,7 @@ use crate::core::builder::{
use crate::core::config::{DebuginfoLevel, LlvmLibunwind, RustcLto, TargetSelection};
use crate::utils::exec::command;
use crate::utils::helpers::{
self, exe, get_clang_cl_resource_dir, get_closest_merge_base_commit, is_debug_info, is_dylib,
symlink_dir, t, up_to_date,
self, exe, get_clang_cl_resource_dir, is_debug_info, is_dylib, symlink_dir, t, up_to_date,
};
use crate::{CLang, Compiler, DependencyType, GitRepo, Mode, LLVM_TOOLS};

Expand Down Expand Up @@ -127,13 +127,9 @@ impl Step for Std {
// the `rust.download-rustc=true` option.
let force_recompile =
if builder.rust_info().is_managed_git_subrepository() && builder.download_rustc() {
let closest_merge_commit = get_closest_merge_base_commit(
Some(&builder.src),
&builder.config.git_config(),
&builder.config.stage0_metadata.config.git_merge_commit_email,
&[],
)
.unwrap();
let closest_merge_commit =
get_closest_merge_commit(Some(&builder.src), &builder.config.git_config(), &[])
.unwrap();

// Check if `library` has changes (returns false otherwise)
!t!(helpers::git(Some(&builder.src))
Expand Down
5 changes: 5 additions & 0 deletions src/bootstrap/src/core/build_steps/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,11 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
adjective = Some("modified");
match get_modified_rs_files(build) {
Ok(Some(files)) => {
if files.is_empty() {
println!("fmt info: No modified files detected for formatting.");
return;
}

for file in files {
override_builder.add(&format!("/{file}")).expect(&file);
}
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 @@ -16,6 +16,7 @@ use std::sync::OnceLock;
use std::{env, io};

use build_helper::ci::CiEnv;
use build_helper::git::get_closest_merge_commit;

use crate::core::builder::{Builder, RunConfig, ShouldRun, Step};
use crate::core::config::{Config, TargetSelection};
Expand Down Expand Up @@ -153,10 +154,9 @@ pub fn prebuilt_llvm_config(builder: &Builder<'_>, target: TargetSelection) -> L
/// This retrieves the LLVM sha we *want* to use, according to git history.
pub(crate) fn detect_llvm_sha(config: &Config, is_git: bool) -> String {
let llvm_sha = if is_git {
helpers::get_closest_merge_base_commit(
get_closest_merge_commit(
Some(&config.src),
&config.git_config(),
&config.stage0_metadata.config.git_merge_commit_email,
&[
config.src.join("src/llvm-project"),
config.src.join("src/bootstrap/download-ci-llvm-stamp"),
Expand Down
1 change: 1 addition & 0 deletions src/bootstrap/src/core/build_steps/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub fn suggest(builder: &Builder<'_>, run: bool) {
.tool_cmd(Tool::SuggestTests)
.env("SUGGEST_TESTS_GIT_REPOSITORY", git_config.git_repository)
.env("SUGGEST_TESTS_NIGHTLY_BRANCH", git_config.nightly_branch)
.env("SUGGEST_TESTS_MERGE_COMMIT_EMAIL", git_config.git_merge_commit_email)
.run_capture_stdout(builder)
.stdout();

Expand Down
1 change: 1 addition & 0 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2098,6 +2098,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
let git_config = builder.config.git_config();
cmd.arg("--git-repository").arg(git_config.git_repository);
cmd.arg("--nightly-branch").arg(git_config.nightly_branch);
cmd.arg("--git-merge-commit-email").arg(git_config.git_merge_commit_email);
cmd.force_coloring_in_ci();

#[cfg(feature = "build-metrics")]
Expand Down
7 changes: 4 additions & 3 deletions src/bootstrap/src/core/build_steps/tool.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
use std::path::PathBuf;
use std::{env, fs};

use build_helper::git::get_closest_merge_commit;

use crate::core::build_steps::compile;
use crate::core::build_steps::toolstate::ToolState;
use crate::core::builder;
use crate::core::builder::{Builder, Cargo as CargoCommand, RunConfig, ShouldRun, Step};
use crate::core::config::TargetSelection;
use crate::utils::channel::GitInfo;
use crate::utils::exec::{command, BootstrapCommand};
use crate::utils::helpers::{add_dylib_path, exe, get_closest_merge_base_commit, git, t};
use crate::utils::helpers::{add_dylib_path, exe, git, t};
use crate::{gha, Compiler, Kind, Mode};

#[derive(Debug, Clone, Hash, PartialEq, Eq)]
Expand Down Expand Up @@ -576,10 +578,9 @@ impl Step for Rustdoc {
&& target_compiler.stage > 0
&& builder.rust_info().is_managed_git_subrepository()
{
let commit = get_closest_merge_base_commit(
let commit = get_closest_merge_commit(
Some(&builder.config.src),
&builder.config.git_config(),
&builder.config.stage0_metadata.config.git_merge_commit_email,
&[],
)
.unwrap();
Expand Down
21 changes: 5 additions & 16 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use std::sync::OnceLock;
use std::{cmp, env, fs};

use build_helper::exit;
use build_helper::git::{output_result, GitConfig};
use build_helper::git::{get_closest_merge_commit, output_result, GitConfig};
use serde::{Deserialize, Deserializer};
use serde_derive::Deserialize;

Expand All @@ -24,7 +24,7 @@ pub use crate::core::config::flags::Subcommand;
use crate::core::config::flags::{Color, Flags, Warnings};
use crate::utils::cache::{Interned, INTERNER};
use crate::utils::channel::{self, GitInfo};
use crate::utils::helpers::{self, exe, get_closest_merge_base_commit, output, t};
use crate::utils::helpers::{self, exe, output, t};

macro_rules! check_ci_llvm {
($name:expr) => {
Expand Down Expand Up @@ -2512,6 +2512,7 @@ impl Config {
GitConfig {
git_repository: &self.stage0_metadata.config.git_repository,
nightly_branch: &self.stage0_metadata.config.nightly_branch,
git_merge_commit_email: &self.stage0_metadata.config.git_merge_commit_email,
}
}

Expand Down Expand Up @@ -2688,13 +2689,7 @@ impl Config {

// Look for a version to compare to based on the current commit.
// Only commits merged by bors will have CI artifacts.
let commit = get_closest_merge_base_commit(
Some(&self.src),
&self.git_config(),
&self.stage0_metadata.config.git_merge_commit_email,
&[],
)
.unwrap();
let commit = get_closest_merge_commit(Some(&self.src), &self.git_config(), &[]).unwrap();
if commit.is_empty() {
println!("ERROR: could not find commit hash for downloading rustc");
println!("HELP: maybe your repository history is too shallow?");
Expand Down Expand Up @@ -2786,13 +2781,7 @@ impl Config {
) -> Option<String> {
// Look for a version to compare to based on the current commit.
// Only commits merged by bors will have CI artifacts.
let commit = get_closest_merge_base_commit(
Some(&self.src),
&self.git_config(),
&self.stage0_metadata.config.git_merge_commit_email,
&[],
)
.unwrap();
let commit = get_closest_merge_commit(Some(&self.src), &self.git_config(), &[]).unwrap();
if commit.is_empty() {
println!("error: could not find commit hash for downloading components from CI");
println!("help: maybe your repository history is too shallow?");
Expand Down
23 changes: 0 additions & 23 deletions src/bootstrap/src/utils/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use std::sync::OnceLock;
use std::time::{Instant, SystemTime, UNIX_EPOCH};
use std::{env, fs, io, str};

use build_helper::git::{get_git_merge_base, output_result, GitConfig};
use build_helper::util::fail;

use crate::core::builder::Builder;
Expand Down Expand Up @@ -523,28 +522,6 @@ pub fn git(source_dir: Option<&Path>) -> BootstrapCommand {
git
}

/// Returns the closest commit available from upstream for the given `author` and `target_paths`.
///
/// If it fails to find the commit from upstream using `git merge-base`, fallbacks to HEAD.
pub fn get_closest_merge_base_commit(
source_dir: Option<&Path>,
config: &GitConfig<'_>,
author: &str,
target_paths: &[PathBuf],
) -> Result<String, String> {
let mut git = git(source_dir);

let merge_base = get_git_merge_base(config, source_dir).unwrap_or_else(|_| "HEAD".into());

git.args(["rev-list", &format!("--author={author}"), "-n1", "--first-parent", &merge_base]);

if !target_paths.is_empty() {
git.arg("--").args(target_paths);
}

Ok(output_result(git.as_command_mut())?.trim().to_owned())
}

/// Sets the file times for a given file at `path`.
pub fn set_file_times<P: AsRef<Path>>(path: P, times: fs::FileTimes) -> io::Result<()> {
// Windows requires file to be writable to modify file times. But on Linux CI the file does not
Expand Down
43 changes: 40 additions & 3 deletions src/tools/build_helper/src/git.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use std::path::Path;
use std::path::{Path, PathBuf};
use std::process::{Command, Stdio};

pub struct GitConfig<'a> {
pub git_repository: &'a str,
pub nightly_branch: &'a str,
pub git_merge_commit_email: &'a str,
}

/// Runs a command and returns the output
Expand Down Expand Up @@ -95,7 +96,11 @@ pub fn updated_master_branch(
Err("Cannot find any suitable upstream master branch".to_owned())
}

pub fn get_git_merge_base(
/// Finds the nearest merge commit by comparing the local `HEAD` with the upstream branch's state.
/// To work correctly, the upstream remote must be properly configured using `git remote add <name> <url>`.
/// In most cases `get_closest_merge_commit` is the function you are looking for as it doesn't require remote
/// to be configured.
fn git_upstream_merge_base(
config: &GitConfig<'_>,
git_dir: Option<&Path>,
) -> Result<String, String> {
Expand All @@ -107,6 +112,38 @@ pub fn get_git_merge_base(
Ok(output_result(git.arg("merge-base").arg(&updated_master).arg("HEAD"))?.trim().to_owned())
}

/// Searches for the nearest merge commit in the repository that also exists upstream.
///
/// If it fails to find the upstream remote, it then looks for the most recent commit made
/// by the merge bot by matching the author's email address with the merge bot's email.
pub fn get_closest_merge_commit(
git_dir: Option<&Path>,
config: &GitConfig<'_>,
target_paths: &[PathBuf],
) -> Result<String, String> {
let mut git = Command::new("git");

if let Some(git_dir) = git_dir {
git.current_dir(git_dir);
}

let merge_base = git_upstream_merge_base(config, git_dir).unwrap_or_else(|_| "HEAD".into());

git.args([
"rev-list",
&format!("--author={}", config.git_merge_commit_email),
"-n1",
"--first-parent",
&merge_base,
]);

if !target_paths.is_empty() {
git.arg("--").args(target_paths);
}

Ok(output_result(&mut git)?.trim().to_owned())
}

/// Returns the files that have been modified in the current branch compared to the master branch.
/// The `extensions` parameter can be used to filter the files by their extension.
/// Does not include removed files.
Expand All @@ -116,7 +153,7 @@ pub fn get_git_modified_files(
git_dir: Option<&Path>,
extensions: &[&str],
) -> Result<Option<Vec<String>>, String> {
let merge_base = get_git_merge_base(config, git_dir)?;
let merge_base = get_closest_merge_commit(git_dir, config, &[])?;

let mut git = Command::new("git");
if let Some(git_dir) = git_dir {
Expand Down
7 changes: 6 additions & 1 deletion src/tools/compiletest/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ pub struct Config {
// Needed both to construct build_helper::git::GitConfig
pub git_repository: String,
pub nightly_branch: String,
pub git_merge_commit_email: String,

/// True if the profiler runtime is enabled for this target.
/// Used by the "needs-profiler-support" header in test files.
Expand Down Expand Up @@ -461,7 +462,11 @@ impl Config {
}

pub fn git_config(&self) -> GitConfig<'_> {
GitConfig { git_repository: &self.git_repository, nightly_branch: &self.nightly_branch }
GitConfig {
git_repository: &self.git_repository,
nightly_branch: &self.nightly_branch,
git_merge_commit_email: &self.git_merge_commit_email,
}
}
}

Expand Down
1 change: 1 addition & 0 deletions src/tools/compiletest/src/header/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ impl ConfigBuilder {
self.target.as_deref().unwrap_or("x86_64-unknown-linux-gnu"),
"--git-repository=",
"--nightly-branch=",
"--git-merge-commit-email=",
];
let mut args: Vec<String> = args.iter().map(ToString::to_string).collect();

Expand Down
9 changes: 8 additions & 1 deletion src/tools/compiletest/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,13 @@ pub fn parse_config(args: Vec<String>) -> Config {
)
.optopt("", "edition", "default Rust edition", "EDITION")
.reqopt("", "git-repository", "name of the git repository", "ORG/REPO")
.reqopt("", "nightly-branch", "name of the git branch for nightly", "BRANCH");
.reqopt("", "nightly-branch", "name of the git branch for nightly", "BRANCH")
.reqopt(
"",
"git-merge-commit-email",
"email address used for finding merge commits",
"EMAIL",
);

let (argv0, args_) = args.split_first().unwrap();
if args.len() == 1 || args[1] == "-h" || args[1] == "--help" {
Expand Down Expand Up @@ -346,6 +352,7 @@ pub fn parse_config(args: Vec<String>) -> Config {

git_repository: matches.opt_str("git-repository").unwrap(),
nightly_branch: matches.opt_str("nightly-branch").unwrap(),
git_merge_commit_email: matches.opt_str("git-merge-commit-email").unwrap(),

profiler_support: matches.opt_present("profiler-support"),
}
Expand Down
1 change: 1 addition & 0 deletions src/tools/suggest-tests/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ fn main() -> ExitCode {
&GitConfig {
git_repository: &env("SUGGEST_TESTS_GIT_REPOSITORY"),
nightly_branch: &env("SUGGEST_TESTS_NIGHTLY_BRANCH"),
git_merge_commit_email: &env("SUGGEST_TESTS_MERGE_COMMIT_EMAIL"),
},
None,
&Vec::new(),
Expand Down

0 comments on commit ff4b3d4

Please sign in to comment.