Do not lock the artifact-dir for check builds + fix uplifting (#16307)

### What does this PR try to resolve?

This PR un-reverts the changes from #16230 as well as adds a bug fix for
a crash reported in #16297

Note: The bug fix relies on behavior in the reverted code, so I added
that back before the bug fix

### How to test and review this PR?

I added a new check test that verifies the existing behavior (crash) and
is updated to verify no files are added to the target dir with no crash.

r? @epage
cc: @stewartadam (thanks, again for the bug report)

Fixes #16305

Author: epage

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

@@ -455,18 +455,13 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
         let uplift_path = if unit.target.is_example() {
             // Examples live in their own little world.
             self.layout(unit.kind)
-                .artifact_dir()
-                .expect("artifact-dir was not locked")
+                .artifact_dir()?
                 .examples()
                 .join(filename)
         } else if unit.target.is_custom_build() {
             self.build_script_dir(unit).join(filename)
         } else {
-            self.layout(unit.kind)
-                .artifact_dir()
-                .expect("artifact-dir was not locked")
-                .dest()
-                .join(filename)
+            self.layout(unit.kind).artifact_dir()?.dest().join(filename)
         };
         if from_path == uplift_path {
             // This can happen with things like examples that reside in the

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

@@ -6,7 +6,7 @@ use std::sync::{Arc, Mutex};
 
 use crate::core::PackageId;
 use crate::core::compiler::compilation::{self, UnitOutput};
-use crate::core::compiler::{self, Unit, artifact};
+use crate::core::compiler::{self, Unit, UserIntent, artifact};
 use crate::util::cache_lock::CacheLockMode;
 use crate::util::errors::CargoResult;
 use annotate_snippets::{Level, Message};
@@ -352,11 +352,32 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
     #[tracing::instrument(skip_all)]
     pub fn prepare_units(&mut self) -> CargoResult<()> {
         let dest = self.bcx.profiles.get_dir_name();
-        let host_layout = Layout::new(self.bcx.ws, None, &dest)?;
+        // We try to only lock the artifact-dir if we need to.
+        // For example, `cargo check` does not write any files to the artifact-dir so we don't need
+        // to lock it.
+        let must_take_artifact_dir_lock = match self.bcx.build_config.intent {
+            UserIntent::Check { .. } => {
+                // Generally cargo check does not need to take the artifact-dir lock but there is
+                // one exception: If check has `--timings` we still need to lock artifact-dir since
+                // we will output the report files.
+                !self.bcx.build_config.timing_outputs.is_empty()
+            }
+            UserIntent::Build
+            | UserIntent::Test
+            | UserIntent::Doc { .. }
+            | UserIntent::Doctest
+            | UserIntent::Bench => true,
+        };
+        let host_layout = Layout::new(self.bcx.ws, None, &dest, must_take_artifact_dir_lock)?;
         let mut targets = HashMap::new();
         for kind in self.bcx.all_kinds.iter() {
             if let CompileKind::Target(target) = *kind {
-                let layout = Layout::new(self.bcx.ws, Some(target), &dest)?;
+                let layout = Layout::new(
+                    self.bcx.ws,
+                    Some(target),
+                    &dest,
+                    must_take_artifact_dir_lock,
+                )?;
                 targets.insert(target, layout);
             }
         }

src/cargo/core/compiler/layout.rs

@@ -127,6 +127,7 @@ impl Layout {
         ws: &Workspace<'_>,
         target: Option<CompileTarget>,
         dest: &str,
+        must_take_artifact_dir_lock: bool,
     ) -> CargoResult<Layout> {
         let is_new_layout = ws.gctx().cli_unstable().build_dir_new_layout;
         let mut root = ws.target_dir();
@@ -150,15 +151,6 @@ impl Layout {
         // actual destination (sub)subdirectory.
         paths::create_dir_all(dest.as_path_unlocked())?;
 
-        // For now we don't do any more finer-grained locking on the artifact
-        // directory, so just lock the entire thing for the duration of this
-        // compile.
-        let artifact_dir_lock = if is_on_nfs_mount(root.as_path_unlocked()) {
-            None
-        } else {
-            Some(dest.open_rw_exclusive_create(".cargo-lock", ws.gctx(), "artifact directory")?)
-        };
-
         let build_dir_lock = if root == build_root || is_on_nfs_mount(build_root.as_path_unlocked())
         {
             None
@@ -169,21 +161,38 @@ impl Layout {
                 "build directory",
             )?)
         };
-        let root = root.into_path_unlocked();
         let build_root = build_root.into_path_unlocked();
-        let dest = dest.into_path_unlocked();
         let build_dest = build_dest.as_path_unlocked();
         let deps = build_dest.join("deps");
         let artifact = deps.join("artifact");
 
-        Ok(Layout {
-            artifact_dir: Some(ArtifactDirLayout {
+        let artifact_dir = if must_take_artifact_dir_lock {
+            // For now we don't do any more finer-grained locking on the artifact
+            // directory, so just lock the entire thing for the duration of this
+            // compile.
+            let artifact_dir_lock = if is_on_nfs_mount(root.as_path_unlocked()) {
+                None
+            } else {
+                Some(dest.open_rw_exclusive_create(
+                    ".cargo-lock",
+                    ws.gctx(),
+                    "artifact directory",
+                )?)
+            };
+            let root = root.into_path_unlocked();
+            let dest = dest.into_path_unlocked();
+            Some(ArtifactDirLayout {
                 dest: dest.clone(),
                 examples: dest.join("examples"),
                 doc: root.join("doc"),
                 timings: root.join("cargo-timings"),
                 _lock: artifact_dir_lock,
-            }),
+            })
+        } else {
+            None
+        };
+        Ok(Layout {
+            artifact_dir,
             build_dir: BuildDirLayout {
                 root: build_root.clone(),
                 deps,

src/cargo/ops/cargo_clean.rs

@@ -117,15 +117,17 @@ fn clean_specs(
     let target_data = RustcTargetData::new(ws, &requested_kinds)?;
     let (pkg_set, resolve) = ops::resolve_ws(ws, dry_run)?;
     let prof_dir_name = profiles.get_dir_name();
-    let host_layout = Layout::new(ws, None, &prof_dir_name)?;
+    let host_layout = Layout::new(ws, None, &prof_dir_name, true)?;
     // Convert requested kinds to a Vec of layouts.
     let target_layouts: Vec<(CompileKind, Layout)> = requested_kinds
         .into_iter()
         .filter_map(|kind| match kind {
-            CompileKind::Target(target) => match Layout::new(ws, Some(target), &prof_dir_name) {
-                Ok(layout) => Some(Ok((kind, layout))),
-                Err(e) => Some(Err(e)),
-            },
+            CompileKind::Target(target) => {
+                match Layout::new(ws, Some(target), &prof_dir_name, true) {
+                    Ok(layout) => Some(Ok((kind, layout))),
+                    Err(e) => Some(Err(e)),
+                }
+            }
             CompileKind::Host => None,
         })
         .collect::<CargoResult<_>>()?;

tests/testsuite/build_dir.rs

@@ -1007,7 +1007,6 @@ fn template_workspace_path_hash_should_handle_symlink() {
 
     p.root().join("target").assert_build_dir_layout(str![[r#"
 [ROOT]/foo/target/CACHEDIR.TAG
-[ROOT]/foo/target/debug/.cargo-lock
 
 "#]]);
 
@@ -1046,7 +1045,6 @@ fn template_workspace_path_hash_should_handle_symlink() {
 
     p.root().join("target").assert_build_dir_layout(str![[r#"
 [ROOT]/foo/target/CACHEDIR.TAG
-[ROOT]/foo/target/debug/.cargo-lock
 
 "#]]);
 

tests/testsuite/build_dir_legacy.rs

@@ -945,7 +945,6 @@ fn template_workspace_path_hash_should_handle_symlink() {
 
     p.root().join("target").assert_build_dir_layout(str![[r#"
 [ROOT]/foo/target/CACHEDIR.TAG
-[ROOT]/foo/target/debug/.cargo-lock
 
 "#]]);
 
@@ -980,7 +979,6 @@ fn template_workspace_path_hash_should_handle_symlink() {
 
     p.root().join("target").assert_build_dir_layout(str![[r#"
 [ROOT]/foo/target/CACHEDIR.TAG
-[ROOT]/foo/target/debug/.cargo-lock
 
 "#]]);
 

tests/testsuite/check.rs

@@ -1705,17 +1705,107 @@ fn check_build_should_not_output_files_to_artifact_dir() {
         .join("target-dir")
         .assert_build_dir_layout(str![[r#"
 [ROOT]/foo/target-dir/CACHEDIR.TAG
-[ROOT]/foo/target-dir/debug/.cargo-lock
 
 "#]]);
 }
 
 #[cargo_test]
-fn check_build_should_lock_artifact_dir() {
+fn check_build_should_not_lock_artifact_dir() {
     let p = project()
         .file("src/main.rs", r#"fn main() { println!("Hello, World!") }"#)
         .build();
 
     p.cargo("check").enable_mac_dsym().run();
-    assert!(p.root().join("target/debug/.cargo-lock").exists());
+    assert!(!p.root().join("target/debug/.cargo-lock").exists());
+}
+
+// Regression test for #16305
+#[cargo_test]
+fn check_build_should_not_uplift_proc_macro_dylib_deps() {
+    let p = project()
+        .file(
+            ".cargo/config.toml",
+            r#"
+            [build]
+            target-dir = "target-dir"
+            build-dir = "build-dir"
+            "#,
+        )
+        .file(
+            "Cargo.toml",
+            r#"
+                [workspace]
+                members = ["foo", "bar", "baz"]
+            "#,
+        )
+        // Bin
+        .file(
+            "foo/Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.0.1"
+                edition = "2015"
+
+                [dependencies]
+                bar = { path = "../bar" }
+            "#,
+        )
+        .file("foo/src/main.rs", "fn main() {}")
+        // Proc macro
+        .file(
+            "bar/Cargo.toml",
+            r#"
+                [package]
+                name = "bar"
+                version = "0.0.1"
+                edition = "2015"
+
+                [lib]
+                proc-macro = true
+
+                [dependencies]
+                baz = { path = "../baz" }
+            "#,
+        )
+        .file(
+            "bar/src/lib.rs",
+            r#"
+            extern crate proc_macro;
+
+            use proc_macro::TokenStream;
+
+            #[proc_macro_derive(B)]
+            pub fn derive(input: TokenStream) -> TokenStream {
+                input
+            }
+            "#,
+        )
+        // Dylib
+        .file(
+            "baz/Cargo.toml",
+            r#"
+                [package]
+                name = "baz"
+                version = "0.1.0"
+                edition = "2015"
+                authors = []
+
+                [lib]
+                crate-type = ["dylib"]
+
+                [dependencies]
+            "#,
+        )
+        .file("baz/src/lib.rs", "pub fn baz() { }")
+        .build();
+
+    p.cargo("check").enable_mac_dsym().run();
+
+    p.root()
+        .join("target-dir")
+        .assert_build_dir_layout(str![[r#"
+[ROOT]/foo/target-dir/CACHEDIR.TAG
+
+"#]]);
 }