tests: prefer max-llvm-major-version over open LLVM ranges

[AayushMainali-Github]

Per review, take option 3: replace open ignore-llvm-version ranges with max-llvm-major-version. Left gdb X - 99.0 ranges alone so CI's older gdb still runs those tests.

Fixes #159338.

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

compiletest directives have been modified. Please add or update docs for the
new or modified directive in src/doc/rustc-dev-guide/.

[rustbot]

Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @jieyouxu, @oli-obk, @wesleywiser, bootstrap
• @jieyouxu, @oli-obk, @wesleywiser, bootstrap expanded to 8 candidates
• Random selection from Mark-Simulacrum, clubby789, oli-obk, wesleywiser

[Mark-Simulacrum]

Presuming my grep isn't broken, there don't seem to be many version range directives... I'm wondering if we can restrict to known versions of some kind in the syntax? Or maybe switch to e.g. Rust range syntax (..) which is less likely to conflict?

$ rg 'version:' tests --no-filename | rg ':.*-'
//@ [aarch64-llvm-pre-23] ignore-llvm-version: 23 - 99
//@ ignore-gdb-version: 15.0 - 99.0
//@ ignore-gdb-version: 15.0 - 99.0
//@ ignore-gdb-version: 13.1 - 99.0
//@[llvm-current] ignore-llvm-version: 23 - 99
//@[loongarch32] ignore-llvm-version: 22 - 23
//@[loongarch64] ignore-llvm-version: 22 - 23
//@[loongarch32] ignore-llvm-version: 22 - 23
//@[loongarch64] ignore-llvm-version: 22 - 23

[jieyouxu]

//@ [aarch64-llvm-pre-23] ignore-llvm-version: 23 - 99

This should just be //@ max-llvm-major-version: 22 IMHO

//@ ignore-gdb-version: 15.0 - 99.0
//@ ignore-gdb-version: 15.0 - 99.0
//@ ignore-gdb-version: 13.1 - 99.0

I almost feel like these should just be //@ ignore-gdb (after #159451). Right now in CI we use the gdb that comes with Ubuntu 22.04, which happens to be gdb 12.1. It would be good to double-check why they require an older gdb rather than newer gdb. Also the end range of 99.0 is really just a funny way to say "ignore newer gdb versions than the version specified in the start range".

//@[llvm-current] ignore-llvm-version: 23 - 99
//@[loongarch32] ignore-llvm-version: 22 - 23
//@[loongarch64] ignore-llvm-version: 22 - 23
//@[loongarch32] ignore-llvm-version: 22 - 23
//@[loongarch64] ignore-llvm-version: 22 - 23

Ditto on //@ max-llvm-major-version, probably 22 and 21 respectively.

For the LLVM versioning, we can probably re-use the .. that //@ edition currently uses, cc

rust/src/tools/compiletest/src/directives.rs

Line 1288 in 234c31c

fn parse_edition_range(config: &Config, line: &DirectiveLine<'_>) -> Option<EditionRange> {

However I'm not actually convinced the ignore-llvm-version: 22 - 23 are good ignore ranges, do those test pass against... LLVM 24? LLVM trunk is only LLVM 23 right? I feel like those loongarch64 ones also want //@ max-llvm-major-version?

[AayushMainali-Github]

Thanks. Happy to change approach if preferred.

Options I’m seeing:

• Switch version ranges to .. (like //@ edition), and keep rejecting ambiguous forms
• Keep but require spaces and hard-error on 23-99 instead of accepting it
• replace X - 99 with max-llvm-major-version / simpler gdb ignores, and only touch the parser as needed

Which direction should this PR take? @Mark-Simulacrum @jieyouxu

[Mark-Simulacrum]

I think option 3 is probably where I lean but r? @jieyouxu, I think you've thought more deeply about this space.

[jieyouxu]

The approach I'd suggest is option 3 as well. I'm somewhat wary of trying to use the .. edition syntax, now that I think about it, because edition can only be NNNN years (pure integers) but LLVM etc. version strings can be X.Y.Z which makes the .. arguably quite confusing

[AayushMainali-Github]

Updated for option 3. Left gdb ranges alone for now (CI still uses older gdb). Happy to change if preferred.

@rustbot review

[jieyouxu]

Thanks. I think these tests probably need revisiting by the target maintainers, but that's orthogonal to the usages here.
@bors r+ rollup

View changes since this review

[rust-bors]

📌 Commit 20ac212 has been approved by jieyouxu

It is now in the queue for this repository.