Reconsider RUSTFLAGS artifact caching.

[ehuss]

When RUSTFLAGS is changed between invocations, this can trigger a complete rebuild. This can be annoying, and the only workaround is to use a separate target directory for each RUSTFLAGS setting. I think we should revisit how this works, and see if there is a way to address the problems.

Motivation:

• Changing RUSTFLAGS can be very expensive time-wise, and frustrating if your project relies on being able to swap between different settings.
• Supporting a shared build directory cache would be thwarted any time someone changes RUSTFLAGS.

History:

• Rustflags in metadata #6503 added RUSTFLAGS to the metadata hash, allowing independent caching. 1.33 (2019-02-28)
• Ignore remap-path-prefix in metadata hash. #6966 Removed --remap-path-prefix from the hash, because it broke reproducible builds. 1.37 (2019-08-15)
• Go back to not hashing RUSTFLAGS in -Cmetadata #7417 Reverted Rustflags in metadata #6503 because it broke profile guided optimization. 1.39 (2019-11-07)

Solutions?
I am uncertain what a good approach would be. Some rough ideas:

• Go back to hashing RUSTFLAGS, but only in the -C extra-filename, and not -C metadata. This should (probably) avoid the PGO problems, since the symbols won't change. However, this could still cause problems with reproducible builds if the rlib filename somehow leaks into the resulting binary (I don't have a reason to think it does, but it seems risky).
• Add a mechanism for specifying RUSTFLAGS so that the user can decide whether or not it gets hashed. (Maybe a separate env var, or some string in the current one, or a side setting?) I feel like there is a problem that specifying RUSTFLAGS is already too complicated (considering the multiple overrides) and at the same time not flexible enough.
• Provide first-class support for some of the things that people use RUSTFLAGS for today. For example, Cargo could have a "reproducible" mode where it tries to create a reproducible build, making whatever sacrifices are necessary. First class support for remap-path-prefix and PGO could also alleviate some of the problems.

[jyn514]

Provide first-class support for some of the things that people use RUSTFLAGS for today.

My main pain point with RUSTFLAGS is xd009642/tarpaulin#416 (comment), which has some niche flags and wouldn't be helped much by this. AFAIK all of those flags are codegen/linking related though, and don't affect .rlibs.

Add a mechanism for specifying RUSTFLAGS so that the user can decide whether or not it gets hashed.

Maybe there can be a way for rustc itself to tell cargo whether an option requires a re-compile? Something like rustc -Z needs-recompile "$RUSTFLAGS". I agree this shouldn't be left up to the user, my main complaint about make other build systems is they require you to specify build dependencies yourself.

I don't know enough about -C metadata and -C extra-filename to have an opinion.

[alexcrichton]

However, this could still cause problems with reproducible builds

I believe this does because the filenames of object files show up as the codegen unit names in dwarf debug info (and probably PDB?). I suspect that everything else in the binary will be the same, but builds differing only in extra-filename are likely to still produce different binaries.

Add a mechanism for specifying RUSTFLAGS so that the user can decide whether or not it gets hashed

I think this will work but I don't think it'll be too feasible if the default is to not hash RUSTFLAGS. The pain of rebuilding too much you only discover after-the-fact that you should have been hashing rustflags all along. I don't think we can hash-by-default though since it breaks the use cases you've mentioned.

Provide first-class support for some of the things that people use RUSTFLAGS for today

I do think a --reproducible flag or --pgo is something we should add to Cargo no matter what, regardless of how this RUSTFLAGS issue is settled.

---

Another possible solution could be adding a new variable like RUSTFLAGS_HASHED? That way if tooling sets it the weird env name is fine (maybe the case for tarpaulin?)

[epage]

Since #6503, we've tests for --remap-path-prefix. In that, we have a test that assumed -C extra-filename is the same as -C metadata and tests for -C extra-filename being stable across a RUSTFLAGS change to verify that -C metadata doesn't change.

cargo/tests/testsuite/rustflags.rs

Lines 1481 to 1497 in 9c3c48f

	// Ensure that --remap-path-prefix does not affect metadata hash.
	let p = project().file("src/lib.rs", "").build();
	p.cargo("build").run();
	let rlibs = p
	.glob("target/debug/deps/*.rlib")
	.collect::<Result<Vec<_>, _>>()
	.unwrap();
	assert_eq!(rlibs.len(), 1);
	p.cargo("clean").run();
	let check_metadata_same = || {
	let rlibs2 = p
	.glob("target/debug/deps/*.rlib")
	.collect::<Result<Vec<_>, _>>()
	.unwrap();
	assert_eq!(rlibs, rlibs2);
	};

@ehuss or @weihanglo any thoughts on another way of testing for -C metadata stability?

[epage]

Terrible thought: run a regex on stderr to capture all -Cmetadata=<something> parameters and then compare those between runs.