Skip to content

Commit

Permalink
Add tidy check to deny merge commits
Browse files Browse the repository at this point in the history
This will prevent users with the pre-push hook from pushing a merge
commit.

Exceptions are added for subtree updates. These exceptions are a little
hacky and may be non-exhaustive but can be extended in the future.

Checkout `master` branch in CI

Add `build_helper` crate to share code between tidy and bootstrap
  • Loading branch information
Noratrieb committed Dec 31, 2022
1 parent 96c1f33 commit e41d603
Show file tree
Hide file tree
Showing 18 changed files with 195 additions and 60 deletions.
15 changes: 15 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ jobs:
uses: actions/checkout@v3
with:
fetch-depth: 2
- name: "checkout the `master` branch for tidy"
uses: actions/checkout@v3
with:
ref: master
fetch-depth: 1
- name: configure the PR in which the error message will be posted
run: "echo \"[CI_PR_NUMBER=$num]\""
env:
Expand Down Expand Up @@ -485,6 +490,11 @@ jobs:
uses: actions/checkout@v3
with:
fetch-depth: 2
- name: "checkout the `master` branch for tidy"
uses: actions/checkout@v3
with:
ref: master
fetch-depth: 1
- name: configure the PR in which the error message will be posted
run: "echo \"[CI_PR_NUMBER=$num]\""
env:
Expand Down Expand Up @@ -600,6 +610,11 @@ jobs:
uses: actions/checkout@v3
with:
fetch-depth: 2
- name: "checkout the `master` branch for tidy"
uses: actions/checkout@v3
with:
ref: master
fetch-depth: 1
- name: configure the PR in which the error message will be posted
run: "echo \"[CI_PR_NUMBER=$num]\""
env:
Expand Down
5 changes: 5 additions & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,10 @@ dependencies = [
"toml",
]

[[package]]
name = "build_helper"
version = "0.1.0"

