Auto merge of #6785 - ehuss:fingerprint-cleanup, r=dwijnand

Some fingerprint cleanup.

Just a minor cleanup.

Move `CARGO_PKG_*` values from Metadata to Fingerprint (added in #3857).  Closes #6208.  This prevents stale artifacts from being left behind when these values change. Also tracks changes to the "repository" value (added in #6096).

Remove `edition` as a separate field. It is already tracked in `target`. This was required previously to #5816 which added per-target editions.

Also adds a helper to the testsuite to make globbing easier.

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

@@ -469,13 +469,6 @@ fn compute_metadata<'a, 'cfg>(
         .stable_hash(bcx.ws.root())
         .hash(&mut hasher);
 
-    // Add package properties which map to environment variables
-    // exposed by Cargo.
-    let manifest_metadata = unit.pkg.manifest().metadata();
-    manifest_metadata.authors.hash(&mut hasher);
-    manifest_metadata.description.hash(&mut hasher);
-    manifest_metadata.homepage.hash(&mut hasher);
-
     // Also mix in enabled features to our metadata. This'll ensure that
     // when changing feature sets each lib is separately cached.
     bcx.resolve

src/cargo/core/compiler/fingerprint.rs

@@ -51,20 +51,17 @@
 //! Immediate dependency’s hashes              | ✓[^1]       | ✓
 //! Target or Host mode                        |             | ✓
 //! __CARGO_DEFAULT_LIB_METADATA[^4]           |             | ✓
-//! authors, description, homepage             |             | ✓[^2]
 //! package_id                                 |             | ✓
+//! authors, description, homepage, repo       | ✓           |
 //! Target src path                            | ✓           |
 //! Target path relative to ws                 | ✓           |
 //! Target flags (test/bench/for_host/edition) | ✓           |
-//! Edition                                    | ✓           |
 //! -C incremental=… flag                      | ✓           |
 //! mtime of sources                           | ✓[^3]       |
 //! RUSTFLAGS/RUSTDOCFLAGS                     | ✓           |
 //!
 //! [^1]: Build script and bin dependencies are not included.
 //!
-//! [^2]: This is a bug, see https://github.com/rust-lang/cargo/issues/6208
-//!
 //! [^3]: The mtime is only tracked for workspace members and path
 //!       dependencies. Git dependencies track the git revision.
 //!
@@ -175,7 +172,7 @@ use serde::de;
 use serde::ser;
 use serde::{Deserialize, Serialize};
 
-use crate::core::{Edition, Package};
+use crate::core::Package;
 use crate::util;
 use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::paths;
@@ -337,17 +334,34 @@ struct DepFingerprint {
 /// graph.
 #[derive(Serialize, Deserialize)]
 pub struct Fingerprint {
+    /// Hash of the version of `rustc` used.
     rustc: u64,
+    /// Sorted list of cfg features enabled.
     features: String,
+    /// Hash of the `Target` struct, including the target name,
+    /// package-relative source path, edition, etc.
     target: u64,
+    /// Hash of the `Profile`, `CompileMode`, and any extra flags passed via
+    /// `cargo rustc` or `cargo rustdoc`.
     profile: u64,
+    /// Hash of the path to the base source file. This is relative to the
+    /// workspace root for path members, or absolute for other sources.
     path: u64,
+    /// Fingerprints of dependencies.
     deps: Vec<DepFingerprint>,
+    /// Information about the inputs that affect this Unit (such as source
+    /// file mtimes or build script environment variables).
     local: Vec<LocalFingerprint>,
+    /// Cached hash of the `Fingerprint` struct. Used to improve performance
+    /// for hashing.
     #[serde(skip_serializing, skip_deserializing)]
     memoized_hash: Mutex<Option<u64>>,
+    /// RUSTFLAGS/RUSTDOCFLAGS environment variable value (or config value).
     rustflags: Vec<String>,
-    edition: Edition,
+    /// Hash of some metadata from the manifest, such as "authors", or
+    /// "description", which are exposed as environment variables during
+    /// compilation.
+    metadata: u64,
 }
 
 impl Serialize for DepFingerprint {
@@ -407,8 +421,8 @@ impl Fingerprint {
             deps: Vec::new(),
             local: Vec::new(),
             memoized_hash: Mutex::new(None),
-            edition: Edition::Edition2015,
             rustflags: Vec::new(),
+            metadata: 0,
         }
     }
 
@@ -463,8 +477,8 @@ impl Fingerprint {
         if self.local.len() != old.local.len() {
             bail!("local lens changed");
         }
-        if self.edition != old.edition {
-            bail!("edition changed")
+        if self.metadata != old.metadata {
+            bail!("metadata changed")
         }
         for (new, old) in self.local.iter().zip(&old.local) {
             match (new, old) {
@@ -546,12 +560,12 @@ impl hash::Hash for Fingerprint {
             profile,
             ref deps,
             ref local,
-            edition,
+            metadata,
             ref rustflags,
             ..
         } = *self;
         (
-            rustc, features, target, path, profile, local, edition, rustflags,
+            rustc, features, target, path, profile, local, metadata, rustflags,
         )
             .hash(h);
 
@@ -678,6 +692,9 @@ fn calculate<'a, 'cfg>(
         bcx.rustflags_args(unit)?
     };
     let profile_hash = util::hash_u64(&(&unit.profile, unit.mode, bcx.extra_args_for(unit)));
+    // Include metadata since it is exposed as environment variables.
+    let m = unit.pkg.manifest().metadata();
+    let metadata = util::hash_u64(&(&m.authors, &m.description, &m.homepage, &m.repository));
     let fingerprint = Arc::new(Fingerprint {
         rustc: util::hash_u64(&bcx.rustc.verbose_version),
         target: util::hash_u64(&unit.target),
@@ -689,7 +706,7 @@ fn calculate<'a, 'cfg>(
         deps,
         local,
         memoized_hash: Mutex::new(None),
-        edition: unit.target.edition(),
+        metadata,
         rustflags: extra_flags,
     });
     cx.fingerprints.insert(*unit, Arc::clone(&fingerprint));

tests/testsuite/check.rs

@@ -4,7 +4,6 @@ use crate::support::install::exe;
 use crate::support::paths::CargoPathExt;
 use crate::support::registry::Package;
 use crate::support::{basic_manifest, project};
-use glob::glob;
 
 #[test]
 fn check_success() {
@@ -623,43 +622,34 @@ fn check_artifacts() {
         .file("benches/b1.rs", "")
         .build();
 
-    let assert_glob = |path: &str, count: usize| {
-        assert_eq!(
-            glob(&p.root().join(path).to_str().unwrap())
-                .unwrap()
-                .count(),
-            count
-        );
-    };
-
     p.cargo("check").run();
     assert!(!p.root().join("target/debug/libfoo.rmeta").is_file());
     assert!(!p.root().join("target/debug/libfoo.rlib").is_file());
     assert!(!p.root().join("target/debug").join(exe("foo")).is_file());
-    assert_glob("target/debug/deps/libfoo-*.rmeta", 2);
+    assert_eq!(p.glob("target/debug/deps/libfoo-*.rmeta").count(), 2);
 
     p.root().join("target").rm_rf();
     p.cargo("check --lib").run();
     assert!(!p.root().join("target/debug/libfoo.rmeta").is_file());
     assert!(!p.root().join("target/debug/libfoo.rlib").is_file());
     assert!(!p.root().join("target/debug").join(exe("foo")).is_file());
-    assert_glob("target/debug/deps/libfoo-*.rmeta", 1);
+    assert_eq!(p.glob("target/debug/deps/libfoo-*.rmeta").count(), 1);
 
     p.root().join("target").rm_rf();
     p.cargo("check --bin foo").run();
     assert!(!p.root().join("target/debug/libfoo.rmeta").is_file());
     assert!(!p.root().join("target/debug/libfoo.rlib").is_file());
     assert!(!p.root().join("target/debug").join(exe("foo")).is_file());
-    assert_glob("target/debug/deps/libfoo-*.rmeta", 2);
+    assert_eq!(p.glob("target/debug/deps/libfoo-*.rmeta").count(), 2);
 
     p.root().join("target").rm_rf();
     p.cargo("check --test t1").run();
     assert!(!p.root().join("target/debug/libfoo.rmeta").is_file());
     assert!(!p.root().join("target/debug/libfoo.rlib").is_file());
     assert!(!p.root().join("target/debug").join(exe("foo")).is_file());
-    assert_glob("target/debug/t1-*", 0);
-    assert_glob("target/debug/deps/libfoo-*.rmeta", 1);
-    assert_glob("target/debug/deps/libt1-*.rmeta", 1);
+    assert_eq!(p.glob("target/debug/t1-*").count(), 0);
+    assert_eq!(p.glob("target/debug/deps/libfoo-*.rmeta").count(), 1);
+    assert_eq!(p.glob("target/debug/deps/libt1-*.rmeta").count(), 1);
 
     p.root().join("target").rm_rf();
     p.cargo("check --example ex1").run();
@@ -670,17 +660,17 @@ fn check_artifacts() {
         .join("target/debug/examples")
         .join(exe("ex1"))
         .is_file());
-    assert_glob("target/debug/deps/libfoo-*.rmeta", 1);
-    assert_glob("target/debug/examples/libex1-*.rmeta", 1);
+    assert_eq!(p.glob("target/debug/deps/libfoo-*.rmeta").count(), 1);
+    assert_eq!(p.glob("target/debug/examples/libex1-*.rmeta").count(), 1);
 
     p.root().join("target").rm_rf();
     p.cargo("check --bench b1").run();
     assert!(!p.root().join("target/debug/libfoo.rmeta").is_file());
     assert!(!p.root().join("target/debug/libfoo.rlib").is_file());
     assert!(!p.root().join("target/debug").join(exe("foo")).is_file());
-    assert_glob("target/debug/b1-*", 0);
-    assert_glob("target/debug/deps/libfoo-*.rmeta", 1);
-    assert_glob("target/debug/deps/libb1-*.rmeta", 1);
+    assert_eq!(p.glob("target/debug/b1-*").count(), 0);
+    assert_eq!(p.glob("target/debug/deps/libfoo-*.rmeta").count(), 1);
+    assert_eq!(p.glob("target/debug/deps/libb1-*.rmeta").count(), 1);
 }
 
 #[test]

tests/testsuite/doc.rs

@@ -2,8 +2,6 @@ use std::fs::{self, File};
 use std::io::Read;
 use std::str;
 
-use glob::glob;
-
 use crate::support::paths::CargoPathExt;
 use crate::support::registry::Package;
 use crate::support::{basic_lib_manifest, basic_manifest, git, project};
@@ -113,23 +111,8 @@ fn doc_deps() {
     assert!(p.root().join("target/doc/bar/index.html").is_file());
 
     // Verify that it only emits rmeta for the dependency.
-    assert_eq!(
-        glob(&p.root().join("target/debug/**/*.rlib").to_str().unwrap())
-            .unwrap()
-            .count(),
-        0
-    );
-    assert_eq!(
-        glob(
-            &p.root()
-                .join("target/debug/deps/libbar-*.rmeta")
-                .to_str()
-                .unwrap()
-        )
-        .unwrap()
-        .count(),
-        1
-    );
+    assert_eq!(p.glob("target/debug/**/*.rlib").count(), 0);
+    assert_eq!(p.glob("target/debug/deps/libbar-*.rmeta").count(), 1);
 
     p.cargo("doc")
         .env("RUST_LOG", "cargo::ops::cargo_rustc::fingerprint")

tests/testsuite/freshness.rs

@@ -1885,3 +1885,76 @@ fn simulated_docker_deps_stay_cached() {
             .run();
     }
 }
+
+#[test]
+fn metadata_change_invalidates() {
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("build").run();
+
+    for attr in &[
+        "authors = [\"foo\"]",
+        "description = \"desc\"",
+        "homepage = \"https://example.com\"",
+        "repository =\"https://example.com\"",
+    ] {
+        let mut file = OpenOptions::new()
+            .write(true)
+            .append(true)
+            .open(p.root().join("Cargo.toml"))
+            .unwrap();
+        writeln!(file, "{}", attr).unwrap();
+        p.cargo("build")
+            .with_stderr_contains("[COMPILING] foo [..]")
+            .run();
+    }
+    p.cargo("build -v")
+        .with_stderr_contains("[FRESH] foo[..]")
+        .run();
+    assert_eq!(p.glob("target/debug/deps/libfoo-*.rlib").count(), 1);
+}
+
+#[test]
+fn edition_change_invalidates() {
+    const MANIFEST: &str = r#"
+        [package]
+        name = "foo"
+        version = "0.1.0"
+    "#;
+    let p = project()
+        .file("Cargo.toml", MANIFEST)
+        .file("src/lib.rs", "")
+        .build();
+    p.cargo("build").run();
+    p.change_file("Cargo.toml", &format!("{}edition = \"2018\"", MANIFEST));
+    p.cargo("build")
+        .with_stderr_contains("[COMPILING] foo [..]")
+        .run();
+    p.change_file(
+        "Cargo.toml",
+        &format!(
+            r#"{}edition = "2018"
+            [lib]
+            edition = "2015"
+            "#,
+            MANIFEST
+        ),
+    );
+    p.cargo("build")
+        .with_stderr_contains("[COMPILING] foo [..]")
+        .run();
+    p.cargo("build -v")
+        .with_stderr_contains("[FRESH] foo[..]")
+        .run();
+    assert_eq!(p.glob("target/debug/deps/libfoo-*.rlib").count(), 1);
+}

tests/testsuite/metabuild.rs

@@ -2,7 +2,6 @@ use crate::support::{
     basic_lib_manifest, basic_manifest, is_coarse_mtime, project, registry::Package, rustc_host,
     Project,
 };
-use glob::glob;
 use serde_json;
 use std::str;
 
@@ -537,17 +536,7 @@ fn metabuild_build_plan() {
         )
         .run();
 
-    assert_eq!(
-        glob(
-            &p.root()
-                .join("target/.metabuild/metabuild-foo-*.rs")
-                .to_str()
-                .unwrap()
-        )
-        .unwrap()
-        .count(),
-        1
-    );
+    assert_eq!(p.glob("target/.metabuild/metabuild-foo-*.rs").count(), 1);
 }
 
 #[test]
@@ -623,14 +612,7 @@ fn metabuild_two_versions() {
         .run();
 
     assert_eq!(
-        glob(
-            &p.root()
-                .join("target/.metabuild/metabuild-member?-*.rs")
-                .to_str()
-                .unwrap()
-        )
-        .unwrap()
-        .count(),
+        p.glob("target/.metabuild/metabuild-member?-*.rs").count(),
         2
     );
 }
@@ -681,17 +663,7 @@ fn metabuild_external_dependency() {
         .with_stdout_contains("[dep 1.0.0] Hello mb")
         .run();
 
-    assert_eq!(
-        glob(
-            &p.root()
-                .join("target/.metabuild/metabuild-dep-*.rs")
-                .to_str()
-                .unwrap()
-        )
-        .unwrap()
-        .count(),
-        1
-    );
+    assert_eq!(p.glob("target/.metabuild/metabuild-dep-*.rs").count(), 1);
 }
 
 #[test]