Skip to content

Commit 3447b11

Browse files
weihangloweihanglo
authored
Moved manifest metadata tracking from fingerprint to dep info (#14973)
<!-- Thanks for submitting a pull request 🎉! Here are some tips for you: * If this is your first contribution, read "Cargo Contribution Guide" first: https://doc.crates.io/contrib/ * Run `cargo fmt --all` to format your code changes. * Small commits and pull requests are always preferable and easy to review. * If your idea is large and needs feedback from the community, read how: https://doc.crates.io/contrib/process/#working-on-large-features * Cargo takes care of compatibility. Read our design principles: https://doc.crates.io/contrib/design.html * When changing help text of cargo commands, follow the steps to generate docs: https://github.com/rust-lang/cargo/tree/master/src/doc#building-the-man-pages * If your PR is not finished, set it as "draft" PR or add "WIP" in its title. * It's ok to use the CI resources to test your PR, but please don't abuse them. ### What does this PR try to resolve? Explain the motivation behind this change. A clear overview along with an in-depth explanation are helpful. You can use `Fixes #<issue number>` to associate this PR to an existing issue. ### How should we test and review this PR? Demonstrate how you test this change and guide reviewers through your PR. With a smooth review process, a pull request usually gets reviewed quicker. If you don't know how to write and run your tests, please read the guide: https://doc.crates.io/contrib/tests ### Additional information Other information you want to mention in this PR, such as prior arts, future extensions, an unresolved problem, or a TODO list. --> ### What does this PR try to resolve? Fixes #14154 ~~Added some of the missing package metadata to the fingerprint so that rebuilds are triggered correctly.~~ ~~Also updated the test to prevent a future regression.~~ Moved the manifest metadata tracking to use dep info in favor of fingerprint so that rebuilds are only triggered if the metadata is actually used.
2 parents 8721d52 + 3d7b154 commit 3447b11

File tree

7 files changed

+207
-142
lines changed

7 files changed

+207
-142
lines changed

‎src/cargo/core/compiler/compilation.rs

Lines changed: 7 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -351,54 +351,27 @@ impl<'gctx> Compilation<'gctx> {
351351
}
352352
}
353353

354-
let metadata = pkg.manifest().metadata();
355-
356354
let cargo_exe = self.gctx.cargo_exe()?;
357355
cmd.env(crate::CARGO_ENV, cargo_exe);
358356

359357
// When adding new environment variables depending on
360358
// crate properties which might require rebuild upon change
361359
// consider adding the corresponding properties to the hash
362360
// in BuildContext::target_metadata()
363-
let rust_version = pkg.rust_version().as_ref().map(ToString::to_string);
364361
cmd.env("CARGO_MANIFEST_DIR", pkg.root())
365362
.env("CARGO_MANIFEST_PATH", pkg.manifest_path())
366363
.env("CARGO_PKG_VERSION_MAJOR", &pkg.version().major.to_string())
367364
.env("CARGO_PKG_VERSION_MINOR", &pkg.version().minor.to_string())
368365
.env("CARGO_PKG_VERSION_PATCH", &pkg.version().patch.to_string())
369366
.env("CARGO_PKG_VERSION_PRE", pkg.version().pre.as_str())
370367
.env("CARGO_PKG_VERSION", &pkg.version().to_string())
371-
.env("CARGO_PKG_NAME", &*pkg.name())
372-
.env(
373-
"CARGO_PKG_DESCRIPTION",
374-
metadata.description.as_ref().unwrap_or(&String::new()),
375-
)
376-
.env(
377-
"CARGO_PKG_HOMEPAGE",
378-
metadata.homepage.as_ref().unwrap_or(&String::new()),
379-
)
380-
.env(
381-
"CARGO_PKG_REPOSITORY",
382-
metadata.repository.as_ref().unwrap_or(&String::new()),
383-
)
384-
.env(
385-
"CARGO_PKG_LICENSE",
386-
metadata.license.as_ref().unwrap_or(&String::new()),
387-
)
388-
.env(
389-
"CARGO_PKG_LICENSE_FILE",
390-
metadata.license_file.as_ref().unwrap_or(&String::new()),
391-
)
392-
.env("CARGO_PKG_AUTHORS", &pkg.authors().join(":"))
393-
.env(
394-
"CARGO_PKG_RUST_VERSION",
395-
&rust_version.as_deref().unwrap_or_default(),
396-
)
397-
.env(
398-
"CARGO_PKG_README",
399-
metadata.readme.as_ref().unwrap_or(&String::new()),
400-
)
401-
.cwd(pkg.root());
368+
.env("CARGO_PKG_NAME", &*pkg.name());
369+
370+
for (key, value) in pkg.manifest().metadata().env_vars() {
371+
cmd.env(key, value.as_ref());
372+
}
373+
374+
cmd.cwd(pkg.root());
402375

