Validate `ignore` and `only` compiletest directive, and add human-readable ignore reasons #108905

pietroalbini · 2023-03-08T14:39:13Z

This PR adds strict validation for the ignore and only compiletest directives, failing if an unknown value is provided to them. Doing so uncovered 79 tests in tests/ui that had invalid directives, so this PR also fixes them.

Finally, this PR adds human-readable ignore reasons when tests are ignored due to ignore or only directives, like "only executed when the architecture is aarch64" or "ignored when the operative system is windows". This was the original reason why I started working on this PR and #108659, as we need both of them for Ferrocene.

The PR is a draft because the code is extremely inefficient: it calls rustc --print=cfg --target $target for every rustc target (to gather the list of allowed ignore values), which on my system takes between 4s and 5s, and performs a lot of allocations of constant values. I'll fix both of them in the coming days.

r? @ehuss

tests/codegen/global_asm.rs

tests/ui/asm/issue-87802.rs

bjorn3 · 2023-03-08T15:03:49Z

tests/ui/symbol-names/x86-stdcall.rs

@@ -1,5 +1,6 @@
 // build-pass
-// only-x86-windows
+// only-x86
+// only-windows


Nice catch!

pietroalbini · 2023-03-09T16:28:34Z

Ok so with --print all-target-specs-json it's now way faster again. Tomorrow I'll look into the test failure, handle "" being inserted into the hashsets, and possibly look if I can speed startup up even more.

ehuss

This looks great, thanks for putting it together!

compiler/rustc_driver_impl/src/lib.rs

pietroalbini · 2023-03-10T15:11:51Z

Ok I should've fixed everything, this PR is ready now.

ehuss · 2023-03-10T15:22:14Z

Looks like it needs to be formatted.

Also, I assume the 10 day period for rust-lang/compiler-team#600 needs to finish?

bors · 2023-03-14T04:29:15Z

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

pietroalbini · 2023-03-14T09:30:30Z

Rebased and formatted.

lqd · 2023-03-14T10:43:42Z

tests/ui/panic-runtime/unwind-tables-target-required.rs

@@ -1,7 +1,7 @@
 // Tests that the compiler errors if the user tries to turn off unwind tables
 // when they are required.
 //
-// only-x86_64-windows-msvc
+// only-x86_64-pc-windows-msvc


note that I already did this one in #108809 and it was not enough

pietroalbini · 2023-03-16T14:26:16Z

Pushed changes to fix the broken annotations in other compiletest suites (in addition to tests/ui).

pietroalbini · 2023-03-21T08:29:04Z

More than 10 days passed, so the MCP has been approved!

bors · 2023-03-21T18:03:27Z

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

ehuss

r=me with rebase

I haven't done a thorough review to check which tests will start or stop running with our CI configurations, or that this won't pass/fail on some of the more unusual targets. However, the changes look valid to me on the surface.

Perhaps someone could review the ignore messages in CI, and compare to a previous run, but I think that will be a lot of work.

ehuss · 2023-03-26T00:19:16Z

src/tools/compiletest/src/header/tests.rs

+        dbg!(target, pattern, ignore);
+        dbg!(config.target_cfgs());


You may want to remove these since they generate a large amount of output.

Whelp that wasn't supposed to be left there, I'll remove it tomorrow.

pietroalbini · 2023-03-26T12:34:49Z

I'll rebase tomorrow!

Perhaps someone could review the ignore messages in CI, and compare to a previous run, but I think that will be a lot of work.

Well, good thing that all the executed tests are now available in the build metrics JSON files since #108659 😂

pietroalbini · 2023-03-30T08:09:27Z

"tomorrow"

@bors r=ehuss rollup=never

bors · 2023-03-30T08:09:29Z

📌 Commit 4fb3e65fc436cf70ae995dcb20d148492daa4d02 has been approved by ehuss

It is now in the queue for this repository.

bors · 2023-04-03T16:41:42Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-04-03T16:54:37Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2023-04-05T16:15:28Z

⌛ Testing commit bbcbb6f with merge b2b676d...

scottmcm · 2023-04-05T18:28:53Z

Thanks for making this? I literally just yesterday caught myself making a type in // ignore-!

bors · 2023-04-05T18:35:27Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing b2b676d to master...

rust-timer · 2023-04-05T19:52:02Z

