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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e57314
Pass --out-dir for run-pass tests in Compiletest
lionellloh May 31, 2022
6a1933d
format code
lionellloh May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/tools/compiletest/src/runtest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@czzrr czzrr Aug 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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

}
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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

}
}
}
Expand Down