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

compiletest: add max-llvm-major-version directive #132310

Merged
merged 2 commits into from
Nov 14, 2024
Merged

compiletest: add max-llvm-major-version directive #132310

merged 2 commits into from
Nov 14, 2024
+29 −5

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Oct 29, 2024 •
edited
Loading

To complement existing min-llvm-version so contributors don't have to use ignore-llvm-version: 20 - 99 to emulate max-llvm-major-version: 19.

Closes #132305.
cc @workingjubilee who suggested this.

Implementation steps

r? bootstrap
Page not found · GitHub · GitHub
Skip to content
404 “This is not the web page you are looking for”
Contact Support — GitHub Status — @githubstatus
@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc 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 29, 2024
@Zalathar

This comment was marked as resolved.

Sign in to view
@Zalathar
Copy link
Contributor

Not mandatory, but consider also editing the ~5 tests that currently use the N-99 pattern, so that the new directive is actually used.

Zalathar
Zalathar reviewed Oct 29, 2024
View reviewed changes
src/tools/compiletest/src/header.rs Outdated
// Ignore if actual version is smaller the minimum required
// version
if let Some(value) = config.parse_name_value_directive(line, "min-llvm-version") {
let value = value.trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

Remark: Separate from this PR, we should just make the parse functions trim automatically. I can't imagine a legitimate use-case for directives actually wanting to see untrimmed whitespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to touch that in the current directive handling, tbh. I prefer we just redo this entirely.

@jieyouxu

This comment was marked as resolved.

Sign in to view
@jieyouxu jieyouxu 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 29, 2024
@jieyouxu
Copy link
Member Author

Not mandatory, but consider also editing the ~5 tests that currently use the N-99 pattern, so that the new directive is actually used.

Good point, I'll update existing tests.

@jieyouxu

This comment was marked as resolved.

Sign in to view
@jieyouxu

This comment was marked as resolved.

Sign in to view
@jieyouxu jieyouxu marked this pull request as draft October 29, 2024 06:41
@workingjubilee workingjubilee added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Oct 29, 2024
@jieyouxu jieyouxu mentioned this pull request Oct 29, 2024
Merged
@bors

This comment was marked as resolved.

Sign in to view
@jieyouxu jieyouxu changed the title compiletest: add max-llvm-version directive compiletest: add max-llvm-major-version directive Oct 30, 2024
@jieyouxu
Copy link
Member Author

jieyouxu commented Oct 30, 2024 •
edited
Loading

Changes since last force push:

@Zalathar
Copy link
Contributor

Not sure if this is a good idea or not, but another possibility would be to reframe the directive as something like llvm-version-less-than.

In practice, I think a lot of the real-world uses of this directive are going to be along the lines of “this doesn't work on LLVM 21”, in which case llvm-version-less-than: 21 might make more sense? Hard to say.

@jieyouxu
Copy link
Member Author

jieyouxu commented Oct 30, 2024 •
edited
Loading

Not sure if this is a good idea or not, but another possibility would be to reframe the directive as something like llvm-version-less-than.

In practice, I think a lot of the real-world uses of this directive are going to be along the lines of “this doesn't work on LLVM 21”, in which case llvm-version-less-than: 21 might make more sense? Hard to say.

We could also just use a semver version range: //@ only-llvm-version-range: >=20.0.0. Since we're using semver in the prerequisite PR anyway this can even do stuff like //@ only-llvm-version-range: >=19 <21. Or 19.2 - 20.3.0.

(But that is also quite obscure, just musing)

jieyouxu added a commit to jieyouxu/rust that referenced this pull request Oct 30, 2024
@jieyouxu
Rollup merge of rust-lang#132315 - jieyouxu:extract-llvm-version, r=o…
c22832f 
…nur-ozkan

compiletest: improve robustness of LLVM version handling

Previously, `extract_llvm_versions` did some gymnastics for llvm versions by combining `(major, minor, patch)` into a combined version integer, but that is not very robust and made it difficult to add `max-llvm-major-version`. This PR tries to:

- Improve llvm version handling robustness by parsing and representing the version as a semver. We intentionally deviate from strict semver standards by allowing omission of minor and patch versions. They default to `0` when absent. This is for convenience to allow the user to write e.g. `//@ min-llvm-version: 18` instead of having to spell out the full `major.minor.patch` semver string `//@ min-llvm-verison: 18.0.0`.
- Adjust some panic messages to include a bit more context about *why* the version string was rejected.

Prerequisite for rust-lang#132310.

r? bootstrap (or compiler)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 30, 2024
@rust-timer
Unrolled build for rust-lang#132315
4a986b0 
Rollup merge of rust-lang#132315 - jieyouxu:extract-llvm-version, r=onur-ozkan

compiletest: improve robustness of LLVM version handling

Previously, `extract_llvm_versions` did some gymnastics for llvm versions by combining `(major, minor, patch)` into a combined version integer, but that is not very robust and made it difficult to add `max-llvm-major-version`. This PR tries to:

- Improve llvm version handling robustness by parsing and representing the version as a semver. We intentionally deviate from strict semver standards by allowing omission of minor and patch versions. They default to `0` when absent. This is for convenience to allow the user to write e.g. `//@ min-llvm-version: 18` instead of having to spell out the full `major.minor.patch` semver string `//@ min-llvm-verison: 18.0.0`.
- Adjust some panic messages to include a bit more context about *why* the version string was rejected.

Prerequisite for rust-lang#132310.

r? bootstrap (or compiler)
@jieyouxu jieyouxu marked this pull request as ready for review November 8, 2024 09:10
@jieyouxu
Copy link
Member Author

jieyouxu commented Nov 8, 2024

Hm, I feel like I forgot to set this as ready, lol.

@rustbot ready

@rustbot rustbot removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 8, 2024
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 8, 2024
This was referenced Nov 8, 2024
Merged
Closed
@jieyouxu

This comment was marked as resolved.

Sign in to view
@jieyouxu
Copy link
Member Author

r? bootstrap

@onur-ozkan
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 13, 2024

📌 Commit f41e1cf 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2024
@bors
Copy link
Collaborator

bors commented Nov 14, 2024

☔ The latest upstream changes (presumably #133006) made this pull request unmergeable. Please resolve the merge conflicts.

@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 Nov 14, 2024
@jieyouxu
compiletest: add max-llvm-major-version directive
7eee9fa 
There's already `min-llvm-version`, and contributors were using
`ignore-llvm-version: 20 - 99` to emulate `max-llvm-major-version: 19`.
@jieyouxu
tests: use max-llvm-major-version instead of ignore-llvm-version …
91fa16b 
…range like `N - 99`

For tests that use `ignore-llvm-version: N - M`, replace that with
`max-llvm-major-version: N-1`.
@jieyouxu
Copy link
Member Author

Just whitespace conflicts.

@bors r=onur-ozkan

@bors
Copy link
Collaborator

bors commented Nov 14, 2024

📌 Commit 91fa16b 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 Nov 14, 2024
@GuillaumeGomez GuillaumeGomez mentioned this pull request Nov 14, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 14, 2024
@bors
Auto merge of rust-lang#133032 - GuillaumeGomez:rollup-vqakdmw, r=Gui…
a4cedec 
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#132010 (ci: Enable full `debuginfo-level=2` in `DEPLOY_ALT`)
 - rust-lang#132310 (compiletest: add `max-llvm-major-version` directive)
 - rust-lang#132773 (PassWrapper: disable UseOdrIndicator for Asan Win32)
 - rust-lang#133013 (compiletest: known-bug / crashes: allow for an "auxiliary" directory to contain files that do not have a "known-bug" directive)
 - rust-lang#133027 (Fix a copy-paste issue in the NuttX raw type definition)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 475203f into rust-lang:master Nov 14, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 14, 2024
@rust-timer
Unrolled build for rust-lang#132310
6271b7b 
Rollup merge of rust-lang#132310 - jieyouxu:max-llvm-version, r=onur-ozkan

compiletest: add `max-llvm-major-version` directive

To complement existing `min-llvm-version` so contributors don't have to use `ignore-llvm-version: 20 - 99` to emulate `max-llvm-major-version: 19`.

Closes rust-lang#132305.
cc `@workingjubilee` who suggested this.

### Implementation steps

- [x] 1. Implement the directive (this PR)
- [x] 2. Open an accompanying dev-guide PR to describe the directive (rust-lang/rustc-dev-guide#2129)

r? bootstrap
@jieyouxu jieyouxu deleted the max-llvm-version branch November 14, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic

@Zalathar Zalathar

@onur-ozkan onur-ozkan

Assignees

@onur-ozkan onur-ozkan

Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc 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.84.0
Development

Successfully merging this pull request may close these issues.

compiletest: add max-llvm-major-version directive
9 participants
@jieyouxu @Zalathar @bors @onur-ozkan @nikic @Mark-Simulacrum @workingjubilee @rustbot @albertlarsan68