Finished benchmarking commit (b2b676d): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[1.5%, 4.4%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

…ehuss [compiletest] Add more test ignore reasons, `needs-` validation, and improved error messages This PR makes more improvements to the way compiletest ignoring headers are handled, following up on rust-lang#108905: * Human-readable ignore reasons have been added for the remaining ignore causes (`needs-*` directives, `*llvm*` directives, and debugger version directives). All ignored tests should now have a human-readable reason. * The code handling `needs-*` directives has been refactored, and now invalid `needs-*` directive emit errors like `ignore-*` and `only-*`. * All errors are now displayed at startup (with line numbers) rather than just the first error of the first file. This PR is best reviewed commit-by-commit. r? `@ehuss`

Compiletest was switched to querying all targets using `--print=all-target-specs-json` and `--print=target-spec-json` in rust-lang#108905. This unintentionally prevented codegen flags like `-Cpanic` from being reflected in the current target configuration. This change gets the current compiletest target config using `--print=cfg` like it was previously while still using the faster prints for getting information on all other targets. Fixes rust-lang#110850.

…target, r=compiler-errors Get current target config from` --print=cfg` Compiletest was switched to querying all targets using `--print=all-target-specs-json` and `--print=target-spec-json` in rust-lang#108905. This unintentionally prevented codegen flags like `-Cpanic` from being reflected in the current target configuration. This change gets the current compiletest target config using `--print=cfg` like it was previously while still using the faster prints for getting information on all other targets. Fixes rust-lang#110850. `@jyn514` might be interested in reviewing since they commented on the issue. cc `@tmandry` since this issue is affecting Fuchsia.

…rget, r=compiler-errors Get current target config from` --print=cfg` Compiletest was switched to querying all targets using `--print=all-target-specs-json` and `--print=target-spec-json` in rust-lang#108905. This unintentionally prevented codegen flags like `-Cpanic` from being reflected in the current target configuration. This change gets the current compiletest target config using `--print=cfg` like it was previously while still using the faster prints for getting information on all other targets. Fixes rust-lang#110850. `@jyn514` might be interested in reviewing since they commented on the issue. cc `@tmandry` since this issue is affecting Fuchsia.

This was changed from stage 0 to 1 in rust-lang#108905, but I'm not sure why. Change it to `top_stage` instead to allow people to choose the stage. This should save quite a bit of time in the `mingw-check` builder, which explicitly runs `x test --stage 0 compiletest`.

…mulacrum Don't compile rustc to self-test compiletest This was changed from stage 0 to 1 in rust-lang#108905, but I'm not sure why. Change it to `top_stage` instead to allow people to choose the stage. This should save quite a bit of time in the `mingw-check` builder, which explicitly runs `x test --stage 0 compiletest`. Note that this also fixes a latent bug that depended on running `x build compiler` before `x doc compiler`, as well as a couple cleanups related to symlinks (which made the latent bug easier to find). cc `@pietroalbini`

Don't compile rustc to self-test compiletest This was changed from stage 0 to 1 in rust-lang/rust#108905, but I'm not sure why. Change it to `top_stage` instead to allow people to choose the stage. This should save quite a bit of time in the `mingw-check` builder, which explicitly runs `x test --stage 0 compiletest`. Note that this also fixes a latent bug that depended on running `x build compiler` before `x doc compiler`, as well as a couple cleanups related to symlinks (which made the latent bug easier to find). cc `@pietroalbini`

rustbot assigned ehuss Mar 8, 2023

rustbot added 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 8, 2023

pietroalbini force-pushed the pa-compiletest-ignore branch from ec7b9ae to cb71607 Compare March 8, 2023 14:50

bjorn3 reviewed Mar 8, 2023

View reviewed changes

tests/codegen/global_asm.rs Show resolved Hide resolved

bjorn3 reviewed Mar 8, 2023

View reviewed changes

tests/ui/asm/issue-87802.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 8, 2023

View reviewed changes

tests/ui/symbol-names/x86-stdcall.rs

@@ -1,5 +1,6 @@

// build-pass

// only-x86-windows

// only-x86

// only-windows

Copy link

Member

bjorn3 Mar 8, 2023

Choose a reason for hiding this comment

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

Nice catch!

pietroalbini mentioned this pull request Mar 9, 2023

Add --print=all-target-specs-json unstable flag rust-lang/compiler-team#600

Closed

3 tasks

ehuss reviewed Mar 9, 2023

View reviewed changes

compiler/rustc_driver_impl/src/lib.rs Outdated Show resolved Hide resolved

compiler/rustc_driver_impl/src/lib.rs Outdated Show resolved Hide resolved

pietroalbini force-pushed the pa-compiletest-ignore branch from c4086c3 to 0bca8fc Compare March 10, 2023 14:30

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Mar 10, 2023

pietroalbini marked this pull request as ready for review March 10, 2023 15:11

pietroalbini force-pushed the pa-compiletest-ignore branch from 82d6b74 to 9be09a4 Compare March 14, 2023 09:30

lqd reviewed Mar 14, 2023

View reviewed changes

pietroalbini force-pushed the pa-compiletest-ignore branch from 9be09a4 to 3d1ccc0 Compare March 16, 2023 13:23

ehuss approved these changes Mar 26, 2023

View reviewed changes

pietroalbini force-pushed the pa-compiletest-ignore branch 2 times, most recently from f27d635 to 4fb3e65 Compare March 30, 2023 08:02

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 3, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 5, 2023

bors merged commit b2b676d into rust-lang:master Apr 5, 2023

rustbot added this to the 1.70.0 milestone Apr 5, 2023

pietroalbini deleted the pa-compiletest-ignore branch April 5, 2023 22:10

pietroalbini mentioned this pull request Apr 14, 2023

[compiletest] Add more test ignore reasons, needs- validation, and improved error messages #110319

Merged

djkoloski mentioned this pull request Apr 26, 2023

Compiletest no longer respects -Cpanic=abort in TARGET_RUSTCFLAGS after #108905 #110850

Closed

djkoloski mentioned this pull request May 11, 2023

Get current target config from --print=cfg #111472

Merged

jyn514 mentioned this pull request Jun 3, 2023

Don't compile rustc to self-test compiletest #112256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate `ignore` and `only` compiletest directive, and add human-readable ignore reasons #108905

Validate `ignore` and `only` compiletest directive, and add human-readable ignore reasons #108905

pietroalbini commented Mar 8, 2023

bjorn3 Mar 8, 2023

pietroalbini commented Mar 9, 2023

ehuss left a comment

pietroalbini commented Mar 10, 2023

ehuss commented Mar 10, 2023

bors commented Mar 14, 2023

pietroalbini commented Mar 14, 2023 •

edited

Loading

lqd Mar 14, 2023

pietroalbini commented Mar 16, 2023

pietroalbini commented Mar 21, 2023

bors commented Mar 21, 2023

ehuss left a comment

ehuss Mar 26, 2023

pietroalbini Mar 26, 2023

pietroalbini commented Mar 26, 2023

pietroalbini commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Apr 3, 2023

rust-log-analyzer commented Apr 3, 2023

bors commented Apr 5, 2023

scottmcm commented Apr 5, 2023

bors commented Apr 5, 2023

rust-timer commented Apr 5, 2023

Validate ignore and only compiletest directive, and add human-readable ignore reasons #108905

Validate ignore and only compiletest directive, and add human-readable ignore reasons #108905

Conversation

pietroalbini commented Mar 8, 2023

bjorn3 Mar 8, 2023

Choose a reason for hiding this comment

pietroalbini commented Mar 9, 2023

ehuss left a comment

Choose a reason for hiding this comment

pietroalbini commented Mar 10, 2023

ehuss commented Mar 10, 2023

bors commented Mar 14, 2023

pietroalbini commented Mar 14, 2023 • edited Loading

lqd Mar 14, 2023

Choose a reason for hiding this comment

pietroalbini commented Mar 16, 2023

pietroalbini commented Mar 21, 2023

bors commented Mar 21, 2023

ehuss left a comment

Choose a reason for hiding this comment

ehuss Mar 26, 2023

Choose a reason for hiding this comment

pietroalbini Mar 26, 2023

Choose a reason for hiding this comment

pietroalbini commented Mar 26, 2023

pietroalbini commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Apr 3, 2023

rust-log-analyzer commented Apr 3, 2023

bors commented Apr 5, 2023

scottmcm commented Apr 5, 2023

bors commented Apr 5, 2023

rust-timer commented Apr 5, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Validate `ignore` and `only` compiletest directive, and add human-readable ignore reasons #108905

Validate `ignore` and `only` compiletest directive, and add human-readable ignore reasons #108905

pietroalbini commented Mar 14, 2023 •

edited

Loading