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

Remove the option to disable llvm-version-check #108619

Merged
merged 1 commit into from
Mar 7, 2023
Merged

Remove the option to disable llvm-version-check #108619

merged 1 commit into from
Mar 7, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 1, 2023

We don't support old versions of LLVM; there's no reason to have an easy way to force bootstrap to use them anyway. If someone really needs to use an unsupported version, they can modify bootstrap to change the version range.

r? @cuviper on whether we want to do this or not, since you maintain rust on Fedora and touched this config last.

@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 Mar 1, 2023
@jyn514
Copy link
Member Author

jyn514 commented Mar 1, 2023

cc @rust-lang/wg-llvm

@cuviper
Copy link
Member

cuviper commented Mar 3, 2023

since you maintain rust on Fedora and touched this config last.

I've never used this on Fedora, but wow, I touched that config way back in #45326! It looks like the original #22147 was motivated to allow newer LLVM versions than were known to work. My change made us check for anything >= minimum, so newer is always allowed (but not guaranteed), while the flag could allow older.

Today, we have folks actively working to make sure that newer LLVM (on its main branch) continues to work. Using an older LLVM will almost certainly require patching our LLVM bindings and such to work, so I agree that such a developer can also patch the checked LLVM minimum while they're at it.

So, I do think it's reasonable to remove this. Do we have a compatibility story around the build config though? I think bootstrap will barf if config.toml has any unknown fields, even if they had kept version-check = true.

@jyn514
Copy link
Member Author

jyn514 commented Mar 5, 2023

So, I do think it's reasonable to remove this. Do we have a compatibility story around the build config though? I think bootstrap will barf if config.toml has any unknown fields, even if they had kept version-check = true.

Right now the only thing we really have is src/bootstrap/CHANGELOG.md and MAJOR_VERSION which prompts you to look at the changelog if changelog-seen is set to an earlier version. I don't think we should commit to any backwards compatibility for bootstrap though, it makes sense to avoid needless breaking changes but we don't have any stability guarantee.

@Mark-Simulacrum
Copy link
Member

Yes, I think we should feel comfortable just dropping options - though a note in the changelog that we did so doesn't seem unreasonable.

@jyn514 jyn514 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 Mar 5, 2023
@jyn514
Remove the option to disable llvm-version-check
8becfed 
We don't support old versions of LLVM; there's no reason to have an easy
way to force bootstrap to use them anyway. If someone really needs to
use an unsupported version, they can modify bootstrap to change the
version range.
@jyn514 jyn514 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 Mar 5, 2023
@cuviper
Copy link
Member

cuviper commented Mar 7, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 7, 2023

📌 Commit 8becfed has been approved by cuviper

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 Mar 7, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 7, 2023
@matthiaskrgr
Rollup merge of rust-lang#108619 - jyn514:llvm-version-check, r=cuviper
c1676bd 
Remove the option to disable `llvm-version-check`

We don't support old versions of LLVM; there's no reason to have an easy way to force bootstrap to use them anyway. If someone really needs to use an unsupported version, they can modify bootstrap to change the version range.

r? `@cuviper` on whether we want to do this or not, since you maintain rust on Fedora and touched this config last.
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 7, 2023
Closed
@JohnTitor JohnTitor mentioned this pull request Mar 7, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2023
@bors
Auto merge of rust-lang#108863 - JohnTitor:rollup-haydnsw, r=JohnTitor
1a521db 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#108619 (Remove the option to disable `llvm-version-check`)
 - rust-lang#108728 (infer: fix and improve comments)
 - rust-lang#108731 (feat: impl better help for `.poll()` not found on `impl Future`)
 - rust-lang#108774 (Greatly improve the error messages when `run-make/translation` fails)
 - rust-lang#108805 (Update askama to 0.12 and improve whitespace control)
 - rust-lang#108823 (Add tracking issue for cf-protection to unstable book)
 - rust-lang#108855 (Custom MIR: Support `as` casts)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a7b9488 into rust-lang:master Mar 7, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cuviper cuviper

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

Successfully merging this pull request may close these issues.
5 participants
@jyn514 @cuviper @Mark-Simulacrum @bors @rustbot