Do not lock the artifact-dir for check builds

[ranger-ross]

What does this PR try to resolve?

This PR modifies the locking logic to avoid locking artifact-dir for check builds.
Part of #4282

Note: This change is not behind -Zbuild-dir-new-layout or -Zfine-grain-locking unstable flags.

How to test and review this PR?

See the tests included in the PR.
You can run cargo check to verify.

r? @epage

[weihanglo]

This should be in check command test file maybe?
https://github.com/rust-lang/cargo/blob/master/tests/testsuite/check.rs

[ranger-ross]

ah good idea. I was thinking about putting it in https://github.com/rust-lang/cargo/blob/master/tests/testsuite/artifact_dep.rs but noticed that file only has test for the unstable --artifact-dir flag.

master/tests/testsuite/check.rs seems like a good place for this

[weihanglo]

Should we also add one test asserting that no files are generated to the artifact directory from cargo check?

(Maybe we already have one though)

[ranger-ross]

Added a test in da0dfd3

[epage]

Instead of conditionally grabbing the lock, can we put the entire artifact layout in an Option? That has the potential to help us find bugs but unsure how messy that would be.

[ranger-ross]

I looked into this and it got pretty hairy. Switching from T to Option<T> is not too bad but there quiet a few places during compilation where the paths are used but not written to.

For example, in build_runner::prepare we build up the root_output

cargo/src/cargo/core/compiler/build_runner/mod.rs

Lines 395 to 397 in 8c4a25c

	self.compilation
	.root_output
	.insert(kind, layout.artifact_dir().dest().to_path_buf());

and we use this in fill_env()

cargo/src/cargo/core/compiler/compilation.rs

Lines 308 to 311 in 8c4a25c

	search_path.extend(super::filter_dynamic_search_path(
	self.native_dirs.iter(),
	&self.root_output[&kind],
	));

[epage]

Doesn't that make the case even stronger? We are trying to read from a location we do not write to and do not have locked.

[ranger-ross]

I looked into this and I think this is a good call. I ended up missing that we still need to lock the artifact-dir for cargo check --timings. Making this Option caused a test to fail instead of silently write files without locking.

I split out this change into a refactor commit before changing the behavior.

[epage]

Instead of intent, can we check if artifacts are produced? Do we know that?

[ranger-ross]

I checked and I think it may be possible to use some of the logic from CompilationFiles output but we would likely need to duplicate it as we need to construct Layout before CompilationFiles.

See https://github.com/rust-lang/cargo/blob/master/src/cargo/core/compiler/build_runner/compilation_files.rs#L405

Let me know if there is a better way to get this data.

[epage]

Hmm, that can get messy. We could create an enum for the base path and calculate the full path later but unsure how well that will work out.

However, this does highlight a need for either this or conditionally grabbing the lock (#16230 (comment))

cargo/src/cargo/core/compiler/build_runner/compilation_files.rs

Lines 529 to 540 in 8c4a25c

	if bcx.build_config.sbom && bcx.gctx.cli_unstable().sbom {
	let sbom_files: Vec<_> = outputs
	.iter()
	.filter(|o| matches!(o.flavor, FileFlavor::Normal | FileFlavor::Linkable))
	.map(|output| OutputFile {
	path: Self::append_sbom_suffix(&output.path),
	hardlink: output.hardlink.as_ref().map(Self::append_sbom_suffix),
	export_path: output.export_path.as_ref().map(Self::append_sbom_suffix),
	flavor: FileFlavor::Sbom,
	})
	.collect();
	outputs.extend(sbom_files.into_iter());

We would have been writing out SBOMs without holding the lock. If we don't feel SBOMs are relevant for cargo check then making the layout optional (to catch problems like this) is sufficient. If not, then we need to somehow be aware of this for locking and then that starts to look like needing to resolve this item.

[epage]

Note: if we change SBOMs, lets do that in its own PR and make a note against rust-lang/rfcs#3553

CC @arlosi

[epage]

Yikes, that is a pretty big exception for us to make that can negate this.

[ranger-ross]

yeah, its unfortunate.

I think we still need to lock the artifact-dir since it could be read during compile time like

let tool_bytes = include_bytes!(env!("CARGO_BIN_EXE_foo"));

Though, I'd imagine this would be a pretty rare usecase?

[epage]

The case where this could come up is with artifact deps though I don't know if those are based on uplifted binaries or not

[epage]

Also, I've wondered about moving CARGO_BIN_EXE_* to also work at runtime for tests. Encouraging a migration to that would make it clearer what the intent is with these.

#t-cargo > cargo_bin_exe and tests @ 💬

[ranger-ross]

okay I was finally able to check on this.

It does point to the uplifted bin location in artifact-dir.
However for cargo check --tests that path points to a nonexisting location in artifact-dir.

Interestingly, I tried doing include_bytes!(env!("CARGO_BIN_EXE_bar")) and I get a compiler error:

error: couldn't read `/home/ross/test-artifact-deps-uplift/target/debug/bar`: No such file or directory (os error 2)
 --> tests/foo.rs:4:13
  |
4 |     let a = include_bytes!(env!("CARGO_BIN_EXE_bar"));
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Even though I see the bar bin in that location after the build. (pipelined build race condition perhaps?)

I am on the fence about the best thing to do here. The options I could think of are:

1. We could add an escape hatch to get the artifact_dir() location from Layout even if we don't lock it.
2. We could point to the pre-uplifted bin for check builds (though it might not exist, same as current behavior)
3. We could have a dummy value for check builds to satisfy compilation.

I think if I had to pick, I'd go with option 2 but input on this is welcome. :D

[epage]

Considering the value is unusable for compilation, I would lean towards (3).

• This protects against race conditions where you see an old version of things
• Pre-uplifted is never a valid answer because it will have the -Cextra-filename

rustc reports CARGO_BIN_EXE_bar in its dep-info file. Does cargo preserve that in its own or filter it out? If it preserves it then that means we are ruining people's caching. We can at least make things more cache friendly with a dummy value. Moving people to reading at runtime would help even more.

[ranger-ross]

Does cargo preserve that in its own or filter it out?

I checked this and it appears that cargo is filtering it out in translate_dep_info

cargo/src/cargo/core/compiler/fingerprint/dep_info.rs

Lines 338 to 343 in 745aae6

	on_disk_info.env.retain(|(key, _)| {
	ManifestMetadata::should_track(key)
	|| env_config.contains_key(key)
	|| !rustc_cmd.get_envs().contains_key(key)
	|| key == CARGO_ENV
	});

So I suppose a dummy value would be fine since we wouldn't be invalidating fingerprints. 👍

[ranger-ross]

I went ahead and updated this PR to add a placeholder CARGO_BIN_EXE_<name>=placeholder:<name> for check builds.
Also removing the need to lock the artifact-dir for check --test builds

[weihanglo]

Probably not really on topic, but I recently felt it inconvenient that CARGO_TARGET_TMPDIR is with build-dir. I usually cd target/tmp to access test files but now I need to navigate to a shared build-dir directory.

I wonder if we should have a project private directory under artifact directory, so that we only lock that for bins

[weihanglo]

Thought came from me reading this thread #16230 (comment), but placeholder method looks nice also

[weihanglo]

bin checks are never usable, forget about my comments