Revert always computing filename Metadata.

When a unit does not have Metadata, the computation of fingerprints
depends on reusing the same fingerprint file to detect if the
output needs to be rebuilt. The previous change that put each unit's
fingerprint into a separate directory was wrong, and broke change
detection in this case (for example, executables on MSVC).

Instead, use a different approach to deal with compiler output caching
by using the same naming convention as the fingerprint file itself.

Author: ehuss

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

@@ -85,7 +85,7 @@ pub struct CompilationFiles<'a, 'cfg> {
     roots: Vec<Unit>,
     ws: &'a Workspace<'cfg>,
     /// Metadata hash to use for each unit.
-    metas: HashMap<Unit, Metadata>,
+    metas: HashMap<Unit, Option<Metadata>>,
     /// For each Unit, a list all files produced.
     outputs: HashMap<Unit, LazyCell<Arc<Vec<OutputFile>>>>,
 }
@@ -154,12 +154,8 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
     ///
     /// Returns `None` if the unit should not use a metadata hash (like
     /// rustdoc, or some dylibs).
-    pub fn metadata(&self, bcx: &BuildContext<'_, '_>, unit: &Unit) -> Option<Metadata> {
-        if should_use_metadata(bcx, unit) {
-            Some(self.metas[unit])
-        } else {
-            None
-        }
+    pub fn metadata(&self, unit: &Unit) -> Option<Metadata> {
+        self.metas[unit]
     }
 
     /// Gets the short hash based only on the `PackageId`.
@@ -192,10 +188,14 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
 
     /// Directory name to use for a package in the form `NAME-HASH`.
     ///
-    /// The hash is unique per Unit.
-    pub fn pkg_dir(&self, unit: &Unit) -> String {
+    /// Note that some units may share the same directory, so care should be
+    /// taken in those cases!
+    fn pkg_dir(&self, unit: &Unit) -> String {
         let name = unit.pkg.package_id().name();
-        format!("{}-{}", name, self.metas[unit])
+        match self.metas[unit] {
+            Some(ref meta) => format!("{}-{}", name, meta),
+            None => format!("{}-{}", name, self.target_short_hash(unit)),
+        }
     }
 
     /// Returns the root of the build output tree for the host
@@ -220,9 +220,29 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
         self.layout(unit.kind).fingerprint().join(dir)
     }
 
+    /// Returns the path for a file in the fingerprint directory.
+    ///
+    /// The "prefix" should be something to distinguish the file from other
+    /// files in the fingerprint directory.
+    pub fn fingerprint_file_path(&self, unit: &Unit, prefix: &str) -> PathBuf {
+        // Different targets need to be distinguished in the
+        let kind = unit.target.kind().description();
+        let flavor = if unit.mode.is_any_test() {
+            "test-"
+        } else if unit.mode.is_doc() {
+            "doc-"
+        } else if unit.mode.is_run_custom_build() {
+            "run-"
+        } else {
+            ""
+        };
+        let name = format!("{}{}{}-{}", prefix, flavor, kind, unit.target.name());
+        self.fingerprint_dir(unit).join(name)
+    }
+
     /// Path where compiler output is cached.
     pub fn message_cache_path(&self, unit: &Unit) -> PathBuf {
-        self.fingerprint_dir(unit).join("output")
+        self.fingerprint_file_path(unit, "output-")
     }
 
     /// Returns the directory where a compiled build script is stored.
@@ -414,7 +434,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
         // Convert FileType to OutputFile.
         let mut outputs = Vec::new();
         for file_type in file_types {
-            let meta = self.metadata(bcx, unit).map(|m| m.to_string());
+            let meta = self.metadata(unit).map(|m| m.to_string());
             let path = out_dir.join(file_type.output_filename(&unit.target, meta.as_deref()));
             let hardlink = self.uplift_to(unit, &file_type, &path);
             let export_path = if unit.target.is_custom_build() {
@@ -437,7 +457,11 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
     }
 }
 
-fn metadata_of(unit: &Unit, cx: &Context<'_, '_>, metas: &mut HashMap<Unit, Metadata>) -> Metadata {
+fn metadata_of(
+    unit: &Unit,
+    cx: &Context<'_, '_>,
+    metas: &mut HashMap<Unit, Option<Metadata>>,
+) -> Option<Metadata> {
     if !metas.contains_key(unit) {
         let meta = compute_metadata(unit, cx, metas);
         metas.insert(unit.clone(), meta);
@@ -451,9 +475,12 @@ fn metadata_of(unit: &Unit, cx: &Context<'_, '_>, metas: &mut HashMap<Unit, Meta
 fn compute_metadata(
     unit: &Unit,
     cx: &Context<'_, '_>,
-    metas: &mut HashMap<Unit, Metadata>,
-) -> Metadata {
+    metas: &mut HashMap<Unit, Option<Metadata>>,
+) -> Option<Metadata> {
     let bcx = &cx.bcx;
+    if !should_use_metadata(bcx, unit) {
+        return None;
+    }
     let mut hasher = SipHasher::new();
 
     // This is a generic version number that can be changed to make
@@ -526,7 +553,7 @@ fn compute_metadata(
     // with user dependencies.
     unit.is_std.hash(&mut hasher);
 
-    Metadata(hasher.finish())
+    Some(Metadata(hasher.finish()))
 }
 
 fn hash_rustc_version(bcx: &BuildContext<'_, '_>, hasher: &mut SipHasher) {

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

@@ -377,7 +377,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
     pub fn get_run_build_script_metadata(&self, unit: &Unit) -> Metadata {
         assert!(unit.mode.is_run_custom_build());
         self.files()
-            .metadata(self.bcx, unit)
+            .metadata(unit)
             .expect("build script should always have hash")
     }
 

src/cargo/core/compiler/fingerprint.rs

@@ -43,8 +43,9 @@
 //! The `Metadata` hash is a hash added to the output filenames to isolate
 //! each unit. See the documentation in the `compilation_files` module for
 //! more details. NOTE: Not all output files are isolated via filename hashes
-//! (like dylibs), but the fingerprint directory always has the `Metadata`
-//! hash in its directory name.
+//! (like dylibs). The fingerprint directory uses a hash, but sometimes units
+//! share the same fingerprint directory (when they don't have Metadata) so
+//! care should be taken to handle this!
 //!
 //! Fingerprints and Metadata are similar, and track some of the same things.
 //! The Metadata contains information that is required to keep Units separate.
@@ -352,8 +353,7 @@ pub fn prepare_target(cx: &mut Context<'_, '_>, unit: &Unit, force: bool) -> Car
         unit.target.name()
     ));
     let bcx = cx.bcx;
-    let new = cx.files().fingerprint_dir(unit);
-    let loc = new.join(&filename(unit));
+    let loc = cx.files().fingerprint_file_path(unit, "");
 
     debug!("fingerprint at: {}", loc.display());
 
@@ -1522,9 +1522,7 @@ pub fn prepare_init(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<()> {
 /// Returns the location that the dep-info file will show up at for the `unit`
 /// specified.
 pub fn dep_info_loc(cx: &mut Context<'_, '_>, unit: &Unit) -> PathBuf {
-    cx.files()
-        .fingerprint_dir(unit)
-        .join(&format!("dep-{}", filename(unit)))
+    cx.files().fingerprint_file_path(unit, "dep-")
 }
 
 /// Returns an absolute path that target directory.
@@ -1680,21 +1678,6 @@ where
     None
 }
 
-/// Returns the filename used for the fingerprint file.
-fn filename(unit: &Unit) -> String {
-    let kind = unit.target.kind().description();
-    let flavor = if unit.mode.is_any_test() {
-        "test-"
-    } else if unit.mode.is_doc() {
-        "doc-"
-    } else if unit.mode.is_run_custom_build() {
-        "run-"
-    } else {
-        ""
-    };
-    format!("{}{}-{}", flavor, kind, unit.target.name())
-}
-
 #[repr(u8)]
 enum DepInfoPathType {
     // src/, e.g. src/lib.rs

src/cargo/core/compiler/layout.rs

@@ -39,7 +39,7 @@
 //!                 lib-$targetname.json
 //!                 # The console output from the compiler. This is cached
 //!                 # so that warnings can be redisplayed for "fresh" units.
-//!                 output
+//!                 output-lib-$targetname
 //!
 //!         # This is the root directory for all rustc artifacts except build
 //!         # scripts, examples, and test and bench executables. Almost every

src/cargo/core/compiler/mod.rs

@@ -194,7 +194,7 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc<dyn Executor>) -> Car
     let pass_l_flag = unit.target.is_lib() || !unit.pkg.targets().iter().any(|t| t.is_lib());
     let pass_cdylib_link_args = unit.target.is_cdylib();
 
-    let dep_info_name = match cx.files().metadata(cx.bcx, unit) {
+    let dep_info_name = match cx.files().metadata(unit) {
         Some(metadata) => format!("{}-{}.d", unit.target.crate_name(), metadata),
         None => format!("{}.d", unit.target.crate_name()),
     };
@@ -870,7 +870,7 @@ fn build_base_args(
         cmd.arg("--cfg").arg(&format!("feature=\"{}\"", feat));
     }
 
-    match cx.files().metadata(cx.bcx, unit) {
+    match cx.files().metadata(unit) {
         Some(m) => {
             cmd.arg("-C").arg(&format!("metadata={}", m));
             cmd.arg("-C").arg(&format!("extra-filename=-{}", m));

tests/testsuite/cache_messages.rs

@@ -194,7 +194,7 @@ fn clears_cache_after_fix() {
     // Fill the cache.
     p.cargo("check").with_stderr_contains("[..]asdf[..]").run();
     let cpath = p
-        .glob("target/debug/.fingerprint/foo-*/output")
+        .glob("target/debug/.fingerprint/foo-*/output-*")
         .next()
         .unwrap()
         .unwrap();
@@ -215,7 +215,10 @@ fn clears_cache_after_fix() {
 ",
         )
         .run();
-    assert_eq!(p.glob("target/debug/.fingerprint/foo-*/output").count(), 0);
+    assert_eq!(
+        p.glob("target/debug/.fingerprint/foo-*/output-*").count(),
+        0
+    );
 
     // And again, check the cache is correct.
     p.cargo("check")
@@ -253,7 +256,10 @@ fn rustdoc() {
     let rustdoc_stderr = as_str(&rustdoc_output.stderr);
     assert!(rustdoc_stderr.contains("private"));
     assert!(rustdoc_stderr.contains("\x1b["));
-    assert_eq!(p.glob("target/debug/.fingerprint/foo-*/output").count(), 1);
+    assert_eq!(
+        p.glob("target/debug/.fingerprint/foo-*/output-*").count(),
+        1
+    );
 
     // Check the cached output.
     let rustdoc_output = p
@@ -331,14 +337,23 @@ fn doesnt_create_extra_files() {
 
     p.cargo("build").run();
 
-    assert_eq!(p.glob("target/debug/.fingerprint/foo-*/output").count(), 0);
-    assert_eq!(p.glob("target/debug/.fingerprint/dep-*/output").count(), 0);
+    assert_eq!(
+        p.glob("target/debug/.fingerprint/foo-*/output-*").count(),
+        0
+    );
+    assert_eq!(
+        p.glob("target/debug/.fingerprint/dep-*/output-*").count(),
+        0
+    );
     if is_coarse_mtime() {
         sleep_ms(1000);
     }
     p.change_file("src/lib.rs", "fn unused() {}");
     p.cargo("build").run();
-    assert_eq!(p.glob("target/debug/.fingerprint/foo-*/output").count(), 1);
+    assert_eq!(
+        p.glob("target/debug/.fingerprint/foo-*/output-*").count(),
+        1
+    );
 }
 
 #[cargo_test]