From 43c19133fc535e838c0c06f4a24bd92fbd8d1549 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 --- src/cargo/core/compiler/compilation.rs | 50 ++----- .../core/compiler/fingerprint/dep_info.rs | 6 +- .../core/compiler/fingerprint/dirty_reason.rs | 2 - src/cargo/core/compiler/fingerprint/mod.rs | 42 +++--- src/cargo/core/package.rs | 118 ++++++++++++++++ tests/testsuite/freshness.rs | 133 +++++++++++++----- tests/testsuite/freshness_checksum.rs | 53 +------ 7 files changed, 255 insertions(+), 149 deletions(-) diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 7531f0fa61b..62f5f235c87 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -1,5 +1,6 @@ //! Type definitions for the result of a compilation. +use std::borrow::Cow; use std::collections::{BTreeSet, HashMap}; use std::ffi::{OsStr, OsString}; use std::path::PathBuf; @@ -351,8 +352,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,45 +359,14 @@ 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()) - .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()); + for (key, value) in pkg.emitted_env_vars() { + match value { + Cow::Borrowed(value) => cmd.env(key, value), + Cow::Owned(ref value) => cmd.env(key, value.as_str()), + }; + } + + 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..ca71fec179d 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::package::EmittablePackageMetadata; 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 + EmittablePackageMetadata::is_emittable_env_var(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..88daf17f612 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 [`EmittablePackageMetadata`]) //! //! #### dep-info files for build system integration. //! @@ -371,6 +372,7 @@ mod dep_info; mod dirty_reason; +use std::borrow::Cow; use std::collections::hash_map::{Entry, HashMap}; use std::env; use std::fs; @@ -378,6 +380,7 @@ use std::fs::File; use std::hash::{self, Hash, Hasher}; use std::io::{self}; use std::path::{Path, PathBuf}; +use std::str::FromStr; use std::sync::{Arc, Mutex}; use std::time::SystemTime; @@ -391,6 +394,7 @@ use serde::{Deserialize, Serialize}; use tracing::{debug, info}; use crate::core::compiler::unit_graph::UnitDep; +use crate::core::package::EmittablePackageMetadata; use crate::core::Package; use crate::util; use crate::util::errors::CargoResult; @@ -612,10 +616,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 +831,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 +850,18 @@ impl LocalFingerprint { return Ok(Some(StaleItem::MissingFile(dep_info))); }; for (key, previous) in info.env.iter() { + if let Ok(t) = EmittablePackageMetadata::from_str(key.as_str()) { + let value = pkg.emitted_env_var(&t); + let value = match value { + Cow::Borrowed(v) => v, + Cow::Owned(ref v) => v.as_str(), + }; + + if Some(value) == previous.as_deref() { + continue; + } + } + let current = if key == CARGO_ENV { Some(cargo_exe.to_str().ok_or_else(|| { format_err!( @@ -932,7 +945,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 +1007,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 +1151,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 +1259,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 +1289,6 @@ impl hash::Hash for Fingerprint { profile, ref deps, ref local, - metadata, config, compile_kind, ref rustflags, @@ -1294,7 +1303,6 @@ impl hash::Hash for Fingerprint { path, profile, &*local, - metadata, config, compile_kind, rustflags, @@ -1445,7 +1453,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 +1537,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 +1565,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/package.rs b/src/cargo/core/package.rs index 3d7c84a4266..61a7cb7bd3f 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::cell::{Cell, Ref, RefCell, RefMut}; use std::cmp::Ordering; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; @@ -6,6 +7,7 @@ use std::hash; use std::mem; use std::path::{Path, PathBuf}; use std::rc::Rc; +use std::str::FromStr; use std::time::{Duration, Instant}; use anyhow::Context as _; @@ -252,6 +254,122 @@ impl Package { } } +/// "Emittable" here meaning emitted to rustc as enviromental variables. Usable by `env!()` +/// If these change we need to trigger a rebuild +/// NOTE: If you add a new variant also add a mapping entry in [`EMITTED_METADATA_MAPPINGS`] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum EmittablePackageMetadata { + ManifestDir, + ManifestPath, + Name, + Version, + VersionMajor, + VersionMinor, + VersionPatch, + VersionPre, + Description, + Homepage, + Repository, + License, + LicenseFile, + Authors, + RustVersion, + Readme, +} + +use crate::core::package::EmittablePackageMetadata::*; +const EMITTED_METADATA_MAPPINGS: [(EmittablePackageMetadata, &'static str); 16] = [ + (ManifestDir, "CARGO_MANIFEST_DIR"), + (ManifestPath, "CARGO_MANIFEST_PATH"), + (Name, "CARGO_PKG_NAME"), + (Version, "CARGO_PKG_VERSION"), + (VersionMajor, "CARGO_PKG_VERSION_MAJOR"), + (VersionMinor, "CARGO_PKG_VERSION_MINOR"), + (VersionPatch, "CARGO_PKG_VERSION_PATCH"), + (VersionPre, "CARGO_PKG_VERSION_PRE"), + (Description, "CARGO_PKG_DESCRIPTION"), + (Homepage, "CARGO_PKG_HOMEPAGE"), + (Repository, "CARGO_PKG_REPOSITORY"), + (License, "CARGO_PKG_LICENSE"), + (LicenseFile, "CARGO_PKG_LICENSE_FILE"), + (Authors, "CARGO_PKG_AUTHORS"), + (RustVersion, "CARGO_PKG_RUST_VERSION"), + (Readme, "CARGO_PKG_README"), +]; + +impl FromStr for EmittablePackageMetadata { + type Err = String; + + fn from_str(value: &str) -> Result { + if let Some(v) = EMITTED_METADATA_MAPPINGS.iter().find(|v| v.1 == value) { + return Ok(v.0); + } + + return Err(format!("Invalid emittable manifest metadata key {value}")); + } +} + +impl From for &'static str { + fn from(value: EmittablePackageMetadata) -> Self { + if let Some(v) = EMITTED_METADATA_MAPPINGS.iter().find(|v| v.0 == value) { + return v.1; + } + + unreachable!("unknown package metadata"); + } +} + +impl EmittablePackageMetadata { + pub fn is_emittable_env_var(key: &str) -> bool { + EmittablePackageMetadata::from_str(key).is_ok() + } +} + +impl Package { + // k/v pairs of the metadata environmental variables emitted to rustc. + pub fn emitted_env_vars(&self) -> Vec<(&'static str, Cow<'_, str>)> { + let mut vars = Vec::with_capacity(EMITTED_METADATA_MAPPINGS.len()); + + for (key, name) in EMITTED_METADATA_MAPPINGS { + let value = self.emitted_env_var(&key); + vars.push((name, value)); + } + + return vars; + } + + pub fn emitted_env_var<'a>(&'a self, key: &EmittablePackageMetadata) -> Cow<'a, str> { + let metadata = self.manifest().metadata(); + let value = match key { + ManifestDir => self.root().as_os_str().to_str().unwrap_or_default(), + ManifestPath => self + .manifest_path() + .as_os_str() + .to_str() + .unwrap_or_default(), + Name => self.name().as_str(), + Version => return Cow::Owned(self.version().to_string()), + VersionMajor => return Cow::Owned(self.version().major.to_string()), + VersionMinor => return Cow::Owned(self.version().minor.to_string()), + VersionPatch => return Cow::Owned(self.version().patch.to_string()), + VersionPre => return Cow::Owned(self.version().pre.to_string()), + Description => metadata.description.as_deref().unwrap_or_default(), + Homepage => metadata.homepage.as_deref().unwrap_or_default(), + Repository => metadata.repository.as_deref().unwrap_or_default(), + License => metadata.license.as_deref().unwrap_or_default(), + LicenseFile => metadata.license_file.as_deref().unwrap_or_default(), + Authors => return Cow::Owned(metadata.authors.join(":")), + RustVersion => { + let rust_version = metadata.rust_version.as_ref().map(ToString::to_string); + return Cow::Owned(rust_version.unwrap_or_default()); + } + Readme => metadata.readme.as_deref().unwrap_or_default(), + }; + + return Cow::Borrowed(value); + } +} + impl fmt::Display for Package { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.summary().package_id()) 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#"