Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make llvm::is_ci_llvm_modified logic more precise #131305

Merged
merged 2 commits into from
Oct 11, 2024
Merged

make llvm::is_ci_llvm_modified logic more precise #131305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef51577
make `llvm::is_ci_llvm_modified` logic more precise
onur-ozkan Oct 5, 2024
f6b7557
stabilize `download_ci_llvm` test
onur-ozkan Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions src/bootstrap/src/core/build_steps/llvm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,9 @@ use build_helper::git::get_closest_merge_commit;

use crate::core::builder::{Builder, RunConfig, ShouldRun, Step};
use crate::core::config::{Config, TargetSelection};
use crate::utils::channel;
use crate::utils::exec::command;
use crate::utils::helpers::{
self, HashStamp, exe, get_clang_cl_resource_dir, output, t, unhashed_basename, up_to_date,
self, HashStamp, exe, get_clang_cl_resource_dir, t, unhashed_basename, up_to_date,
};
use crate::{CLang, GitRepo, Kind, generate_smart_stamp_hash};

Expand Down Expand Up @@ -166,7 +165,7 @@ pub(crate) fn detect_llvm_sha(config: &Config, is_git: bool) -> String {
config.src.join("src/version"),
])
.unwrap()
} else if let Some(info) = channel::read_commit_info_file(&config.src) {
} else if let Some(info) = crate::utils::channel::read_commit_info_file(&config.src) {
info.sha.trim().to_owned()
} else {
"".to_owned()
Expand Down Expand Up @@ -242,15 +241,29 @@ pub(crate) fn is_ci_llvm_available(config: &Config, asserts: bool) -> bool {

/// 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 {
CiEnv::is_rust_lang_managed_ci_job() && config.rust_info.is_managed_git_subrepository() && {
// 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(helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD").as_command_mut());
let head_sha = head_sha.trim();
llvm_sha == head_sha
// If not running in a CI environment, return false.
if !CiEnv::is_ci() {
return false;
}

// In rust-lang/rust managed CI, assert the existence of the LLVM submodule.
if CiEnv::is_rust_lang_managed_ci_job() {
Kobzol marked this conversation as resolved.
Show resolved Hide resolved
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is wrong, and this code (which was in a previous commit) should be used instead.

Or at the very least, we should change the documentation of get_closest_merge_commit. Because detect_llvm_sha uses get_closest_merge_commit, which is supposed to return the latest upstream merge commit (that modified LLVM). In that case, it can by definition never return HEAD on CI (because HEAD is definitely not upstream).

However, we know that currently get_closest_merge_commit is bugged and probably just returns HEAD on CI, so we should probably first resolve #131358 before continuing with this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR unblocks LLVM update PRs and it doesn't do anything other than improving current logic. I don't think #131358 should block this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's fix the download_ci_llvm test for now to unblock the update PRs. But the logic is still wrong, #131358 should hopefully fix it later.

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)]
Expand Down
36 changes: 14 additions & 22 deletions src/bootstrap/src/core/config/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,21 @@ fn parse(config: &str) -> Config {

#[test]
fn download_ci_llvm() {
if crate::core::build_steps::llvm::is_ci_llvm_modified(&parse("")) {
eprintln!("Detected LLVM as non-available: running in CI and modified LLVM in this change");
return;
assert!(parse("").llvm_from_ci);
assert!(parse("llvm.download-ci-llvm = true").llvm_from_ci);
assert!(!parse("llvm.download-ci-llvm = false").llvm_from_ci);

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)
.is_none();

assert!(
!has_changes,
"CI LLVM can't be enabled with 'if-unchanged' while there are changes in LLVM submodule."
);
}

let parse_llvm = |s| parse(s).llvm_from_ci;
let if_unchanged = parse_llvm("llvm.download-ci-llvm = \"if-unchanged\"");

assert!(parse_llvm("llvm.download-ci-llvm = true"));
assert!(!parse_llvm("llvm.download-ci-llvm = false"));
assert_eq!(parse_llvm(""), if_unchanged);
assert_eq!(parse_llvm("rust.channel = \"dev\""), if_unchanged);
assert!(parse_llvm("rust.channel = \"stable\""));
assert_eq!(parse_llvm("build.build = \"x86_64-unknown-linux-gnu\""), if_unchanged);
assert_eq!(
parse_llvm(
"llvm.assertions = true \r\n build.build = \"x86_64-unknown-linux-gnu\" \r\n llvm.download-ci-llvm = \"if-unchanged\""
),
if_unchanged
);
assert!(!parse_llvm(
"llvm.assertions = true \r\n build.build = \"aarch64-apple-darwin\" \r\n llvm.download-ci-llvm = \"if-unchanged\""
Comment on lines -34 to -44
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if-unchanged is not default since #130529. These didn’t fail before because if there are no changes (which is ensured here), both download-ci-llvm=if-unchanged and true will behave the same.

));
}

// FIXME(onur-ozkan): extend scope of the test
Expand Down
Loading