From 80a5adf871c30e8bfdd696d22595b70a584cce3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Wed, 19 Mar 2025 14:31:15 +0100 Subject: [PATCH 1/6] Unify LLVM invalidation path handling Before it was using a different set of paths in different call-sites. --- src/bootstrap/src/core/build_steps/gcc.rs | 2 +- src/bootstrap/src/core/build_steps/llvm.rs | 21 ++++++++++----------- src/bootstrap/src/core/config/config.rs | 5 +++-- src/bootstrap/src/core/config/tests.rs | 3 ++- src/build_helper/src/git.rs | 4 ++-- 5 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/gcc.rs b/src/bootstrap/src/core/build_steps/gcc.rs index b88a5f2bbf13f..48bb5cb8e8761 100644 --- a/src/bootstrap/src/core/build_steps/gcc.rs +++ b/src/bootstrap/src/core/build_steps/gcc.rs @@ -273,7 +273,7 @@ fn detect_gcc_sha(config: &crate::Config, is_git: bool) -> String { get_closest_merge_commit( Some(&config.src), &config.git_config(), - &[config.src.join("src/gcc"), config.src.join("src/bootstrap/download-ci-gcc-stamp")], + &["src/gcc", "src/bootstrap/download-ci-gcc-stamp"], ) .unwrap() } else if let Some(info) = crate::utils::channel::read_commit_info_file(&config.src) { diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index 0ae5256a18fdc..efecca6ebddd5 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -174,20 +174,19 @@ pub fn prebuilt_llvm_config( LlvmBuildStatus::ShouldBuild(Meta { stamp, res, out_dir, root: root.into() }) } +/// Paths whose changes invalidate LLVM downloads. +pub const LLVM_INVALIDATION_PATHS: &[&str] = &[ + "src/llvm-project", + "src/bootstrap/download-ci-llvm-stamp", + // the LLVM shared object file is named `LLVM--rust-{version}-nightly` + "src/version", +]; + /// 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 { - get_closest_merge_commit( - Some(&config.src), - &config.git_config(), - &[ - config.src.join("src/llvm-project"), - config.src.join("src/bootstrap/download-ci-llvm-stamp"), - // the LLVM shared object file is named `LLVM-12-rust-{version}-nightly` - config.src.join("src/version"), - ], - ) - .unwrap() + get_closest_merge_commit(Some(&config.src), &config.git_config(), LLVM_INVALIDATION_PATHS) + .unwrap() } else if let Some(info) = crate::utils::channel::read_commit_info_file(&config.src) { info.sha.trim().to_owned() } else { diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 2b8c1f49afbef..dfd64c3246d2a 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -23,6 +23,7 @@ use tracing::{instrument, span}; use crate::core::build_steps::compile::CODEGEN_BACKEND_PREFIX; use crate::core::build_steps::llvm; +use crate::core::build_steps::llvm::LLVM_INVALIDATION_PATHS; pub use crate::core::config::flags::Subcommand; use crate::core::config::flags::{Color, Flags, Warnings}; use crate::core::download::is_download_ci_available; @@ -3109,9 +3110,9 @@ impl Config { #[cfg(not(test))] self.update_submodule("src/llvm-project"); - // Check for untracked changes in `src/llvm-project`. + // Check for untracked changes in `src/llvm-project` and other important places. let has_changes = self - .last_modified_commit(&["src/llvm-project"], "download-ci-llvm", true) + .last_modified_commit(LLVM_INVALIDATION_PATHS, "download-ci-llvm", true) .is_none(); // Return false if there are untracked changes, otherwise check if CI LLVM is available. diff --git a/src/bootstrap/src/core/config/tests.rs b/src/bootstrap/src/core/config/tests.rs index fb2c52966ebb7..6f21016ea744c 100644 --- a/src/bootstrap/src/core/config/tests.rs +++ b/src/bootstrap/src/core/config/tests.rs @@ -12,6 +12,7 @@ use super::flags::Flags; use super::{ChangeIdWrapper, Config, RUSTC_IF_UNCHANGED_ALLOWED_PATHS}; use crate::core::build_steps::clippy::{LintConfig, get_clippy_rules_in_order}; use crate::core::build_steps::llvm; +use crate::core::build_steps::llvm::LLVM_INVALIDATION_PATHS; use crate::core::config::{LldMode, Target, TargetSelection, TomlConfig}; pub(crate) fn parse(config: &str) -> Config { @@ -41,7 +42,7 @@ fn download_ci_llvm() { let if_unchanged_config = parse("llvm.download-ci-llvm = \"if-unchanged\""); if if_unchanged_config.llvm_from_ci { let has_changes = if_unchanged_config - .last_modified_commit(&["src/llvm-project"], "download-ci-llvm", true) + .last_modified_commit(LLVM_INVALIDATION_PATHS, "download-ci-llvm", true) .is_none(); assert!( diff --git a/src/build_helper/src/git.rs b/src/build_helper/src/git.rs index 9f778a2fd7742..bdec54a774147 100644 --- a/src/build_helper/src/git.rs +++ b/src/build_helper/src/git.rs @@ -1,4 +1,4 @@ -use std::path::{Path, PathBuf}; +use std::path::Path; use std::process::{Command, Stdio}; use crate::ci::CiEnv; @@ -121,7 +121,7 @@ fn git_upstream_merge_base( pub fn get_closest_merge_commit( git_dir: Option<&Path>, config: &GitConfig<'_>, - target_paths: &[PathBuf], + target_paths: &[&str], ) -> Result { let mut git = Command::new("git"); From 9c05758ed47166d14bf073e6941dc95020ff7713 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Wed, 19 Mar 2025 14:39:16 +0100 Subject: [PATCH 2/6] Remove duplicated check for LLVM modifications and disable `download-ci-llvm=true` on CI --- src/bootstrap/src/core/build_steps/llvm.rs | 40 ++-------------------- src/bootstrap/src/core/config/config.rs | 11 ++++-- src/bootstrap/src/core/config/tests.rs | 14 ++++++-- 3 files changed, 23 insertions(+), 42 deletions(-) diff --git a/src/bootstrap/src/core/build_steps/llvm.rs b/src/bootstrap/src/core/build_steps/llvm.rs index efecca6ebddd5..28a8afd9c04ad 100644 --- a/src/bootstrap/src/core/build_steps/llvm.rs +++ b/src/bootstrap/src/core/build_steps/llvm.rs @@ -14,7 +14,6 @@ use std::path::{Path, PathBuf}; use std::sync::OnceLock; use std::{env, fs}; -use build_helper::ci::CiEnv; use build_helper::git::get_closest_merge_commit; #[cfg(feature = "tracing")] use tracing::instrument; @@ -206,10 +205,9 @@ pub(crate) fn detect_llvm_sha(config: &Config, is_git: bool) -> String { /// Returns whether the CI-found LLVM is currently usable. /// -/// This checks both the build triple platform to confirm we're usable at all, -/// and then verifies if the current HEAD matches the detected LLVM SHA head, -/// in which case LLVM is indicated as not available. -pub(crate) fn is_ci_llvm_available(config: &Config, asserts: bool) -> bool { +/// This checks the build triple platform to confirm we're usable at all, and if LLVM +/// with/without assertions is available. +pub(crate) fn is_ci_llvm_available_for_target(config: &Config, asserts: bool) -> bool { // This is currently all tier 1 targets and tier 2 targets with host tools // (since others may not have CI artifacts) // https://doc.rust-lang.org/rustc/platform-support.html#tier-1 @@ -254,41 +252,9 @@ pub(crate) fn is_ci_llvm_available(config: &Config, asserts: bool) -> bool { return false; } - if is_ci_llvm_modified(config) { - eprintln!("Detected LLVM as non-available: running in CI and modified LLVM in this change"); - return false; - } - true } -/// Returns true if we're running in CI with modified LLVM (and thus can't download it) -pub(crate) fn is_ci_llvm_modified(config: &Config) -> bool { - // If not running in a CI environment, return false. - if !config.is_running_on_ci { - return false; - } - - // In rust-lang/rust managed CI, assert the existence of the LLVM submodule. - if CiEnv::is_rust_lang_managed_ci_job() { - assert!( - config.in_tree_llvm_info.is_managed_git_subrepository(), - "LLVM submodule must be fetched in rust-lang/rust managed CI builders." - ); - } - // If LLVM submodule isn't present, skip the change check as it won't work. - else if !config.in_tree_llvm_info.is_managed_git_subrepository() { - return false; - } - - let llvm_sha = detect_llvm_sha(config, true); - let head_sha = crate::output( - helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD").as_command_mut(), - ); - let head_sha = head_sha.trim(); - llvm_sha == head_sha -} - #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub struct Llvm { pub target: TargetSelection, diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index dfd64c3246d2a..b8068373c73f9 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -3116,7 +3116,7 @@ impl Config { .is_none(); // Return false if there are untracked changes, otherwise check if CI LLVM is available. - if has_changes { false } else { llvm::is_ci_llvm_available(self, asserts) } + if has_changes { false } else { llvm::is_ci_llvm_available_for_target(self, asserts) } }; match download_ci_llvm { @@ -3127,8 +3127,15 @@ impl Config { ); } + if b && self.is_running_on_ci { + // On CI, we must always rebuild LLVM if there were any modifications to it + panic!( + "`llvm.download-ci-llvm` cannot be set to `true` on CI. Use `if-unchanged` instead." + ); + } + // If download-ci-llvm=true we also want to check that CI llvm is available - b && llvm::is_ci_llvm_available(self, asserts) + b && llvm::is_ci_llvm_available_for_target(self, asserts) } StringOrBool::String(s) if s == "if-unchanged" => if_unchanged(), StringOrBool::String(other) => { diff --git a/src/bootstrap/src/core/config/tests.rs b/src/bootstrap/src/core/config/tests.rs index 6f21016ea744c..068e237c2cdcd 100644 --- a/src/bootstrap/src/core/config/tests.rs +++ b/src/bootstrap/src/core/config/tests.rs @@ -25,13 +25,21 @@ pub(crate) fn parse(config: &str) -> Config { #[test] fn download_ci_llvm() { let config = parse(""); - let is_available = llvm::is_ci_llvm_available(&config, config.llvm_assertions); + let is_available = llvm::is_ci_llvm_available_for_target(&config, config.llvm_assertions); if is_available { assert!(config.llvm_from_ci); } - let config = parse("llvm.download-ci-llvm = true"); - let is_available = llvm::is_ci_llvm_available(&config, config.llvm_assertions); + let config = Config::parse_inner( + Flags::parse(&[ + "check".to_string(), + "--config=/does/not/exist".to_string(), + "--ci".to_string(), + "false".to_string(), + ]), + |&_| toml::from_str("llvm.download-ci-llvm = true"), + ); + let is_available = llvm::is_ci_llvm_available_for_target(&config, config.llvm_assertions); if is_available { assert!(config.llvm_from_ci); } From 68aaa8d103f8d17dc9fc721a80ceb45d2ec9585d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Wed, 19 Mar 2025 14:40:38 +0100 Subject: [PATCH 3/6] Allow unused code in tests To avoid working around some code being unused in tests due to it being stubbed out with `#[cfg(test)]`. --- src/bootstrap/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 1fba17dcf3087..ec5c0c53baa69 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -15,6 +15,7 @@ //! //! More documentation can be found in each respective module below, and you can //! also check out the `src/bootstrap/README.md` file for more information. +#![cfg_attr(test, allow(unused))] use std::cell::{Cell, RefCell}; use std::collections::{BTreeSet, HashMap, HashSet}; From f53acd17cb172c02bd8ec24aadd4475579ee0b3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 20 Mar 2025 10:06:51 +0100 Subject: [PATCH 4/6] Set `if-unchanged` as the default value for `download-ci-llvm` when we're on CI. --- src/bootstrap/src/core/config/config.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index b8068373c73f9..ffa3c2ddb6d06 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -3097,7 +3097,14 @@ impl Config { download_ci_llvm: Option, asserts: bool, ) -> bool { - let download_ci_llvm = download_ci_llvm.unwrap_or(StringOrBool::Bool(true)); + // We don't ever want to use `true` on CI, as we should not + // download upstream artifacts if there are any local modifications. + let default = if self.is_running_on_ci { + StringOrBool::String("if-unchanged".to_string()) + } else { + StringOrBool::Bool(true) + }; + let download_ci_llvm = download_ci_llvm.unwrap_or(default); let if_unchanged = || { if self.rust_info.is_from_tarball() { From e288faa4b015676829f96f83dc5e124ee7c0b3e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Thu, 20 Mar 2025 13:43:21 +0100 Subject: [PATCH 5/6] Disable CI mode when checking default bootstrap profiles --- .../host-x86_64/mingw-check/check-default-config-profiles.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ci/docker/host-x86_64/mingw-check/check-default-config-profiles.sh b/src/ci/docker/host-x86_64/mingw-check/check-default-config-profiles.sh index d706927d6d950..0c85d4b449db0 100755 --- a/src/ci/docker/host-x86_64/mingw-check/check-default-config-profiles.sh +++ b/src/ci/docker/host-x86_64/mingw-check/check-default-config-profiles.sh @@ -8,5 +8,6 @@ config_dir="../src/bootstrap/defaults" # Loop through each configuration file in the directory for config_file in "$config_dir"/*.toml; do - python3 ../x.py check --config $config_file --dry-run + # Disable CI mode, because it is not compatible with all profiles + python3 ../x.py check --config $config_file --dry-run --ci=false done From f5c59a444f45e32772f393358c2c28c5346f49a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 21 Mar 2025 12:23:44 +0100 Subject: [PATCH 6/6] Fix test using `download-ci-llvm=true` on CI --- src/bootstrap/src/core/builder/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/src/core/builder/tests.rs b/src/bootstrap/src/core/builder/tests.rs index b7a51a33dbd26..fd3b28e4e6ab2 100644 --- a/src/bootstrap/src/core/builder/tests.rs +++ b/src/bootstrap/src/core/builder/tests.rs @@ -1074,7 +1074,7 @@ fn test_prebuilt_llvm_config_path_resolution() { let config = configure( r#" [llvm] - download-ci-llvm = true + download-ci-llvm = "if-unchanged" "#, );