Skip to content

simplify LLVM submodule handling #130918

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
Merged
merged 1 commit into from
Sep 28, 2024
Merged

simplify LLVM submodule handling #130918

merged 1 commit into from
Sep 28, 2024

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Sep 27, 2024 •
edited
Loading

Fixes #130906.

@rustbot
Copy link
Collaborator

rustbot commented Sep 27, 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 Sep 27, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 27, 2024

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

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

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

Kobzol
Kobzol reviewed Sep 27, 2024
View reviewed changes
Copy link
Contributor

@Kobzol Kobzol left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't like this change very much, tbh, as I'd like to move as much side-effect heavy and command-invocking logic as possible from Config parsing. It does not seem very intuitive that we check out a git submodule while parsing a config option. The previous code was also quite clear in that it only updated LLVM if it was already checked out, and initialized the submodule only if it wasn't downloaded in the meantime.

It seems like there is some different bug going on? The existing code should explicitly not check out LLVM if it was downloaded.

@onur-ozkan onur-ozkan force-pushed the better-llvm-submodule-handling branch from 98111bd to a126c56 Compare September 27, 2024 12:01
@onur-ozkan
Copy link
Member Author

I don't like this change very much, tbh, as I'd like to move as much side-effect heavy and command-invocking logic as possible from Config parsing. It does not seem very intuitive that we check out a git submodule while parsing a config option.

Yeah, agreed.

It seems like there is some different bug going on? The existing code should explicitly not check out LLVM if it was downloaded.

I'm not sure what to call it but it's not exactly a "bug". The mir-opt test involves some cross-targeting and when it's run with the --bless flag, and then this line calls llvm::prebuilt_llvm_config with a different target which bypasses the early return for CI LLVM. a126c56 should handle the situation in a better way than the previous attempt.

onur-ozkan
onur-ozkan commented Sep 27, 2024
View reviewed changes
@onur-ozkan
improve LLVM submodule handling logic in llvm::prebuilt_llvm_config
cd1b245 
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan onur-ozkan force-pushed the better-llvm-submodule-handling branch from a126c56 to cd1b245 Compare September 27, 2024 13:09
Kobzol
Kobzol reviewed Sep 27, 2024
View reviewed changes
@Kobzol
Copy link
Contributor

Kobzol commented Sep 27, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 27, 2024

📌 Commit cd1b245 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 Sep 27, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 28, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 28, 2024
@bors
Auto merge of rust-lang#130979 - matthiaskrgr:rollup-u7ylca5, r=matth…
e6eb451 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#128778 (atomics: allow atomic and non-atomic reads to race)
 - rust-lang#130918 (simplify LLVM submodule handling)
 - rust-lang#130960 (Only add an automatic SONAME for Rust dylibs)
 - rust-lang#130973 (compiletest: rename "runtest/crash.rs" to "runtest/crashes.rs" to be in line with the test directory)
 - rust-lang#130976 (remove couple redundant clones)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f33fa3f into rust-lang:master Sep 28, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 28, 2024
@onur-ozkan onur-ozkan deleted the better-llvm-submodule-handling branch September 28, 2024 17:11
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 28, 2024
@rust-timer
Unrolled build for rust-lang#130918
357b800 
Rollup merge of rust-lang#130918 - onur-ozkan:better-llvm-submodule-handling, r=Kobzol

simplify LLVM submodule handling

Fixes rust-lang#130906.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kobzol Kobzol Kobzol left review comments

Assignees

@albertlarsan68 albertlarsan68

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

x test mir-opt --bless tries to checkout src/llvm-project
5 participants
@onur-ozkan @rustbot @Kobzol @bors @albertlarsan68