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.

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

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

Merged
merged 5 commits into from
Nov 5, 2022

Conversation

pietroalbini
Copy link
Member

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).

@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Oct 20, 2022
@rust-highfive
Copy link
Collaborator

r? @jyn514

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2022
@@ -1869,6 +1877,7 @@ impl<'test> TestCx<'test> {
output_file: TargetLocation,
emit_metadata: EmitMetadata,
allow_unused: AllowUnused,
link_to_aux: LinkToAux,
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 24, 2022
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 Outdated Show resolved Hide resolved
src/tools/tidy/src/ui_tests.rs Outdated Show resolved Hide resolved
AllowUnused::No,
LinkToAux::Yes,
);
rustc.arg("--emit=llvm-ir");
Copy link
Member

Choose a reason for hiding this comment

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

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

@jyn514 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
Copy link
Member Author

@rustbot review

@rustbot 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
@jyn514
Copy link
Member

jyn514 commented Nov 3, 2022

Looks great, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 3, 2022

📌 Commit 4c55b29 has been approved by jyn514

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 3, 2022
@bors
Copy link
Contributor

bors commented Nov 4, 2022

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

@bors
Copy link
Contributor

bors commented Nov 5, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 5, 2022
@bors bors merged commit 81ff7e7 into rust-lang:master Nov 5, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 5, 2022
@rust-timer
Copy link
Collaborator

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

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…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).
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 11, 2024
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-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants