feat: Do not lock the artifact-dir for check builds

Author: ranger-ross

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,37 @@ 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 are
+                // two notable exceptions:
+                // 1. If check has `--test` or `--tests` we need to pass `CARGO_BIN_EXE_` which
+                //    points into the artifact-dir. In theory we don't need to take the lock here
+                //    but we do to satify the Layout struct.
+                // 2. If check has `--timings` we still need to lock artifact-dir since we will
+                //    output the report files.
+                let is_test = self.bcx.roots.iter().any(|u| u.mode.is_any_test());
+                is_test || !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

@@ -116,15 +116,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

@@ -1005,7 +1005,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
 
 "#]]);
 
@@ -1044,7 +1043,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

@@ -943,7 +943,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
 
 "#]]);
 
@@ -978,7 +977,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,16 @@ 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());
 }