From c6c29bf146634cc81eed34567842f67a43c92240 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Sat, 15 Feb 2025 03:12:55 +0800 Subject: [PATCH 1/5] compiletest: remove `needs-git-hash` directive Previously, `tests/run-make/version-verbose-commit-hash` relied upon information from *bootstrap* to determine if git-hash was available. However, this is flawed: if bootstrap's git-hash logic fails, the test that tries to check that the rustc is built with git-hash information may never run, because it's gated via `//@ needs-git-hash` that trusts bootstrap's information! Instead, we have to bypass bootstrap and communicate between CI jobs and the run-make test directly. --- src/tools/compiletest/src/common.rs | 3 --- src/tools/compiletest/src/directive-list.rs | 1 - src/tools/compiletest/src/header/needs.rs | 5 ----- src/tools/compiletest/src/header/tests.rs | 18 ------------------ src/tools/compiletest/src/lib.rs | 6 ------ 5 files changed, 33 deletions(-) diff --git a/src/tools/compiletest/src/common.rs b/src/tools/compiletest/src/common.rs index cde4f7a665cbf..7e1c225ad3aa7 100644 --- a/src/tools/compiletest/src/common.rs +++ b/src/tools/compiletest/src/common.rs @@ -357,9 +357,6 @@ pub struct Config { /// The current Rust channel pub channel: String, - /// Whether adding git commit information such as the commit hash has been enabled for building - pub git_hash: bool, - /// The default Rust edition pub edition: Option, diff --git a/src/tools/compiletest/src/directive-list.rs b/src/tools/compiletest/src/directive-list.rs index a7ac875d0a36c..13aceff1eb17f 100644 --- a/src/tools/compiletest/src/directive-list.rs +++ b/src/tools/compiletest/src/directive-list.rs @@ -135,7 +135,6 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[ "needs-dynamic-linking", "needs-enzyme", "needs-force-clang-based-tests", - "needs-git-hash", "needs-llvm-components", "needs-llvm-zstd", "needs-profiler-runtime", diff --git a/src/tools/compiletest/src/header/needs.rs b/src/tools/compiletest/src/header/needs.rs index 12f0790fb1040..aaf342c1ee372 100644 --- a/src/tools/compiletest/src/header/needs.rs +++ b/src/tools/compiletest/src/header/needs.rs @@ -129,11 +129,6 @@ pub(super) fn handle_needs( condition: cache.dlltool, ignore_reason: "ignored when dlltool for the current architecture is not present", }, - Need { - name: "needs-git-hash", - condition: config.git_hash, - ignore_reason: "ignored when git hashes have been omitted for building", - }, Need { name: "needs-dynamic-linking", condition: config.target_cfg().dynamic_linking, diff --git a/src/tools/compiletest/src/header/tests.rs b/src/tools/compiletest/src/header/tests.rs index 522d340b678f9..d51eaecef32dc 100644 --- a/src/tools/compiletest/src/header/tests.rs +++ b/src/tools/compiletest/src/header/tests.rs @@ -75,7 +75,6 @@ struct ConfigBuilder { stage: Option, stage_id: Option, llvm_version: Option, - git_hash: bool, system_llvm: bool, profiler_runtime: bool, rustc_debug_assertions: bool, @@ -118,11 +117,6 @@ impl ConfigBuilder { self } - fn git_hash(&mut self, b: bool) -> &mut Self { - self.git_hash = b; - self - } - fn system_llvm(&mut self, s: bool) -> &mut Self { self.system_llvm = s; self @@ -184,9 +178,6 @@ impl ConfigBuilder { args.push(llvm_version.clone()); } - if self.git_hash { - args.push("--git-hash".to_owned()); - } if self.system_llvm { args.push("--system-llvm".to_owned()); } @@ -427,15 +418,6 @@ fn debugger() { assert!(check_ignore(&config, "//@ ignore-lldb")); } -#[test] -fn git_hash() { - let config: Config = cfg().git_hash(false).build(); - assert!(check_ignore(&config, "//@ needs-git-hash")); - - let config: Config = cfg().git_hash(true).build(); - assert!(!check_ignore(&config, "//@ needs-git-hash")); -} - #[test] fn sanitizers() { // Target that supports all sanitizers: diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index d0a83cab9cd99..097f81206a35f 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -166,11 +166,6 @@ pub fn parse_config(args: Vec) -> Config { .optflag("", "profiler-runtime", "is the profiler runtime enabled for this target") .optflag("h", "help", "show this message") .reqopt("", "channel", "current Rust channel", "CHANNEL") - .optflag( - "", - "git-hash", - "run tests which rely on commit version being compiled into the binaries", - ) .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") @@ -381,7 +376,6 @@ pub fn parse_config(args: Vec) -> Config { has_html_tidy, has_enzyme, channel: matches.opt_str("channel").unwrap(), - git_hash: matches.opt_present("git-hash"), edition: matches.opt_str("edition"), cc: matches.opt_str("cc").unwrap(), From 5afd2d9e492d962ba4a6d5e93e05d94ccd40cb64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Sat, 15 Feb 2025 03:31:59 +0800 Subject: [PATCH 2/5] bootstrap: no longer pass `--git-hash` --- src/bootstrap/src/core/build_steps/test.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs index 26ed0e5deaa05..8bf5877c57115 100644 --- a/src/bootstrap/src/core/build_steps/test.rs +++ b/src/bootstrap/src/core/build_steps/test.rs @@ -2125,10 +2125,6 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the cmd.arg("--channel").arg(&builder.config.channel); - if !builder.config.omit_git_hash { - cmd.arg("--git-hash"); - } - 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); From 74d823aeb0e78daac43aabd855a80e8f3af75150 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Sat, 15 Feb 2025 03:30:52 +0800 Subject: [PATCH 3/5] tests: update `version-verbose-commit-hash` - No longer gate by `//@ needs-git-hash`. Instead, check for `COMPILETEST_HAS_GIT_HASH=1` env var being set. - Document the env var and reason for bypassing bootstrap. --- .../version-verbose-commit-hash/rmake.rs | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/tests/run-make/version-verbose-commit-hash/rmake.rs b/tests/run-make/version-verbose-commit-hash/rmake.rs index 733c0e2cdb14f..04b2f7f0de49f 100644 --- a/tests/run-make/version-verbose-commit-hash/rmake.rs +++ b/tests/run-make/version-verbose-commit-hash/rmake.rs @@ -1,13 +1,22 @@ -// `--version --verbose` should display the git-commit hashes of rustc and rustdoc, but this -// functionality was lost due to #104184. After this feature was returned by #109981, this -// test ensures it will not be broken again. -// See https://github.com/rust-lang/rust/issues/107094 - -//@ needs-git-hash +//! `--version --verbose` should display the git-commit hashes of rustc and rustdoc, but this +//! functionality was lost due to #104184. After this feature was returned by #109981, this +//! test ensures it will not be broken again. +//! See . +//! +//! # Important note +//! +//! This test is **not** gated by compiletest, and **cannot** trust bootstrap's git-hash logic e.g. +//! if bootstrap reports git-hash is available yet the built rustc doesn't actually have a hash. It +//! must directly communicate with CI, and gate it being run on an env var expected to be set in CI +//! (or that env var being set locally), `COMPILETEST_HAS_GIT_HASH=1`. use run_make_support::{bare_rustc, bare_rustdoc, regex}; fn main() { + if !std::env::var("COMPILETEST_HAS_GIT_HASH").is_ok_and(|v| v == "1") { + return; + } + let out_rustc = bare_rustc().arg("--version").arg("--verbose").run().stdout_utf8().to_lowercase(); let out_rustdoc = @@ -15,6 +24,10 @@ fn main() { let re = regex::Regex::new(r#"commit-hash: [0-9a-f]{40}\ncommit-date: [0-9]{4}-[0-9]{2}-[0-9]{2}"#) .unwrap(); + + println!("rustc:\n{}", out_rustc); + println!("rustdoc:\n{}", out_rustdoc); + assert!(re.is_match(&out_rustc)); assert!(re.is_match(&out_rustdoc)); } From ecfbe3ec5b26c40262c61d05cf31ac5ce2e76355 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Sat, 15 Feb 2025 03:35:05 +0800 Subject: [PATCH 4/5] rustc-dev-guide: document `COMPILETEST_HAS_GIT_HASH=1` --- src/doc/rustc-dev-guide/src/tests/misc.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/doc/rustc-dev-guide/src/tests/misc.md b/src/doc/rustc-dev-guide/src/tests/misc.md index c0288b3dd10cd..cf3651b6c77fc 100644 --- a/src/doc/rustc-dev-guide/src/tests/misc.md +++ b/src/doc/rustc-dev-guide/src/tests/misc.md @@ -38,3 +38,11 @@ fn main() { .run(); } ``` + +## `tests/run-make/version-verbose-commit-hash` + +This is a special `run-make` test that checks if git hash info is available with +`rustc -vV` and `rustdoc -vV`. It must not trust bootstrap's reporting of +whether git hash is available, as bootstrap may be bugged. Instead, this test +directly checks for `COMPILETEST_HAS_GIT_HASH=1` being set in CI, otherwise this +test is a no-op locally. From 80812e94b32aba037aa6e4d7ec48a1fd2e04637e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Sat, 15 Feb 2025 03:36:21 +0800 Subject: [PATCH 5/5] [DO NOT MERGE] reminder: must set env var in CI after #136864