403376
apply_env_config(self.gctx, &mut cmd)?;
404377

‎src/cargo/core/compiler/fingerprint/dep_info.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ use cargo_util::paths;
1919
use cargo_util::ProcessBuilder;
2020
use cargo_util::Sha256;
2121

22+
use crate::core::manifest::ManifestMetadata;
2223
use crate::CargoResult;
2324
use crate::CARGO_ENV;
2425

@@ -334,7 +335,10 @@ pub fn translate_dep_info(
334335
//
335336
// For cargo#13280, We trace env vars that are defined in the `[env]` config table.
336337
on_disk_info.env.retain(|(key, _)| {
337-
env_config.contains_key(key) || !rustc_cmd.get_envs().contains_key(key) || key == CARGO_ENV
338+
ManifestMetadata::should_track(key)
339+
|| env_config.contains_key(key)
340+
|| !rustc_cmd.get_envs().contains_key(key)
341+
|| key == CARGO_ENV
338342
});
339343

340344
let serialize_path = |file| {

‎src/cargo/core/compiler/fingerprint/dirty_reason.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ pub enum DirtyReason {
2626
old: Vec<String>,
2727
new: Vec<String>,
2828
},
29-
MetadataChanged,
3029
ConfigSettingsChanged,
3130
CompileKindChanged,
3231
LocalLengthsChanged,
@@ -168,7 +167,6 @@ impl DirtyReason {
168167
s.dirty_because(unit, "the profile configuration changed")
169168
}
170169
DirtyReason::RustflagsChanged { .. } => s.dirty_because(unit, "the rustflags changed"),
171-
DirtyReason::MetadataChanged => s.dirty_because(unit, "the metadata changed"),
172170
DirtyReason::ConfigSettingsChanged => {
173171
s.dirty_because(unit, "the config settings changed")
174172
}

‎src/cargo/core/compiler/fingerprint/mod.rs

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@
7878
//! [`CompileKind`] (host/target) | ✓ | ✓ | ✓ | ✓
7979
//! `__CARGO_DEFAULT_LIB_METADATA`[^4] | | ✓ | ✓ | ✓
8080
//! `package_id` | | ✓ | ✓ | ✓
81-
//! authors, description, homepage, repo | ✓ | | |
8281
//! Target src path relative to ws | ✓ | | |
8382
//! Target flags (test/bench/for_host/edition) | ✓ | | |
8483
//! -C incremental=… flag | ✓ | | |
@@ -189,6 +188,8 @@
189188
//! files to learn about environment variables that the rustc compile depends on.
190189
//! Cargo then later uses this to trigger a recompile if a referenced env var
191190
//! changes (even if the source didn't change).
191+
//! This also includes env vars generated from Cargo metadata like `CARGO_PKG_DESCRIPTION`.
192+
//! (See [`crate::core::manifest::ManifestMetadata`]
192193
//!
193194
//! #### dep-info files for build system integration.
194195
//!
@@ -612,10 +613,6 @@ pub struct Fingerprint {
612613
memoized_hash: Mutex<Option<u64>>,
613614
/// RUSTFLAGS/RUSTDOCFLAGS environment variable value (or config value).
614615
rustflags: Vec<String>,
615-
/// Hash of some metadata from the manifest, such as "authors", or
616-
/// "description", which are exposed as environment variables during
617-
/// compilation.
618-
metadata: u64,
619616
/// Hash of various config settings that change how things are compiled.
620617
config: u64,
621618
/// The rustc target. This is only relevant for `.json` files, otherwise
@@ -831,11 +828,12 @@ impl LocalFingerprint {
831828
&self,
832829
mtime_cache: &mut HashMap<PathBuf, FileTime>,
833830
checksum_cache: &mut HashMap<PathBuf, Checksum>,
834-
pkg_root: &Path,
831+
pkg: &Package,
835832
target_root: &Path,
836833
cargo_exe: &Path,
837834
gctx: &GlobalContext,
838835
) -> CargoResult<Option<StaleItem>> {
836+
let pkg_root = pkg.root();
839837
match self {
840838
// We need to parse `dep_info`, learn about the crate's dependencies.
841839
//
@@ -849,6 +847,12 @@ impl LocalFingerprint {
849847
return Ok(Some(StaleItem::MissingFile(dep_info)));
850848
};
851849
for (key, previous) in info.env.iter() {
850+
if let Some(value) = pkg.manifest().metadata().env_var(key.as_str()) {
851+
if Some(value.as_ref()) == previous.as_deref() {
852+
continue;
853+
}
854+
}
855+
852856
let current = if key == CARGO_ENV {
853857
Some(cargo_exe.to_str().ok_or_else(|| {
854858
format_err!(
@@ -932,7 +936,6 @@ impl Fingerprint {
932936
local: Mutex::new(Vec::new()),
933937
memoized_hash: Mutex::new(None),
934938
rustflags: Vec::new(),
935-
metadata: 0,
936939
config: 0,
937940
compile_kind: 0,
938941
fs_status: FsStatus::Stale,
@@ -995,9 +998,6 @@ impl Fingerprint {
995998
new: self.rustflags.clone(),
996999
};
9971000
}
998-
if self.metadata != old.metadata {
999-
return DirtyReason::MetadataChanged;
1000-
}
10011001
if self.config != old.config {
10021002
return DirtyReason::ConfigSettingsChanged;
10031003
}
@@ -1142,13 +1142,14 @@ impl Fingerprint {
11421142
&mut self,
11431143
mtime_cache: &mut HashMap<PathBuf, FileTime>,
11441144
checksum_cache: &mut HashMap<PathBuf, Checksum>,
1145-
pkg_root: &Path,
1145+
pkg: &Package,
11461146
target_root: &Path,
11471147
cargo_exe: &Path,
11481148
gctx: &GlobalContext,
11491149
) -> CargoResult<()> {
11501150
assert!(!self.fs_status.up_to_date());
11511151

1152+
let pkg_root = pkg.root();
11521153
let mut mtimes = HashMap::new();
11531154

11541155
// Get the `mtime` of all outputs. Optionally update their mtime
@@ -1249,7 +1250,7 @@ impl Fingerprint {
12491250
if let Some(item) = local.find_stale_item(
12501251
mtime_cache,
12511252
checksum_cache,
1252-
pkg_root,
1253+
pkg,
12531254
target_root,
12541255
cargo_exe,
12551256
gctx,
@@ -1279,7 +1280,6 @@ impl hash::Hash for Fingerprint {
12791280
profile,
12801281
ref deps,
12811282
ref local,
1282-
metadata,
12831283
config,
12841284
compile_kind,
12851285
ref rustflags,
@@ -1294,7 +1294,6 @@ impl hash::Hash for Fingerprint {
12941294
path,
12951295
profile,
12961296
&*local,
1297-
metadata,
12981297
config,
12991298
compile_kind,
13001299
rustflags,
@@ -1445,7 +1444,7 @@ fn calculate(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResult
14451444
fingerprint.check_filesystem(
14461445
&mut build_runner.mtime_cache,
14471446
&mut build_runner.checksum_cache,
1448-
unit.pkg.root(),
1447+
&unit.pkg,
14491448
&target_root,
14501449
cargo_exe,
14511450
build_runner.bcx.gctx,
@@ -1529,9 +1528,6 @@ fn calculate_normal(
15291528
build_runner.lto[unit],
15301529
unit.pkg.manifest().lint_rustflags(),
15311530
));
1532-
// Include metadata since it is exposed as environment variables.
1533-
let m = unit.pkg.manifest().metadata();
1534-
let metadata = util::hash_u64((&m.authors, &m.description, &m.homepage, &m.repository));
15351531
let mut config = StableHasher::new();
15361532
if let Some(linker) = build_runner.compilation.target_linker(unit.kind) {
15371533
linker.hash(&mut config);
@@ -1560,7 +1556,6 @@ fn calculate_normal(
15601556
deps,
15611557
local: Mutex::new(local),
15621558
memoized_hash: Mutex::new(None),
1563-
metadata,
15641559
config: Hasher::finish(&config),
15651560
compile_kind,
15661561
rustflags: extra_flags,

‎src/cargo/core/manifest.rs

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
use std::borrow::Cow;
12
use std::collections::{BTreeMap, HashMap};
23
use std::fmt;
34
use std::hash::{Hash, Hasher};
@@ -146,6 +147,86 @@ pub struct ManifestMetadata {
146147
pub rust_version: Option<RustVersion>,
147148
}
148149

150+
macro_rules! get_metadata_env {
151+
($meta:ident, $field:ident) => {
152+
$meta.$field.as_deref().unwrap_or_default().into()
153+
};
154+
($meta:ident, $field:ident, $to_var:expr) => {
155+
$to_var($meta).into()
156+
};
157+
}
158+
159+
macro_rules! metadata_envs {
160+
(
161+
$(
162+
($field:ident, $key:literal$(, $to_var:expr)?),
163+
)*
164+
) => {
165+
struct MetadataEnvs;
166+
impl MetadataEnvs {
167+
$(
168+
fn $field(meta: &ManifestMetadata) -> Cow<'_, str> {
169+
get_metadata_env!(meta, $field$(, $to_var)?)
170+
}
171+
)*
172+
173+
pub fn should_track(key: &str) -> bool {
174+
let keys = [$($key),*];
175+
key.strip_prefix("CARGO_PKG_")
176+
.map(|key| keys.iter().any(|k| *k == key))
177+
.unwrap_or_default()
178+
}
179+
180+
pub fn var<'a>(meta: &'a ManifestMetadata, key: &str) -> Option<Cow<'a, str>> {
181+
key.strip_prefix("CARGO_PKG_").and_then(|key| match key {
182+
$($key => Some(Self::$field(meta)),)*
183+
_ => None,
184+
})
185+
}
186+
187+
pub fn vars(meta: &ManifestMetadata) -> impl Iterator<Item = (&'static str, Cow<'_, str>)> {
188+
[
189+
$(
190+
(
191+
concat!("CARGO_PKG_", $key),
192+
Self::$field(meta),
193+
),
194+
)*
195+
].into_iter()
196+
}
197+
}
198+
}
199+
}
200+
201+
// Metadata enviromental variables that are emitted to rustc. Usable by `env!()`
202+
// If these change we need to trigger a rebuild.
203+
// NOTE: The env var name will be prefixed with `CARGO_PKG_`
204+
metadata_envs! {
205+
(description, "DESCRIPTION"),
206+
(homepage, "HOMEPAGE"),
207+
(repository, "REPOSITORY"),
208+
(license, "LICENSE"),
209+
(license_file, "LICENSE_FILE"),
210+
(authors, "AUTHORS", |m: &ManifestMetadata| m.authors.join(":")),
211+
(rust_version, "RUST_VERSION", |m: &ManifestMetadata| m.rust_version.as_ref().map(ToString::to_string).unwrap_or_default()),
212+
(readme, "README"),
213+
}
214+
215+
impl ManifestMetadata {
216+
/// Whether the given env var should be tracked by Cargo's dep-info.
217+
pub fn should_track(env_key: &str) -> bool {
218+
MetadataEnvs::should_track(env_key)
219+
}
220+
221+
pub fn env_var<'a>(&'a self, env_key: &str) -> Option<Cow<'a, str>> {
222+
MetadataEnvs::var(self, env_key)
223+
}
224+
225+
pub fn env_vars(&self) -> impl Iterator<Item = (&'static str, Cow<'_, str>)> {
226+
MetadataEnvs::vars(self)
227+
}
228+
}
229+
149230
#[derive(Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
150231
pub enum TargetKind {
151232
Lib(Vec<CrateType>),

0 commit comments

Comments
 (0)