Skip to content

Adapt llvm-has-rust-patches validation to take llvm-config into account. #120890

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 11, 2024
Merged

Adapt llvm-has-rust-patches validation to take llvm-config into account. #120890

merged 1 commit into from
Feb 11, 2024

Conversation

TimNN
Copy link
Contributor

@TimNN TimNN commented Feb 10, 2024 •
edited
Loading

This adapts an assertion that was added in #119556.

The current assertion does not take the llvm-config setting into accounts, which does not match the llvm-has-rust-patches documentation, which states:

This would be used in conjunction with either an llvm-config or build.submodules = false.

(It also breaks my workflow: I build LLVM separately, but do have the rust patches applied).

edit: Originally this PR just removed the assertion, but it now implements the alternative mentioned here:

An alternative fix would be to take llvm-config into account in the assertion, but to me the assertion seems to provide little value, thus the simpler fix of just removing it.

cc @onur-ozkan, in case I'm missing a reason to keep the assertion.

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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 Feb 10, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 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
onur-ozkan reviewed Feb 10, 2024
View reviewed changes
@onur-ozkan
Copy link
Member

r? onur-ozkan

@onur-ozkan onur-ozkan 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 Feb 10, 2024
@TimNN TimNN force-pushed the relax-patches-check branch 2 times, most recently from 174a311 to 36bdb6b Compare February 10, 2024 21:36
@TimNN TimNN changed the title Remove restriction on when llvm-has-rust-patches can be set. Adapt llvm-has-rust-patches validation to take llvm-config into account. Feb 10, 2024
@TimNN TimNN force-pushed the relax-patches-check branch from 36bdb6b to fd470e5 Compare February 10, 2024 21:42
@TimNN
Copy link
Contributor Author

TimNN commented Feb 10, 2024

Sorry, I messed up the initial fixup commit. Should be resolved now.

onur-ozkan
onur-ozkan approved these changes Feb 10, 2024
View reviewed changes
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Thank you!

@onur-ozkan
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Feb 10, 2024

📌 Commit fd470e5 has been approved by onur-ozkan

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 10, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 11, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2024
@bors
Auto merge of rust-lang#120903 - matthiaskrgr:rollup-tmsuzth, r=matth…
7ffdfda 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119213 (simd intrinsics: add simd_shuffle_generic and other missing intrinsics)
 - rust-lang#120272 (Suppress suggestions in derive macro)
 - rust-lang#120773 (large_assignments: Allow moves into functions)
 - rust-lang#120874 (Take empty `where` bounds into account when suggesting predicates)
 - rust-lang#120882 (interpret/write_discriminant: when encoding niched variant, ensure the stored value matches)
 - rust-lang#120883 (interpret: rename ReadExternStatic → ExternStatic)
 - rust-lang#120890 (Adapt `llvm-has-rust-patches` validation to take `llvm-config` into account.)
 - rust-lang#120895 (don't skip coercions for types with errors)
 - rust-lang#120896 (Print kind of coroutine closure)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2024
@bors
Auto merge of rust-lang#120903 - matthiaskrgr:rollup-tmsuzth, r=matth…
aba3b36 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119213 (simd intrinsics: add simd_shuffle_generic and other missing intrinsics)
 - rust-lang#120272 (Suppress suggestions in derive macro)
 - rust-lang#120773 (large_assignments: Allow moves into functions)
 - rust-lang#120874 (Take empty `where` bounds into account when suggesting predicates)
 - rust-lang#120882 (interpret/write_discriminant: when encoding niched variant, ensure the stored value matches)
 - rust-lang#120883 (interpret: rename ReadExternStatic → ExternStatic)
 - rust-lang#120890 (Adapt `llvm-has-rust-patches` validation to take `llvm-config` into account.)
 - rust-lang#120895 (don't skip coercions for types with errors)
 - rust-lang#120896 (Print kind of coroutine closure)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2024
@bors
Auto merge of rust-lang#120903 - matthiaskrgr:rollup-tmsuzth, r=matth…
a8ba864 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119213 (simd intrinsics: add simd_shuffle_generic and other missing intrinsics)
 - rust-lang#120272 (Suppress suggestions in derive macro)
 - rust-lang#120773 (large_assignments: Allow moves into functions)
 - rust-lang#120874 (Take empty `where` bounds into account when suggesting predicates)
 - rust-lang#120882 (interpret/write_discriminant: when encoding niched variant, ensure the stored value matches)
 - rust-lang#120883 (interpret: rename ReadExternStatic → ExternStatic)
 - rust-lang#120890 (Adapt `llvm-has-rust-patches` validation to take `llvm-config` into account.)
 - rust-lang#120895 (don't skip coercions for types with errors)
 - rust-lang#120896 (Print kind of coroutine closure)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2024
@bors
Auto merge of rust-lang#120903 - matthiaskrgr:rollup-tmsuzth, r=matth…
a166af7 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119213 (simd intrinsics: add simd_shuffle_generic and other missing intrinsics)
 - rust-lang#120272 (Suppress suggestions in derive macro)
 - rust-lang#120773 (large_assignments: Allow moves into functions)
 - rust-lang#120874 (Take empty `where` bounds into account when suggesting predicates)
 - rust-lang#120882 (interpret/write_discriminant: when encoding niched variant, ensure the stored value matches)
 - rust-lang#120883 (interpret: rename ReadExternStatic → ExternStatic)
 - rust-lang#120890 (Adapt `llvm-has-rust-patches` validation to take `llvm-config` into account.)
 - rust-lang#120895 (don't skip coercions for types with errors)
 - rust-lang#120896 (Print kind of coroutine closure)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 955cbdf into rust-lang:master Feb 11, 2024
@rustbot rustbot added this to the 1.78.0 milestone Feb 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2024
@rust-timer
Unrolled build for rust-lang#120890
72ae404 
Rollup merge of rust-lang#120890 - TimNN:relax-patches-check, r=onur-ozkan

Adapt `llvm-has-rust-patches` validation to take `llvm-config` into account.

This adapts an assertion that was added in rust-lang#119556.

The current assertion does not take the `llvm-config` setting into accounts, which does not match the `llvm-has-rust-patches` documentation, which states:

> This would be used in conjunction with either an llvm-config or build.submodules = false.

(It also breaks my workflow: I build LLVM separately, but do have the rust patches applied).

---

**edit:** Originally this PR just removed the assertion, but it now implements the alternative mentioned here:

An alternative fix would be to take `llvm-config` into account in the assertion, but to me the assertion seems to provide little value, thus the simpler fix of just removing it.

cc `@onur-ozkan,` in case I'm missing a reason to keep the assertion.
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@onur-ozkan onur-ozkan onur-ozkan approved these changes

Assignees

@onur-ozkan onur-ozkan

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.78.0
Development

Successfully merging this pull request may close these issues.
5 participants
@TimNN @rustbot @onur-ozkan @bors @Mark-Simulacrum