From 632330440e10094d37c97a2e5e22e0d690380c8c Mon Sep 17 00:00:00 2001 From: ranger-ross Date: Sat, 21 Dec 2024 17:30:53 +0900 Subject: [PATCH] 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) --- src/cargo/core/compiler/compilation.rs | 41 +----- .../core/compiler/fingerprint/dep_info.rs | 6 +- .../core/compiler/fingerprint/dirty_reason.rs | 2 - src/cargo/core/compiler/fingerprint/mod.rs | 33 ++--- src/cargo/core/manifest.rs | 81 +++++++++++ tests/testsuite/freshness.rs | 133 +++++++++++++----- tests/testsuite/freshness_checksum.rs | 53 +------ 7 files changed, 207 insertions(+), 142 deletions(-) diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 7531f0fa61b..c30343ef16c 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -351,8 +351,6 @@ impl<'gctx> Compilation<'gctx> { } } - let metadata = pkg.manifest().metadata(); - let cargo_exe = self.gctx.cargo_exe()?; cmd.env(crate::CARGO_ENV, cargo_exe); @@ -360,7 +358,6 @@ impl<'gctx> Compilation<'gctx> { // 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()) @@ -368,37 +365,13 @@ impl<'gctx> Compilation<'gctx> { .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 index e8628f34ebf..746c0832035 100644 --- 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 index 2da7d1e6d88..6881f72a837 100644 --- 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, new: Vec, }, - 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 index ac29b0d15d9..623e4c59b60 100644 --- 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>, /// RUSTFLAGS/RUSTDOCFLAGS environment variable value (or config value). rustflags: Vec, - /// 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, checksum_cache: &mut HashMap, - pkg_root: &Path, + pkg: &Package, target_root: &Path, cargo_exe: &Path, gctx: &GlobalContext, ) -> CargoResult> { + 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, checksum_cache: &mut HashMap, - 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 index d63dbd61de3..88845ff26e0 100644 --- 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, } +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> { + key.strip_prefix("CARGO_PKG_").and_then(|key| match key { + $($key => Some(Self::$field(meta)),)* + _ => None, + }) + } + + pub fn vars(meta: &ManifestMetadata) -> impl Iterator)> { + [ + $( + ( + 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> { + MetadataEnvs::var(self, env_key) + } + + pub fn env_vars(&self) -> impl Iterator)> { + MetadataEnvs::vars(self) + } +} + #[derive(Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] pub enum TargetKind { Lib(Vec), diff --git a/tests/testsuite/freshness.rs b/tests/testsuite/freshness.rs index d2a6135e8a5..5b3042f721f 100644 --- a/tests/testsuite/freshness.rs +++ b/tests/testsuite/freshness.rs @@ -953,7 +953,7 @@ new desc "#]]) .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the metadata changed +[DIRTY] foo v0.0.1 ([ROOT]/foo): the environment variable CARGO_PKG_DESCRIPTION changed [COMPILING] foo v0.0.1 ([ROOT]/foo) [RUNNING] `rustc [..] [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -2113,49 +2113,114 @@ fn simulated_docker_deps_stay_cached() { #[cargo_test] fn metadata_change_invalidates() { - let p = project() - .file( - "Cargo.toml", + // (key, value, value-updated, env-var-name) + let scenarios = [ + ( + "description", + r#""foo""#, + r#""foo_updated""#, + "CARGO_PKG_DESCRIPTION", + ), + ( + "homepage", + r#""foo""#, + r#""foo_updated""#, + "CARGO_PKG_HOMEPAGE", + ), + ( + "repository", + r#""foo""#, + r#""foo_updated""#, + "CARGO_PKG_REPOSITORY", + ), + ( + "license", + r#""foo""#, + r#""foo_updated""#, + "CARGO_PKG_LICENSE", + ), + ( + "license-file", + r#""foo""#, + r#""foo_updated""#, + "CARGO_PKG_LICENSE_FILE", + ), + ( + "authors", + r#"["foo"]"#, + r#"["foo_updated"]"#, + "CARGO_PKG_AUTHORS", + ), + ( + "rust-version", + r#""1.0.0""#, + r#""1.0.1""#, + "CARGO_PKG_RUST_VERSION", + ), + ("readme", r#""foo""#, r#""foo_updated""#, "CARGO_PKG_README"), + ]; + let base_cargo_toml = r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2015" + "#; + + let p = project().build(); + for (key, value, value_updated, env_var) in scenarios { + p.change_file("Cargo.toml", base_cargo_toml); + p.change_file( + "src/main.rs", + &format!( + r#" + fn main() {{ + let output = env!("{env_var}"); + println!("{{output}}"); + }} + "# + ), + ); + + // Compile the first time + p.cargo("build").run(); + + // Update the manifest, rebuild, and verify the build was invalided + p.change_file("Cargo.toml", &format!("{base_cargo_toml}\n{key} = {value}")); + p.cargo("build -v") + .with_stderr_data(format!( + r#"[DIRTY] foo v0.1.0 ([ROOT]/foo): the environment variable {env_var} changed +[COMPILING] foo v0.1.0 ([ROOT]/foo) +[RUNNING] `rustc [..] +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s +"# + )) + .run(); + + // Remove references to the metadata and rebuild + p.change_file( + "src/main.rs", r#" - [package] - name = "foo" - version = "0.1.0" - edition = "2015" + fn main() { + println!("foo"); + } "#, - ) - .file("src/lib.rs", "") - .build(); + ); + p.cargo("build").run(); - p.cargo("build").run(); + // Update the manifest value and verify the build is NOT invalidated. + p.change_file( + "Cargo.toml", + &format!("{base_cargo_toml}\n{key} = {value_updated}"), + ); - 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") + p.cargo("build -v") .with_stderr_data(str![[r#" -[COMPILING] foo v0.1.0 ([ROOT]/foo) +[FRESH] foo v0.1.0 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) .run(); } - p.cargo("build -v") - .with_stderr_data(str![[r#" -[FRESH] foo v0.1.0 ([ROOT]/foo) -[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s - -"#]]) - .run(); - assert_eq!(p.glob("target/debug/deps/libfoo-*.rlib").count(), 1); } #[cargo_test] diff --git a/tests/testsuite/freshness_checksum.rs b/tests/testsuite/freshness_checksum.rs index 5c4b88fdbc3..1ae2ba1cd2c 100644 --- a/tests/testsuite/freshness_checksum.rs +++ b/tests/testsuite/freshness_checksum.rs @@ -1099,7 +1099,7 @@ new desc "#]]) .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the metadata changed +[DIRTY] foo v0.0.1 ([ROOT]/foo): the environment variable CARGO_PKG_DESCRIPTION changed [COMPILING] foo v0.0.1 ([ROOT]/foo) [RUNNING] `rustc [..] [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -1994,57 +1994,6 @@ fn script_fails_stay_dirty() { .run(); } -#[cargo_test(nightly, reason = "requires -Zchecksum-hash-algorithm")] -fn metadata_change_invalidates() { - let p = project() - .file( - "Cargo.toml", - r#" - [package] - name = "foo" - version = "0.1.0" - edition = "2015" - "#, - ) - .file("src/lib.rs", "") - .build(); - - p.cargo("build -Zchecksum-freshness") - .masquerade_as_nightly_cargo(&["checksum-freshness"]) - .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 -Zchecksum-freshness") - .masquerade_as_nightly_cargo(&["checksum-freshness"]) - .with_stderr_data(str![[r#" -[COMPILING] foo v0.1.0 ([ROOT]/foo) -[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s - -"#]]) - .run(); - } - p.cargo("build -Zchecksum-freshness -v") - .masquerade_as_nightly_cargo(&["checksum-freshness"]) - .with_stderr_data(str![[r#" -[FRESH] foo v0.1.0 ([ROOT]/foo) -[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s - -"#]]) - .run(); - assert_eq!(p.glob("target/debug/deps/libfoo-*.rlib").count(), 1); -} - #[cargo_test(nightly, reason = "requires -Zchecksum-hash-algorithm")] fn edition_change_invalidates() { const MANIFEST: &str = r#"