[[package]]
name = "bump-stage0"
version = "0.1.0"
Expand Down Expand Up @@ -5304,6 +5308,7 @@ dependencies = [
name = "tidy"
version = "0.1.0"
dependencies = [
"build_helper",
"cargo_metadata 0.14.0",
"ignore",
"lazy_static",
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ members = [
"library/std",
"library/test",
"src/rustdoc-json-types",
"src/tools/build_helper",
"src/tools/cargotest",
"src/tools/clippy",
"src/tools/clippy/clippy_dev",
Expand Down
5 changes: 5 additions & 0 deletions src/bootstrap/Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ dependencies = [
name = "bootstrap"
version = "0.0.0"
dependencies = [
"build_helper",
"cc",
"cmake",
"fd-lock",
Expand Down Expand Up @@ -70,6 +71,10 @@ dependencies = [
"regex-automata",
]

[[package]]
name = "build_helper"
version = "0.1.0"

[[package]]
name = "cc"
version = "1.0.73"
Expand Down
1 change: 1 addition & 0 deletions src/bootstrap/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ path = "bin/sccache-plus-cl.rs"
test = false

[dependencies]
build_helper = { path = "../tools/build_helper" }
cmake = "0.1.38"
fd-lock = "3.0.8"
filetime = "0.2"
Expand Down
30 changes: 1 addition & 29 deletions src/bootstrap/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use crate::builder::Builder;
use crate::util::{output, program_out_of_date, t};
use build_helper::git::get_rust_lang_rust_remote;
use ignore::WalkBuilder;
use std::collections::VecDeque;
use std::path::{Path, PathBuf};
Expand Down Expand Up @@ -95,35 +96,6 @@ fn get_modified_rs_files(build: &Builder<'_>) -> Option<Vec<String>> {
)
}

/// Finds the remote for rust-lang/rust.
/// For example for these remotes it will return `upstream`.
/// ```text
/// origin https://github.com/Nilstrieb/rust.git (fetch)
/// origin https://github.com/Nilstrieb/rust.git (push)
/// upstream https://github.com/rust-lang/rust (fetch)
/// upstream https://github.com/rust-lang/rust (push)
/// ```
fn get_rust_lang_rust_remote() -> Result<String, String> {
let mut git = Command::new("git");
git.args(["config", "--local", "--get-regex", "remote\\..*\\.url"]);

let output = git.output().map_err(|err| format!("{err:?}"))?;
if !output.status.success() {
return Err("failed to execute git config command".to_owned());
}

let stdout = String::from_utf8(output.stdout).map_err(|err| format!("{err:?}"))?;

let rust_lang_remote = stdout
.lines()
.find(|remote| remote.contains("rust-lang"))
.ok_or_else(|| "rust-lang/rust remote not found".to_owned())?;

let remote_name =
rust_lang_remote.split('.').nth(1).ok_or_else(|| "remote name not found".to_owned())?;
Ok(remote_name.into())
}

#[derive(serde::Deserialize)]
struct RustfmtConfig {
ignore: Vec<String>,
Expand Down
3 changes: 2 additions & 1 deletion src/bootstrap/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ use std::path::{Path, PathBuf};
use std::process::Command;
use std::str;

use build_helper::ci::CiEnv;
use channel::GitInfo;
use config::{DryRun, Target};
use filetime::FileTime;
Expand All @@ -121,7 +122,7 @@ use once_cell::sync::OnceCell;
use crate::builder::Kind;
use crate::config::{LlvmLibunwind, TargetSelection};
use crate::util::{
exe, libdir, mtime, output, run, run_suppressed, symlink_dir, try_run_suppressed, CiEnv,
exe, libdir, mtime, output, run, run_suppressed, symlink_dir, try_run_suppressed,
};

mod bolt;
Expand Down
4 changes: 3 additions & 1 deletion src/bootstrap/native.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ use crate::util::get_clang_cl_resource_dir;
use crate::util::{self, exe, output, t, up_to_date};
use crate::{CLang, GitRepo};

use build_helper::ci::CiEnv;

#[derive(Clone)]
pub struct LlvmResult {
/// Path to llvm-config binary.
Expand Down Expand Up @@ -217,7 +219,7 @@ pub(crate) fn is_ci_llvm_available(config: &Config, asserts: bool) -> bool {
return false;
}

if crate::util::CiEnv::is_ci() {
if CiEnv::is_ci() {
// We assume we have access to git, so it's okay to unconditionally pass
// `true` here.
let llvm_sha = detect_llvm_sha(config, true);
Expand Down
29 changes: 0 additions & 29 deletions src/bootstrap/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,35 +255,6 @@ pub enum CiEnv {
GitHubActions,
}

impl CiEnv {
/// Obtains the current CI environment.
pub fn current() -> CiEnv {
if env::var("TF_BUILD").map_or(false, |e| e == "True") {
CiEnv::AzurePipelines
} else if env::var("GITHUB_ACTIONS").map_or(false, |e| e == "true") {
CiEnv::GitHubActions
} else {
CiEnv::None
}
}

pub fn is_ci() -> bool {
Self::current() != CiEnv::None
}

/// If in a CI environment, forces the command to run with colors.
pub fn force_coloring_in_ci(self, cmd: &mut Command) {
if self != CiEnv::None {
// Due to use of stamp/docker, the output stream of rustbuild is not
// a TTY in CI, so coloring is by-default turned off.
// The explicit `TERM=xterm` environment is needed for
// `--color always` to actually work. This env var was lost when
// compiling through the Makefile. Very strange.
cmd.env("TERM", "xterm").args(&["--color", "always"]);
}
}
}

pub fn forcing_clang_based_tests() -> bool {
if let Some(var) = env::var_os("RUSTBUILD_FORCE_CLANG_BASED_TESTS") {
match &var.to_string_lossy().to_lowercase()[..] {
Expand Down
6 changes: 6 additions & 0 deletions src/ci/github-actions/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,12 @@ x--expand-yaml-anchors--remove:
with:
fetch-depth: 2

- name: checkout the `master` branch for tidy
uses: actions/checkout@v3
with:
ref: master
fetch-depth: 1

# Rust Log Analyzer can't currently detect the PR number of a GitHub
# Actions build on its own, so a hint in the log message is needed to
# point it in the right direction.
Expand Down
8 changes: 8 additions & 0 deletions src/tools/build_helper/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[package]
name = "build_helper"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
40 changes: 40 additions & 0 deletions src/tools/build_helper/src/ci.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
use std::process::Command;

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum CiEnv {
/// Not a CI environment.
None,
/// The Azure Pipelines environment, for Linux (including Docker), Windows, and macOS builds.
AzurePipelines,
/// The GitHub Actions environment, for Linux (including Docker), Windows and macOS builds.
GitHubActions,
}

impl CiEnv {
/// Obtains the current CI environment.
pub fn current() -> CiEnv {
if std::env::var("TF_BUILD").map_or(false, |e| e == "True") {
CiEnv::AzurePipelines
} else if std::env::var("GITHUB_ACTIONS").map_or(false, |e| e == "true") {
CiEnv::GitHubActions
} else {
CiEnv::None
}
}

pub fn is_ci() -> bool {
Self::current() != CiEnv::None
}

/// If in a CI environment, forces the command to run with colors.
pub fn force_coloring_in_ci(self, cmd: &mut Command) {
if self != CiEnv::None {
// Due to use of stamp/docker, the output stream of rustbuild is not
// a TTY in CI, so coloring is by-default turned off.
// The explicit `TERM=xterm` environment is needed for
// `--color always` to actually work. This env var was lost when
// compiling through the Makefile. Very strange.
cmd.env("TERM", "xterm").args(&["--color", "always"]);
}
}
}
30 changes: 30 additions & 0 deletions src/tools/build_helper/src/git.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
use std::process::Command;

/// Finds the remote for rust-lang/rust.
/// For example for these remotes it will return `upstream`.
/// ```text
/// origin https://github.com/Nilstrieb/rust.git (fetch)
/// origin https://github.com/Nilstrieb/rust.git (push)
/// upstream https://github.com/rust-lang/rust (fetch)
/// upstream https://github.com/rust-lang/rust (push)
/// ```
pub fn get_rust_lang_rust_remote() -> Result<String, String> {
let mut git = Command::new("git");
git.args(["config", "--local", "--get-regex", "remote\\..*\\.url"]);

let output = git.output().map_err(|err| format!("{err:?}"))?;
if !output.status.success() {
return Err("failed to execute git config command".to_owned());
}

let stdout = String::from_utf8(output.stdout).map_err(|err| format!("{err:?}"))?;

let rust_lang_remote = stdout
.lines()
.find(|remote| remote.contains("rust-lang"))
.ok_or_else(|| "rust-lang/rust remote not found".to_owned())?;

let remote_name =
rust_lang_remote.split('.').nth(1).ok_or_else(|| "remote name not found".to_owned())?;
Ok(remote_name.into())
}
2 changes: 2 additions & 0 deletions src/tools/build_helper/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pub mod ci;
pub mod git;
1 change: 1 addition & 0 deletions src/tools/tidy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ edition = "2021"
autobins = false

[dependencies]
build_helper = { path = "../build_helper" }
cargo_metadata = "0.14"
regex = "1"
miropt-test-tools = { path = "../miropt-test-tools" }
Expand Down
1 change: 1 addition & 0 deletions src/tools/tidy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ pub mod errors;
pub mod extdeps;
pub mod features;
pub mod mir_opt_tests;
pub mod no_merge;
pub mod pal;
pub mod primitive_docs;
pub mod style;
Expand Down
2 changes: 2 additions & 0 deletions src/tools/tidy/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ fn main() {
check!(alphabetical, &compiler_path);
check!(alphabetical, &library_path);

check!(no_merge, ());

let collected = {
drain_handles(&mut handles);

Expand Down
72 changes: 72 additions & 0 deletions src/tools/tidy/src/no_merge.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
//! This check makes sure that no accidental merge commits are introduced to the repository.
//! It forbids all merge commits that are not caused by rollups/bors or subtree syncs.

use std::process::Command;

use build_helper::ci::CiEnv;
use build_helper::git::get_rust_lang_rust_remote;

macro_rules! try_unwrap_in_ci {
($expr:expr) => {
match $expr {
Ok(value) => value,
Err(err) if CiEnv::is_ci() => {
panic!("Encountered error while testing Git status: {:?}", err)
}
Err(_) => return,
}
};
}

pub fn check(_: (), bad: &mut bool) {
let remote = try_unwrap_in_ci!(get_rust_lang_rust_remote());
let merge_commits = try_unwrap_in_ci!(find_merge_commits(&remote));

let mut bad_merge_commits = merge_commits.lines().filter(|commit| {
!(
// Bors is the ruler of merge commits.
commit.starts_with("Auto merge of") || commit.starts_with("Rollup merge of")
)
});

if let Some(merge) = bad_merge_commits.next() {
tidy_error!(
bad,
"found a merge commit in the history: `{merge}`.
To resolve the issue, see this: https://rustc-dev-guide.rust-lang.org/git.html#i-made-a-merge-commit-by-accident.
If you're doing a subtree sync, add your tool to the list in the code that emitted this error."
);
}
}

/// Runs `git log --merges --format=%s $REMOTE/master..HEAD` and returns all commits
fn find_merge_commits(remote: &str) -> Result<String, String> {
let mut git = Command::new("git");
git.args([
"log",
"--merges",
"--format=%s",
&format!("{remote}/master..HEAD"),
// Ignore subtree syncs. Add your new subtrees here.
":!src/tools/miri",
":!src/tools/rust-analyzer",
":!compiler/rustc_smir",
":!library/portable-simd",
":!compiler/rustc_codegen_gcc",
":!src/tools/rustfmt",
":!compiler/rustc_codegen_cranelift",
":!src/tools/clippy",
]);

let output = git.output().map_err(|err| format!("{err:?}"))?;
if !output.status.success() {
return Err(format!(
"failed to execute git log command: {}",
String::from_utf8_lossy(&output.stderr)
));
}

let stdout = String::from_utf8(output.stdout).map_err(|err| format!("{err:?}"))?;

Ok(stdout)
}

0 comments on commit e41d603

Please sign in to comment.