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

Jump to bottom

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

merged 2 commits into from
Oct 11, 2024

Conversation

onur-ozkan
Copy link
Member

Fixes #131303.

@rustbot
Copy link
Collaborator

rustbot commented Oct 5, 2024

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Oct 5, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 5, 2024

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

@Kobzol
Copy link
Contributor

Kobzol commented Oct 7, 2024

The new logic looks good, but I don't understand how it fixes the issue. What specifically has changed which would cause it to work properly now?

@onur-ozkan
Copy link
Member Author

rust/src/bootstrap/src/core/config/tests.rs

Lines 22 to 27 in 690332a

#[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;
}

The test above supposed to skip when there is a change in the LLVM submodule in CI. However, as reported in #131303, it isn't behaving as expected. I suspect this is related to the CiEnv::is_rust_lang_managed_ci_job() logic (maybe some pipelines do not have this env variables set together?), though I'm not entirely certain.

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Oct 7, 2024 •
edited
Loading

I will test it with try build later today/tomorrow.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 7, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Oct 7, 2024

These two variables should be indeed set unconditionally for all CI jobs. So I suspect that the erorr might have been in this logic instead.

@onur-ozkan
Copy link
Member Author

Probably, will debug that and fix it.

@onur-ozkan
Copy link
Member Author

It didn't happen in CI; maybe it was something else or the first attempt already fixed it. Either way, I made the function even better.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2024
@onur-ozkan onur-ozkan mentioned this pull request Oct 9, 2024
Merged
@onur-ozkan
Copy link
Member Author

Tested and it's fixed #131448 (comment) issue in #131455.

Kobzol
Kobzol reviewed Oct 10, 2024
View reviewed changes
src/bootstrap/src/core/build_steps/llvm.rs Outdated
.args(["diff-index", "--quiet", &commit])
.arg("--")
.args([
config.src.join("src/llvm-project"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please unify these three files with the same filelist used in detect_llvm_sha?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's no longer necessary.

@onur-ozkan
make llvm::is_ci_llvm_modified logic more precise
ef51577 
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 2024

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@onur-ozkan
stabilize download_ci_llvm test
f6b7557 
Signed-off-by: onur-ozkan <work@onurozkan.dev>
onur-ozkan
onur-ozkan commented Oct 10, 2024
View reviewed changes
src/bootstrap/src/core/config/tests.rs
Comment on lines -34 to -44
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\""
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.

Kobzol
Kobzol reviewed Oct 10, 2024
View reviewed changes
src/bootstrap/src/core/build_steps/llvm.rs
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.

@Kobzol
Copy link
Contributor

Kobzol commented Oct 11, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2024

📌 Commit f6b7557 has been approved by Kobzol

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2024
@Zalathar Zalathar mentioned this pull request Oct 11, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2024
@bors
Auto merge of rust-lang#131532 - Zalathar:rollup-688kw5t, r=Zalathar
ce697f9 
Rollup of 3 pull requests

Successful merges:

 - rust-lang#131305 (make `llvm::is_ci_llvm_modified` logic more precise)
 - rust-lang#131524 (compiletest: Remove the magic hacks for finding output with `lto=thin`)
 - rust-lang#131525 (compiletest: Simplify the choice of `--emit` mode for assembly tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c4e7425 into rust-lang:master Oct 11, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2024
@rust-timer
Unrolled build for rust-lang#131305
f7b91a1 
Rollup merge of rust-lang#131305 - onur-ozkan:131303, r=Kobzol

make `llvm::is_ci_llvm_modified` logic more precise

Fixes rust-lang#131303.
@onur-ozkan onur-ozkan deleted the 131303 branch October 11, 2024 09:45
@bors
Copy link
Contributor

bors commented Oct 11, 2024

⌛ Testing commit f6b7557 with merge ce697f9...

@matthiaskrgr
Copy link
Member

@bors retry r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu left review comments

@Kobzol Kobzol Kobzol left review comments

Assignees

@albertlarsan68 albertlarsan68

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.

download_ci_llvm test failure when updating LLVM
7 participants
@onur-ozkan @rustbot @Kobzol @bors @matthiaskrgr @jieyouxu @albertlarsan68