Ensure that compile-flags arguments are the last in UI tests #103298

pietroalbini · 2022-10-20T10:46:08Z

Before this PR, compiletest would add -L path/to/aux at the end of the rustc flags, even after the custom ones set with the compile-flags header comment. This made it impossible to check how rustc would behave when a flag requiring an argument was passed without the argument, because the argument would become -L.

This PR fixes that by adding the -L path/to/aux before the arguments defined in compile-flags, at least for UI tests. Other test suites might either be fixed as well by this change, or still present the old behavior (-L is now always passed before, but other tests suites might add additional flags after the custom ones).

rust-highfive · 2022-10-20T10:46:11Z

r? @jyn514

(rust-highfive has picked a reviewer for you, use r? to override)

jyn514 · 2022-10-20T13:55:43Z

src/tools/compiletest/src/runtest.rs

@@ -1869,6 +1877,7 @@ impl<'test> TestCx<'test> {
        output_file: TargetLocation,
        emit_metadata: EmitMetadata,
        allow_unused: AllowUnused,
+        link_to_aux: LinkToAux,


for my sanity: this new argument is necessary because there's no Command available until after this function returns it. We might want to refactor at some point to allow passing custom flags without needing all of them to be passed into this function.

jyn514 · 2022-10-20T13:57:03Z

src/tools/compiletest/src/runtest.rs

+            AllowUnused::No,
+            LinkToAux::Yes,
+        );
+        rustc.arg("--emit=llvm-ir");


this will have the same problem as -L aux_dir, right? should we fix that here too?

Hmm, there are multiple places around that add flags. If we wanted to remove all the chances of this happening in the future, we should probably have a

prepare_and_run_rustc(..., |cmd| { cmd.arg("--emit=llvm-ir") });

Would that be something you'd like?

hmm, that sounds like a lot of changes to use the new API - can we add a src/test/ui test that verifies this works instead?

Thanks for adding the test! Looks like this is still broken for codegen tests though, can you add a test for that case too?

Fixed codegen and asm test suites. I couldn't add a test though, because both suites expect the compilation to pass, and // build-fail doesn't work in non-UI suites.

Before this commit, compiletest would add `-L path/to/aux` at the end of the rustc flags, even after the custom ones set with the compile-flags header comment. This made it impossible to check how rustc would behave when a flag requiring an argument was passed without the argument, because the argument would become `-L`. This PR fixes that by adding the `-L path/to/aux` before the arguments defined in compile-flags, at least for UI tests. Other test suites might either be fixed as well by this change, or still present the old behavior.

src/test/ui/compiletest-compile-flags-last.rs

src/tools/tidy/src/ui_tests.rs

jyn514 · 2022-10-27T15:10:56Z

src/tools/compiletest/src/runtest.rs

+            AllowUnused::No,
+            LinkToAux::Yes,
+        );
+        rustc.arg("--emit=llvm-ir");


Thanks for adding the test! Looks like this is still broken for codegen tests though, can you add a test for that case too?

pietroalbini · 2022-11-03T11:10:51Z

@rustbot review

jyn514 · 2022-11-03T13:03:30Z

Looks great, thanks!

@bors r+

bors · 2022-11-03T13:03:32Z

📌 Commit 4c55b29 has been approved by jyn514

It is now in the queue for this repository.

bors · 2022-11-04T22:24:49Z

⌛ Testing commit 4c55b29 with merge 81ff7e7...

bors · 2022-11-05T01:20:34Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 81ff7e7 to master...

rust-timer · 2022-11-05T03:39:48Z

Finished benchmarking commit (81ff7e7): comparison URL.

Overall result: ❌ regressions - 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.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.3%, 0.3%]	1

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)	7.2%	[7.2%, 7.2%]	1
Improvements ✅ (primary)	-0.9%	[-0.9%, -0.9%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-0.9%, -0.9%]	1

Cycles

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-3.1%, -2.2%]	3
All ❌✅ (primary)	-	-	0

