Moved manifest metadata tracking from fingerprint to dep info

This change moves the manifest metadata track to dep-info files with the goal of reduce unneeded rebuilds when metadata is changed as well fixing issues where builds are not retrigged due to metadata changes when they should (ie. #14154)
rust-lang · Dec 24, 2024 · 3d7b154 · 3d7b154
1 parent 0c157e0
commit 3d7b154
Show file tree

Hide file tree

Showing 7 changed files with 207 additions and 142 deletions.
diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs
@@ -351,54 +351,27 @@ impl<'gctx> Compilation<'gctx> {
             }
         }
 
-        let metadata = pkg.manifest().metadata();
-
         let cargo_exe = self.gctx.cargo_exe()?;
         cmd.env(crate::CARGO_ENV, cargo_exe);
 
         // When adding new environment variables depending on
         // crate properties which might require rebuild upon change
         // consider adding the corresponding properties to the hash
         // in BuildContext::target_metadata()
-        let rust_version = pkg.rust_version().as_ref().map(ToString::to_string);
         cmd.env("CARGO_MANIFEST_DIR", pkg.root())
             .env("CARGO_MANIFEST_PATH", pkg.manifest_path())
             .env("CARGO_PKG_VERSION_MAJOR", &pkg.version().major.to_string())
             .env("CARGO_PKG_VERSION_MINOR", &pkg.version().minor.to_string())
             .env("CARGO_PKG_VERSION_PATCH", &pkg.version().patch.to_string())
             .env("CARGO_PKG_VERSION_PRE", pkg.version().pre.as_str())
             .env("CARGO_PKG_VERSION", &pkg.version().to_string())
-            .env("CARGO_PKG_NAME", &*pkg.name())
-            .env(
-                "CARGO_PKG_DESCRIPTION",
-                metadata.description.as_ref().unwrap_or(&String::new()),
-            )
-            .env(
-                "CARGO_PKG_HOMEPAGE",
-                metadata.homepage.as_ref().unwrap_or(&String::new()),
-            )
-            .env(
-                "CARGO_PKG_REPOSITORY",
-                metadata.repository.as_ref().unwrap_or(&String::new()),
-            )
-            .env(
-                "CARGO_PKG_LICENSE",
-                metadata.license.as_ref().unwrap_or(&String::new()),
-            )
-            .env(
-                "CARGO_PKG_LICENSE_FILE",
-                metadata.license_file.as_ref().unwrap_or(&String::new()),
-            )
-            .env("CARGO_PKG_AUTHORS", &pkg.authors().join(":"))
-            .env(
-                "CARGO_PKG_RUST_VERSION",
-                &rust_version.as_deref().unwrap_or_default(),
-            )
-            .env(
-                "CARGO_PKG_README",
-                metadata.readme.as_ref().unwrap_or(&String::new()),
-            )
-            .cwd(pkg.root());
+            .env("CARGO_PKG_NAME", &*pkg.name());
+
+        for (key, value) in pkg.manifest().metadata().env_vars() {
+            cmd.env(key, value.as_ref());
+        }
+
+        cmd.cwd(pkg.root());
 
         apply_env_config(self.gctx, &mut cmd)?;
 

diff --git a/src/cargo/core/compiler/fingerprint/dep_info.rs b/src/cargo/core/compiler/fingerprint/dep_info.rs
@@ -19,6 +19,7 @@ use cargo_util::paths;
 use cargo_util::ProcessBuilder;
 use cargo_util::Sha256;
 
+use crate::core::manifest::ManifestMetadata;
 use crate::CargoResult;
 use crate::CARGO_ENV;
 
