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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

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

wants to merge 3 commits into from
+171 −76

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Oct 29, 2024 •
edited
Loading

Currently rebased on top of #132315.

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.

r? bootstrap

@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: Marked as 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
compiletest: improve robustness of LLVM version handling
e419b3d
@jieyouxu
compiletest: add max-llvm-major-version directive
a4107b0 
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 …
b6a7cca 
…range like `N - 99`

For tests that use `ignore-llvm-version: N - M`, replace that with
`max-llvm-major-version: N-1`.
@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.

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zalathar Zalathar Zalathar left review comments

Assignees

@albertlarsan68 albertlarsan68

Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. 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
No milestone
Development

Successfully merging this pull request may close these issues.

compiletest: add max-llvm-version directive
6 participants
@jieyouxu @Zalathar @bors @workingjubilee @rustbot @albertlarsan68