…yn514 Ensure that compile-flags arguments are the last in UI tests Before this PR, compiletest would add `-L path/to/aux` at the end of the rustc flags, even after the custom ones set with the compile-flags header comment. This made it impossible to check how rustc would behave when a flag requiring an argument was passed without the argument, because the argument would become `-L`. This PR fixes that by adding the `-L path/to/aux` before the arguments defined in compile-flags, at least for UI tests. Other test suites might either be fixed as well by this change, or still present the old behavior (`-L` is now always passed before, but other tests suites might add additional flags after the custom ones).

compiletest: Simplify the choice of `--emit` mode for assembly tests Tiny little cleanup that I noticed while working on rust-lang#131524. No functional change. Historically, the original code structure (rust-lang#58791) predates the `Emit` enum (rust-lang#103298), so it was manually adding `--emit` flags to the compiler invocation. But now the match can just evaluate to the appropriate `Emit` value directly.

Rollup merge of rust-lang#131525 - Zalathar:emit-asm, r=jieyouxu compiletest: Simplify the choice of `--emit` mode for assembly tests Tiny little cleanup that I noticed while working on rust-lang#131524. No functional change. Historically, the original code structure (rust-lang#58791) predates the `Emit` enum (rust-lang#103298), so it was manually adding `--emit` flags to the compiler invocation. But now the match can just evaluate to the appropriate `Emit` value directly.

rust-highfive assigned jyn514 Oct 20, 2022

rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Oct 20, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2022

jyn514 reviewed Oct 20, 2022

View reviewed changes

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 24, 2022

pietroalbini force-pushed the pa-compile-flags-last branch from 8015e81 to f869abb Compare October 25, 2022 08:37

pietroalbini added 2 commits October 25, 2022 10:41

add a test to verify that compile-flags are provided last

9546490

pietroalbini force-pushed the pa-compile-flags-last branch from f869abb to 9546490 Compare October 25, 2022 08:42

jyn514 reviewed Oct 27, 2022

View reviewed changes

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 Oct 27, 2022

pietroalbini added 3 commits November 3, 2022 11:47

check for pattern in compiletest-compile-flags-last

1adf83b

move compiletest's own ui tests into a subdirectory

5ed753c

put custom flags as last in codegen and asm tests

4c55b29

rustbot 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 Nov 3, 2022

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 3, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 5, 2022

bors merged commit 81ff7e7 into rust-lang:master Nov 5, 2022

rustbot added this to the 1.67.0 milestone Nov 5, 2022

bors mentioned this pull request Nov 5, 2022

Expose compiletest as a library for use in Clippy #103266

Closed

matthiaskrgr mentioned this pull request Nov 21, 2023

ice: ProjectionKinds Deref and Field were mismatched #118144

Closed

Jules-Bertholet mentioned this pull request Dec 29, 2023

ice: Field projection applied to a type other than Adt or Tuple with -Wrust-2021-incompatible-closure-captures #119382

Open

Zalathar mentioned this pull request Oct 11, 2024

compiletest: Simplify the choice of --emit mode for assembly tests #131525

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that compile-flags arguments are the last in UI tests #103298

Ensure that compile-flags arguments are the last in UI tests #103298

pietroalbini commented Oct 20, 2022

rust-highfive commented Oct 20, 2022

jyn514 Oct 20, 2022

jyn514 Oct 20, 2022

pietroalbini Oct 20, 2022

jyn514 Oct 20, 2022

pietroalbini Oct 24, 2022

jyn514 Oct 27, 2022

pietroalbini Nov 3, 2022

jyn514 Oct 27, 2022

pietroalbini commented Nov 3, 2022

jyn514 commented Nov 3, 2022

bors commented Nov 3, 2022

bors commented Nov 4, 2022

bors commented Nov 5, 2022

rust-timer commented Nov 5, 2022

Ensure that compile-flags arguments are the last in UI tests #103298

Ensure that compile-flags arguments are the last in UI tests #103298

Conversation

pietroalbini commented Oct 20, 2022

rust-highfive commented Oct 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietroalbini commented Nov 3, 2022

jyn514 commented Nov 3, 2022

bors commented Nov 3, 2022

bors commented Nov 4, 2022

bors commented Nov 5, 2022

rust-timer commented Nov 5, 2022

Overall result: ❌ regressions - no action needed