@@ -334,7 +335,10 @@ pub fn translate_dep_info(
     //
     // For cargo#13280, We trace env vars that are defined in the `[env]` config table.
     on_disk_info.env.retain(|(key, _)| {
-        env_config.contains_key(key) || !rustc_cmd.get_envs().contains_key(key) || key == CARGO_ENV
+        ManifestMetadata::should_track(key)
+            || env_config.contains_key(key)
+            || !rustc_cmd.get_envs().contains_key(key)
+            || key == CARGO_ENV
     });
 
     let serialize_path = |file| {

diff --git a/src/cargo/core/compiler/fingerprint/dirty_reason.rs b/src/cargo/core/compiler/fingerprint/dirty_reason.rs
@@ -26,7 +26,6 @@ pub enum DirtyReason {
         old: Vec<String>,
         new: Vec<String>,
     },
-    MetadataChanged,
     ConfigSettingsChanged,
     CompileKindChanged,
     LocalLengthsChanged,
@@ -168,7 +167,6 @@ impl DirtyReason {
                 s.dirty_because(unit, "the profile configuration changed")
             }
             DirtyReason::RustflagsChanged { .. } => s.dirty_because(unit, "the rustflags changed"),
-            DirtyReason::MetadataChanged => s.dirty_because(unit, "the metadata changed"),
             DirtyReason::ConfigSettingsChanged => {
                 s.dirty_because(unit, "the config settings changed")
             }

diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs
@@ -78,7 +78,6 @@
 //! [`CompileKind`] (host/target)              | ✓           | ✓                   | ✓                      | ✓
 //! `__CARGO_DEFAULT_LIB_METADATA`[^4]         |             | ✓                   | ✓                      | ✓
 //! `package_id`                               |             | ✓                   | ✓                      | ✓
-//! authors, description, homepage, repo       | ✓           |                     |                        |
 //! Target src path relative to ws             | ✓           |                     |                        |
 //! Target flags (test/bench/for_host/edition) | ✓           |                     |                        |
 //! -C incremental=… flag                      | ✓           |                     |                        |
@@ -189,6 +188,8 @@
 //! files to learn about environment variables that the rustc compile depends on.
 //! Cargo then later uses this to trigger a recompile if a referenced env var
 //! changes (even if the source didn't change).
+//! This also includes env vars generated from Cargo metadata like `CARGO_PKG_DESCRIPTION`.
+//! (See [`crate::core::manifest::ManifestMetadata`]
 //!
 //! #### dep-info files for build system integration.
 //!
@@ -612,10 +613,6 @@ pub struct Fingerprint {
     memoized_hash: Mutex<Option<u64>>,
     /// RUSTFLAGS/RUSTDOCFLAGS environment variable value (or config value).
     rustflags: Vec<String>,
-    /// Hash of some metadata from the manifest, such as "authors", or
-    /// "description", which are exposed as environment variables during
-    /// compilation.
-    metadata: u64,
     /// Hash of various config settings that change how things are compiled.
     config: u64,
     /// The rustc target. This is only relevant for `.json` files, otherwise
@@ -831,11 +828,12 @@ impl LocalFingerprint {
         &self,
         mtime_cache: &mut HashMap<PathBuf, FileTime>,
         checksum_cache: &mut HashMap<PathBuf, Checksum>,
-        pkg_root: &Path,
+        pkg: &Package,
         target_root: &Path,
         cargo_exe: &Path,
         gctx: &GlobalContext,
     ) -> CargoResult<Option<StaleItem>> {
+        let pkg_root = pkg.root();
         match self {
             // We need to parse `dep_info`, learn about the crate's dependencies.
             //
@@ -849,6 +847,12 @@ impl LocalFingerprint {
                     return Ok(Some(StaleItem::MissingFile(dep_info)));
                 };
                 for (key, previous) in info.env.iter() {
+                    if let Some(value) = pkg.manifest().metadata().env_var(key.as_str()) {
+                        if Some(value.as_ref()) == previous.as_deref() {
+                            continue;
+                        }
+                    }
+
                     let current = if key == CARGO_ENV {
                         Some(cargo_exe.to_str().ok_or_else(|| {
                             format_err!(
@@ -932,7 +936,6 @@ impl Fingerprint {
             local: Mutex::new(Vec::new()),
             memoized_hash: Mutex::new(None),
             rustflags: Vec::new(),
-            metadata: 0,
             config: 0,
             compile_kind: 0,
             fs_status: FsStatus::Stale,
@@ -995,9 +998,6 @@ impl Fingerprint {
                 new: self.rustflags.clone(),
             };
         }
-        if self.metadata != old.metadata {
-            return DirtyReason::MetadataChanged;
-        }
         if self.config != old.config {
             return DirtyReason::ConfigSettingsChanged;
         }
@@ -1142,13 +1142,14 @@ impl Fingerprint {
         &mut self,
         mtime_cache: &mut HashMap<PathBuf, FileTime>,
         checksum_cache: &mut HashMap<PathBuf, Checksum>,
-        pkg_root: &Path,
+        pkg: &Package,
         target_root: &Path,
         cargo_exe: &Path,
         gctx: &GlobalContext,
     ) -> CargoResult<()> {
         assert!(!self.fs_status.up_to_date());
 
+        let pkg_root = pkg.root();
         let mut mtimes = HashMap::new();
 
         // Get the `mtime` of all outputs. Optionally update their mtime
@@ -1249,7 +1250,7 @@ impl Fingerprint {
             if let Some(item) = local.find_stale_item(
                 mtime_cache,
                 checksum_cache,
-                pkg_root,
+                pkg,
                 target_root,
                 cargo_exe,
                 gctx,
@@ -1279,7 +1280,6 @@ impl hash::Hash for Fingerprint {
             profile,
             ref deps,
             ref local,
-            metadata,
             config,
             compile_kind,
             ref rustflags,
@@ -1294,7 +1294,6 @@ impl hash::Hash for Fingerprint {
             path,
             profile,
             &*local,
-            metadata,
             config,
             compile_kind,
             rustflags,
@@ -1445,7 +1444,7 @@ fn calculate(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResult
     fingerprint.check_filesystem(
         &mut build_runner.mtime_cache,
         &mut build_runner.checksum_cache,
-        unit.pkg.root(),
+        &unit.pkg,
         &target_root,
         cargo_exe,
         build_runner.bcx.gctx,
@@ -1529,9 +1528,6 @@ fn calculate_normal(
         build_runner.lto[unit],
         unit.pkg.manifest().lint_rustflags(),
     ));
-    // 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 mut config = StableHasher::new();
     if let Some(linker) = build_runner.compilation.target_linker(unit.kind) {
         linker.hash(&mut config);
@@ -1560,7 +1556,6 @@ fn calculate_normal(
         deps,
         local: Mutex::new(local),
         memoized_hash: Mutex::new(None),
-        metadata,
         config: Hasher::finish(&config),
         compile_kind,
         rustflags: extra_flags,

diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs
@@ -1,3 +1,4 @@
+use std::borrow::Cow;
 use std::collections::{BTreeMap, HashMap};
 use std::fmt;
 use std::hash::{Hash, Hasher};
@@ -146,6 +147,86 @@ pub struct ManifestMetadata {
     pub rust_version: Option<RustVersion>,
 }
 
+macro_rules! get_metadata_env {
+    ($meta:ident, $field:ident) => {
+        $meta.$field.as_deref().unwrap_or_default().into()
+    };
+    ($meta:ident, $field:ident, $to_var:expr) => {
+        $to_var($meta).into()
+    };
+}
+
+macro_rules! metadata_envs {
+    (
+        $(
+            ($field:ident, $key:literal$(, $to_var:expr)?),
+        )*
+    ) => {
+        struct MetadataEnvs;
+        impl MetadataEnvs {
+            $(
+                fn $field(meta: &ManifestMetadata) -> Cow<'_, str> {
+                    get_metadata_env!(meta, $field$(, $to_var)?)
+                }
+            )*
+
+            pub fn should_track(key: &str) -> bool {
+                let keys = [$($key),*];
+                key.strip_prefix("CARGO_PKG_")
+                    .map(|key| keys.iter().any(|k| *k == key))
+                    .unwrap_or_default()
+            }
+
+            pub fn var<'a>(meta: &'a ManifestMetadata, key: &str) -> Option<Cow<'a, str>> {
+                key.strip_prefix("CARGO_PKG_").and_then(|key| match key {
+                    $($key => Some(Self::$field(meta)),)*
+                    _ => None,
+                })
+            }
+
+            pub fn vars(meta: &ManifestMetadata) -> impl Iterator<Item = (&'static str, Cow<'_, str>)> {
+                [
+                    $(
+                        (
+                            concat!("CARGO_PKG_", $key),
+                            Self::$field(meta),
+                        ),
+                    )*
+                ].into_iter()
+            }
+        }
+    }
+}
+
+// Metadata enviromental variables that are emitted to rustc. Usable by `env!()`
+// If these change we need to trigger a rebuild.
+// NOTE: The env var name will be prefixed with `CARGO_PKG_`
+metadata_envs! {
+    (description, "DESCRIPTION"),
+    (homepage, "HOMEPAGE"),
+    (repository, "REPOSITORY"),
+    (license, "LICENSE"),
+    (license_file, "LICENSE_FILE"),
+    (authors, "AUTHORS", |m: &ManifestMetadata| m.authors.join(":")),
+    (rust_version, "RUST_VERSION", |m: &ManifestMetadata| m.rust_version.as_ref().map(ToString::to_string).unwrap_or_default()),
+    (readme, "README"),
+}
+
+impl ManifestMetadata {
+    /// Whether the given env var should be tracked by Cargo's dep-info.
+    pub fn should_track(env_key: &str) -> bool {
+        MetadataEnvs::should_track(env_key)
+    }
+
+    pub fn env_var<'a>(&'a self, env_key: &str) -> Option<Cow<'a, str>> {
+        MetadataEnvs::var(self, env_key)
+    }
+
+    pub fn env_vars(&self) -> impl Iterator<Item = (&'static str, Cow<'_, str>)> {
+        MetadataEnvs::vars(self)
+    }
+}
+
 #[derive(Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
 pub enum TargetKind {
     Lib(Vec<CrateType>),