Compiletest should set RUST_SAVE_ANALYSIS_CONFIG for run-pass tests

[jyn514]

#96777 was necessary because a test used both -Zsave-analysis and run-pass. That generated an untracked save-analysis-temp file in the root directory. That behavior is not great - we shouldn't do that even if the test is marked run-pass. We can avoid it by passing --out-dir config.build_base in compiletest:

rust/src/tools/compiletest/src/runtest.rs

Lines 1895 to 1907 in fee75fb

	match output_file {
	TargetLocation::ThisFile(path) => {
	rustc.arg("-o").arg(path);
	}
	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);
	}
	}
	}

We should also investigate why this wasn't caught by CI - it makes the source directory read only, which should have prevented this being merged.

@rustbot label +E-easy +A-testsuite +E-mentor

[jyn514]

We should also investigate why this wasn't caught by CI - it makes the source directory read only, which should have prevented this being merged.

I think it might run x.py from a working directory other than the source root.

[kingdido999]

@rustbot claim

[lionellloh]

@kingdido999 are you still working on it?

[lionellloh]

@rustbot claim

[lionellloh]

Hey @jyn514, thanks for offering to mentor!

Some questions -

What is the difference between check-pass and run-pass?

That generated an untracked save-analysis-temp file in the root directory.

What is the best way to repro this?

[jyn514]

@lionellloh "the difference between cargo check and cargo run" - the first just runs rustc --emit metadata while the second actually generates a binary and runs it. The reason it causes trouble here is because save-analysis uses --out-dir to determine where to put the generated JSON file, and defaults to the working directory if it isn't passed.

What is the best way to repro this?

Change any existing UI test with -Z save-analysis and check-pass to be run-pass instead.

[lionellloh]

@jyn514

Qq regarding dev efficiency:
I managed to repro the issue by running ./x.py test src/test/ui/const-generics/const-argument-non-static-lifetime.rs and changing check-pass to be run-pass.

The command ran for 10 minutes. I deleted the save-analysis-temp folder after the repro, made my patch and tried to run again. I realised the tests were ignored this time. I deleted the build folder so that the tests may not be ignored, but it seems like that causes the test to run for a pretty long time again. I wonder if that is avoidable?

One more n00b qn:
I am a little curious how // [full] run-pass is parsed. They seem like comments to me. Why does changing it change anything?

[jyn514]

@lionellloh touch src/test/ui/const-generics/const-argument-non-static-lifetime.rs should work and be quite fast.

[jyn514]

I think there's also a force-rerun flag or something like that.

[lionellloh]

Pardon the noob qns again -

Here's my latest attempt, posting because I am not understanding the behaviour I am seeing.

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

I ran the compiled the test again at 15:32

I can see that the json file is freshly written:

lionellloh@lionellloh-mbp rust % ls -al ./build/x86_64-apple-darwin/test/ui/save-analysis 
total 16
drwxr-xr-x    3 lionellloh  staff    96 May 30 14:37 .
drwxr-xr-x  297 lionellloh  staff  9504 May 30 15:26 ..
-rw-r--r--    1 lionellloh  staff  7193 May 30 15:32 const_argument_non_static_lifetime.json

But at the same time I also see that the json is save-analysis-temp is freshly written. In cases when the temp folder is deleted, it is recreated and both files are written:

lionellloh@lionellloh-mbp rust % ls -al save-analysis-temp 
total 16
drwxr-xr-x   3 lionellloh  staff    96 May 30 15:32 .
drwxr-xr-x  33 lionellloh  staff  1056 May 30 15:32 ..
-rw-r--r--   1 lionellloh  staff  7714 May 30 15:32 const_argument_non_static_lifetime.json

[lionellloh]

The reason it causes trouble here is because save-analysis uses --out-dir to determine where to put the generated JSON file, and defaults to the working directory if it isn't passed

Why do you say it was not passed? It seems like path is passed all the time? I think the symptom might be manifesting for a different reason.

[jyn514]

@lionellloh path is passed all the time, but sometimes passed with -o instead of --out-dir. You can confirm this is the issue by running your x test command with -v, copy pasting the rustc command, adding an --out-dir argument, and making sure that the JSON file is no longer generated in the current directory.

[lionellloh]

@jyn514 So I think the code block is run twice, once matching on TargetLocation::ThisFile(path) and once matching on TargetLocation::ThisDirectory(path). The tricky bit is that in the first match. If we set it using rustc.arg("--out-dir").arg(&self.config.build_base) the binary also gets directed to somewhere else, and cannot be found.

If we set it using rustc.arg("--out-dir").arg(path), it seems that the json gets saved first, which converts .../test/ui/const-generics/const-argument-non-static-lifetime.full/a into a folder which eventually contains the binary and the json. This causes a panic when the binary later cannot be found. :/

[lionellloh]

Ok I think I got it, let me submit a PR for review.

[jyn514]

For anyone planning to pick up this issue: take a look at #97573 (comment), my instructions here are not quite right.

[passing both -o and --out-dir] fails 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 think 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.

[voidcoefficient]

@lionellloh Are you going to continue working on this issue?

[chenyukang]

@rustbot claim

[czzrr]

@rustbot claim

[azadnn]

@rustbot claim

[azadnn]

Hi @jyn514,
I have spent some time looking at this issue and I can see that, as you indicated above, the (only) likely solution is to set the RUST_SAVE_ANALYSIS_CONFIG environment variable. This is because rustc_save_analysis is always called with config=None as can be seen here:

rust/compiler/rustc_driver/src/lib.rs

Lines 378 to 386 in 05c1ac0

	sess.time("save_analysis", || {
	save::process_crate(
	tcx,
	crate_name,
	compiler.input(),
	None,
	DumpHandler::new(compiler.output_dir().as_deref(), crate_name),
	)
	});

I see an example of the RUST_SAVE_ANALYSIS_CONFIG env variable though output_file is null here:

rust/src/bootstrap/builder.rs

Lines 1795 to 1801 in 40ba0e8

	rustflags.arg("-Zsave-analysis");
	cargo.env(
	"RUST_SAVE_ANALYSIS_CONFIG",
	"{\"output_file\": null,\"full_docs\": false,\
	\"pub_only\": true,\"reachable_only\": false,\
	\"distro_crate\": true,\"signatures\": false,\"borrow_data\": false}",
	);

My observation is that when run-pass tests are run, output_file matches TargetLocation::ThisFile(path) which is where I propose to set the environment variable:

rust/src/tools/compiletest/src/runtest.rs

Lines 1995 to 1998 in 40ba0e8

	match output_file {
	TargetLocation::ThisFile(path) => {
	rustc.arg("-o").arg(path);
	}

I should post a PR soon

[jyn514]

Save-analysis has been removed from the compiler.