Pass --out-dir for run-pass tests in Compiletest #97573

lionellloh · 2022-05-31T02:02:57Z

This addresses the issue #96928

Ran the following command with a local patch:

Local Patch

--- a/src/test/ui/const-generics/const-argument-non-static-lifetime.rs
+++ b/src/test/ui/const-generics/const-argument-non-static-lifetime.rs
@@ -1,4 +1,4 @@
-// [full] check-pass
+// [full] run-pass

Command

./x.py test  --bless src/test/ui/const-generics/const-argument-non-static-lifetime.rs

Before Change behavior: We create a temp folder save-analysis-temp which contains the json as well as one in the build folder.

After Change Behaviour: Only one json file is created in the build folder

lionellloh@lionellloh-mbp rust % find ./ -name "const_argument_non_static_lifetime.json"
.//build/x86_64-apple-darwin/test/ui/save-analysis/const_argument_non_static_lifetime.json

rust-highfive · 2022-05-31T02:02:59Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

Mark-Simulacrum · 2022-06-01T02:18:36Z

r? @jyn514

jyn514

Sorry for the long delay. I think I had the wrong idea on the original issue unfortunately, but I left suggestions for how to fix the CI failure.

jyn514 · 2022-06-08T03:01:22Z

src/tools/compiletest/src/runtest.rs

            }
            TargetLocation::ThisDirectory(path) => {
                if is_rustdoc {
                    // `rustdoc` uses `-o` for the output directory.
                    rustc.arg("-o").arg(path);
                } else {
-                    rustc.arg("--out-dir").arg(path);
+                    rustc.arg("--out-dir").arg(&self.config.build_base);


These are not the same. build_base is the same for all tests in the suite, while path is based on the test name:

[src/tools/compiletest/src/runtest.rs:1914] &path = "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/test/ui/const-generics/const-argument-non-static-lifetime.min" [src/tools/compiletest/src/runtest.rs:1914] &self.config.build_base = "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/test/ui"

The difference matters when - like in this case - the test creates a subdirectory in the out-dir. If multiple tests create /home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/test/ui/save-analysis, then it's impossible to test the output, because it will non-deterministic when tests are run in parallel. I think we should keep using path here.

jyn514 · 2022-06-08T03:02:08Z

src/tools/compiletest/src/runtest.rs

@@ -1905,13 +1905,14 @@ impl<'test> TestCx<'test> {
        match output_file {
            TargetLocation::ThisFile(path) => {
                rustc.arg("-o").arg(path);
+                rustc.arg("--out-dir").arg(&self.config.build_base);


This has the same bug as below - we should find a way to pass path to the ThisFile branch. It looks like usually ThisFile(path) is derived from the directory name:

[src/tools/compiletest/src/runtest.rs:1905] output_file = ThisFile( "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/test/ui/const-generics/const-argument-non-static-lifetime.full/a", )

So likely we can pass arg("--out-dir").arg(path.parent()).

jyn514 · 2022-06-08T03:08:45Z

src/tools/compiletest/src/runtest.rs

@@ -1905,13 +1905,14 @@ impl<'test> TestCx<'test> {
        match output_file {
            TargetLocation::ThisFile(path) => {
                rustc.arg("-o").arg(path);
+                rustc.arg("--out-dir").arg(&self.config.build_base);


This is failing the UI test:

The actual stderr differed from the expected stderr. Actual stderr saved to /home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/test/ui/const-generics/const-argument-non-static-lifetime.full/const-argument-non-static-lifetime.full.stderr To update references, rerun the tests and pass the `--bless` flag To only update this specific test, also pass `--test-args const-generics/const-argument-non-static-lifetime.rs` error in revision `full`: 1 errors occurred comparing output. status: exit status: 0 command: "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/home/jnelson/rust-lang/rust/src/test/ui/const-generics/const-argument-non-static-lifetime.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--cfg" "full" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-C" "prefer-dynamic" "-o" "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/test/ui/const-generics/const-argument-non-static-lifetime.full/a" "--out-dir" "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/test/ui/const-generics/const-argument-non-static-lifetime.full" "-Crpath" "-O" "-Cdebuginfo=0" "-Lnative=/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Zsave-analysis" "-L" "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/test/ui/const-generics/const-argument-non-static-lifetime.full/auxiliary" stdout: none --- stderr ------------------------------- warning: ignoring --out-dir flag due to -o flag warning: 1 warning emitted ------------------------------------------ failures: [ui] src/test/ui/const-generics/const-argument-non-static-lifetime.rs#full

I think the root issue is that --out-dir is not ignored by save-analysis, even if -o is also passed. I wonder if we should be using it at all - I see that there's some code in save-analysis that reads the config from an environment variable:

rust/compiler/rustc_save_analysis/src/lib.rs

Line 1006 in b17e9d7

match env::var_os("RUST_SAVE_ANALYSIS_CONFIG") {

I wonder if we should be using that instead, to avoid touching anything other than save-analysis itself? Try doing that instead of adding the --out-dir path and see if it works.

@jyn514
From the stderr, --out-dir is ignored:

warning: ignoring --out-dir flag due to -o flag

@chenyukang the stderr is wrong. out-dir is ignored by most of the compiler when -o is passed, but not by save-analysis.

@jyn514 Changing src/test/ui/const-generics/const-argument-non-static-lifetime.rs to run-pass and running the test with rustc.arg("--out-dir").arg(self.output_base_dir()); produces the save-analysis file build/x86_64-unknown-linux-gnu/test/ui/const-generics/const-argument-non-static-lifetime.full/save-analysis/const_argument_non_static_lifetime.json without save-analysis-temp in the root directory as desired.

The test fails because of the stderr warning you got.
Is this a problem?

@czzrr please ask these questions on Zulip or the tracking issue. My github notifications are a mess and I'm very unlikely to see any comment on a closed PR.

jyn514 · 2022-06-17T17:53:34Z

Hi @lionellloh, have you had time to look at this? Are the next steps clear to you? Happy to help if you're confused :)

lionellloh · 2022-06-17T21:42:51Z

Hi @lionellloh, have you had time to look at this? Are the next steps clear to you? Happy to help if you're confused :)

Hey @jyn514, thanks for the kind offer! I will get back to this soon! (Maybe right after the weekend)

JohnCSimon · 2022-07-23T23:42:35Z

Ping from triage:
@lionellloh - What is the status of this PR? Can you fix the build failures? If you're not moving forward with this please close it.

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Dylan-DPC · 2022-07-31T17:59:33Z

Closing this as inactive

Pass --out-dir for run-pass tests in Compiletest

4e57314

rust-highfive assigned Mark-Simulacrum May 31, 2022

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

This comment has been minimized.

Sign in to view

format code

6a1933d

rust-highfive assigned jyn514 and unassigned Mark-Simulacrum Jun 1, 2022

jyn514 suggested changes Jun 8, 2022

View reviewed changes

Dylan-DPC closed this Jul 31, 2022

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 31, 2022

jyn514 mentioned this pull request Aug 2, 2022

Compiletest should set RUST_SAVE_ANALYSIS_CONFIG for run-pass tests #96928

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass --out-dir for run-pass tests in Compiletest #97573

Pass --out-dir for run-pass tests in Compiletest #97573

lionellloh commented May 31, 2022 •

edited

Loading

rust-highfive commented May 31, 2022

This comment has been minimized.

Mark-Simulacrum commented Jun 1, 2022

jyn514 left a comment

jyn514 Jun 8, 2022

jyn514 Jun 8, 2022

jyn514 Jun 8, 2022

chenyukang Aug 6, 2022

jyn514 Aug 24, 2022

czzrr Aug 28, 2022 •

edited

Loading

jyn514 Oct 12, 2022

jyn514 commented Jun 17, 2022

lionellloh commented Jun 17, 2022

JohnCSimon commented Jul 23, 2022

Dylan-DPC commented Jul 31, 2022

Pass --out-dir for run-pass tests in Compiletest #97573

Pass --out-dir for run-pass tests in Compiletest #97573

Conversation

lionellloh commented May 31, 2022 • edited Loading

rust-highfive commented May 31, 2022

This comment has been minimized.

Mark-Simulacrum commented Jun 1, 2022

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 Jun 8, 2022

Choose a reason for hiding this comment

jyn514 Jun 8, 2022

Choose a reason for hiding this comment

jyn514 Jun 8, 2022

Choose a reason for hiding this comment

chenyukang Aug 6, 2022

Choose a reason for hiding this comment

jyn514 Aug 24, 2022

Choose a reason for hiding this comment

czzrr Aug 28, 2022 • edited Loading

Choose a reason for hiding this comment

jyn514 Oct 12, 2022

Choose a reason for hiding this comment

jyn514 commented Jun 17, 2022

lionellloh commented Jun 17, 2022

JohnCSimon commented Jul 23, 2022

Dylan-DPC commented Jul 31, 2022

lionellloh commented May 31, 2022 •

edited

Loading

czzrr Aug 28, 2022 •

edited

Loading