From b2aa08bbc66e08dc5b2a360ebbb7b134666ae7e3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 5 Dec 2024 16:10:47 -0600 Subject: [PATCH 1/9] test(fingerprint): Verify rustc extra flags like RUSTFLAGS I almost overlooked these --- tests/testsuite/freshness.rs | 42 ++++++++++++++++++++++++ tests/testsuite/freshness_checksum.rs | 46 +++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/tests/testsuite/freshness.rs b/tests/testsuite/freshness.rs index e01adc6564b..7e5a68a9407 100644 --- a/tests/testsuite/freshness.rs +++ b/tests/testsuite/freshness.rs @@ -1366,6 +1366,48 @@ fn changing_rustflags_is_cached() { .run(); } +#[cargo_test] +fn changing_rustc_extra_flags_is_cached() { + let p = project().file("src/lib.rs", "").build(); + + // This isn't ever cached, we always have to recompile + p.cargo("rustc") + .with_stderr_data(str![[r#" +[COMPILING] foo v0.0.1 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); + p.cargo("rustc -v -- -C linker=cc") + .with_stderr_data(str![[r#" +[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed +[COMPILING] foo v0.0.1 ([ROOT]/foo) +[RUNNING] `rustc [..] +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); + + p.cargo("rustc -v") + .with_stderr_data(str![[r#" +[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed +[COMPILING] foo v0.0.1 ([ROOT]/foo) +[RUNNING] `rustc [..] +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); + p.cargo("rustc -v -- -C linker=cc") + .with_stderr_data(str![[r#" +[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed +[COMPILING] foo v0.0.1 ([ROOT]/foo) +[RUNNING] `rustc [..] +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +} + #[cargo_test] fn update_dependency_mtime_does_not_rebuild() { let p = project() diff --git a/tests/testsuite/freshness_checksum.rs b/tests/testsuite/freshness_checksum.rs index 386ee11a31a..c2f8b65b139 100644 --- a/tests/testsuite/freshness_checksum.rs +++ b/tests/testsuite/freshness_checksum.rs @@ -1540,6 +1540,52 @@ fn changing_rustflags_is_cached() { .run(); } +#[cargo_test(nightly, reason = "requires -Zchecksum-hash-algorithm")] +fn changing_rustc_extra_flags_is_cached() { + let p = project().file("src/lib.rs", "").build(); + + // This isn't ever cached, we always have to recompile + p.cargo("rustc -Zchecksum-freshness") + .masquerade_as_nightly_cargo(&["checksum-freshness"]) + .with_stderr_data(str![[r#" +[COMPILING] foo v0.0.1 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); + p.cargo("rustc -Zchecksum-freshness -v -- -C linker=cc") + .masquerade_as_nightly_cargo(&["checksum-freshness"]) + .with_stderr_data(str![[r#" +[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed +[COMPILING] foo v0.0.1 ([ROOT]/foo) +[RUNNING] `rustc [..] +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); + + p.cargo("rustc -Zchecksum-freshness -v") + .masquerade_as_nightly_cargo(&["checksum-freshness"]) + .with_stderr_data(str![[r#" +[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed +[COMPILING] foo v0.0.1 ([ROOT]/foo) +[RUNNING] `rustc [..] src/lib.rs [..] +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); + p.cargo("rustc -Zchecksum-freshness -v -- -C linker=cc") + .masquerade_as_nightly_cargo(&["checksum-freshness"]) + .with_stderr_data(str![[r#" +[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed +[COMPILING] foo v0.0.1 ([ROOT]/foo) +[RUNNING] `rustc [..] +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); +} + #[cargo_test(nightly, reason = "requires -Zchecksum-hash-algorithm")] fn reuse_panic_build_dep_test() { let p = project() From 6f937203b462f26de36f7dc3fdb029d28bb4a6b9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Nov 2024 14:53:38 -0600 Subject: [PATCH 2/9] refactor(fingerprint): Separate Metadata lookup from Metadata::meta_hash lookup --- .../compiler/build_runner/compilation_files.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/compiler/build_runner/compilation_files.rs b/src/cargo/core/compiler/build_runner/compilation_files.rs index d8199c72614..36a92ac9e49 100644 --- a/src/cargo/core/compiler/build_runner/compilation_files.rs +++ b/src/cargo/core/compiler/build_runner/compilation_files.rs @@ -588,6 +588,12 @@ fn compute_metadata( metas: &mut HashMap, ) -> Metadata { let bcx = &build_runner.bcx; + let deps_metadata = build_runner + .unit_deps(unit) + .iter() + .map(|dep| *metadata_of(&dep.unit, build_runner, metas)) + .collect::>(); + let mut hasher = StableHasher::new(); METADATA_VERSION.hash(&mut hasher); @@ -604,13 +610,12 @@ fn compute_metadata( unit.features.hash(&mut hasher); // Mix in the target-metadata of all the dependencies of this target. - let mut deps_metadata = build_runner - .unit_deps(unit) + let mut dep_hashes = deps_metadata .iter() - .map(|dep| metadata_of(&dep.unit, build_runner, metas).meta_hash) + .map(|m| m.meta_hash) .collect::>(); - deps_metadata.sort(); - deps_metadata.hash(&mut hasher); + dep_hashes.sort(); + dep_hashes.hash(&mut hasher); // Throw in the profile we're compiling with. This helps caching // `panic=abort` and `panic=unwind` artifacts, additionally with various From 43eccbc92437232171046f948cb7890f9b358737 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Nov 2024 14:54:20 -0600 Subject: [PATCH 3/9] refactor(fingerprint): Hash dependencies at the end --- .../compiler/build_runner/compilation_files.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/cargo/core/compiler/build_runner/compilation_files.rs b/src/cargo/core/compiler/build_runner/compilation_files.rs index 36a92ac9e49..e1d3ee979c2 100644 --- a/src/cargo/core/compiler/build_runner/compilation_files.rs +++ b/src/cargo/core/compiler/build_runner/compilation_files.rs @@ -609,14 +609,6 @@ fn compute_metadata( // when changing feature sets each lib is separately cached. unit.features.hash(&mut hasher); - // Mix in the target-metadata of all the dependencies of this target. - let mut dep_hashes = deps_metadata - .iter() - .map(|m| m.meta_hash) - .collect::>(); - dep_hashes.sort(); - dep_hashes.hash(&mut hasher); - // Throw in the profile we're compiling with. This helps caching // `panic=abort` and `panic=unwind` artifacts, additionally with various // settings like debuginfo and whatnot. @@ -685,6 +677,14 @@ fn compute_metadata( target_configs_are_different.hash(&mut hasher); } + // Mix in the target-metadata of all the dependencies of this target. + let mut dep_hashes = deps_metadata + .iter() + .map(|m| m.meta_hash) + .collect::>(); + dep_hashes.sort(); + dep_hashes.hash(&mut hasher); + Metadata { meta_hash: UnitHash(hasher.finish()), use_extra_filename: use_extra_filename(bcx, unit), From a251ee58212c13756de60604f2275bdc8e7eb455 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Nov 2024 14:57:24 -0600 Subject: [PATCH 4/9] refactor(fingerprint): Separate out Metadata field initialization --- src/cargo/core/compiler/build_runner/compilation_files.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/compiler/build_runner/compilation_files.rs b/src/cargo/core/compiler/build_runner/compilation_files.rs index e1d3ee979c2..24f302412bc 100644 --- a/src/cargo/core/compiler/build_runner/compilation_files.rs +++ b/src/cargo/core/compiler/build_runner/compilation_files.rs @@ -593,6 +593,7 @@ fn compute_metadata( .iter() .map(|dep| *metadata_of(&dep.unit, build_runner, metas)) .collect::>(); + let use_extra_filename = use_extra_filename(bcx, unit); let mut hasher = StableHasher::new(); @@ -685,9 +686,11 @@ fn compute_metadata( dep_hashes.sort(); dep_hashes.hash(&mut hasher); + let meta_hash = UnitHash(hasher.finish()); + Metadata { - meta_hash: UnitHash(hasher.finish()), - use_extra_filename: use_extra_filename(bcx, unit), + meta_hash, + use_extra_filename, } } From 79333d5d57ef1d0c82b96fc244d554b679a820f6 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Nov 2024 15:02:46 -0600 Subject: [PATCH 5/9] refactor(fingerprint): Make the hasher name more specific --- .../build_runner/compilation_files.rs | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/cargo/core/compiler/build_runner/compilation_files.rs b/src/cargo/core/compiler/build_runner/compilation_files.rs index 24f302412bc..8a9ca9c4d11 100644 --- a/src/cargo/core/compiler/build_runner/compilation_files.rs +++ b/src/cargo/core/compiler/build_runner/compilation_files.rs @@ -595,47 +595,47 @@ fn compute_metadata( .collect::>(); let use_extra_filename = use_extra_filename(bcx, unit); - let mut hasher = StableHasher::new(); + let mut shared_hasher = StableHasher::new(); - METADATA_VERSION.hash(&mut hasher); + METADATA_VERSION.hash(&mut shared_hasher); // Unique metadata per (name, source, version) triple. This'll allow us // to pull crates from anywhere without worrying about conflicts. unit.pkg .package_id() .stable_hash(bcx.ws.root()) - .hash(&mut hasher); + .hash(&mut shared_hasher); // Also mix in enabled features to our metadata. This'll ensure that // when changing feature sets each lib is separately cached. - unit.features.hash(&mut hasher); + unit.features.hash(&mut shared_hasher); // Throw in the profile we're compiling with. This helps caching // `panic=abort` and `panic=unwind` artifacts, additionally with various // settings like debuginfo and whatnot. - unit.profile.hash(&mut hasher); - unit.mode.hash(&mut hasher); - build_runner.lto[unit].hash(&mut hasher); + unit.profile.hash(&mut shared_hasher); + unit.mode.hash(&mut shared_hasher); + build_runner.lto[unit].hash(&mut shared_hasher); // Artifacts compiled for the host should have a different // metadata piece than those compiled for the target, so make sure // we throw in the unit's `kind` as well. Use `fingerprint_hash` // so that the StableHash doesn't change based on the pathnames // of the custom target JSON spec files. - unit.kind.fingerprint_hash().hash(&mut hasher); + unit.kind.fingerprint_hash().hash(&mut shared_hasher); // Finally throw in the target name/kind. This ensures that concurrent // compiles of targets in the same crate don't collide. - unit.target.name().hash(&mut hasher); - unit.target.kind().hash(&mut hasher); + unit.target.name().hash(&mut shared_hasher); + unit.target.kind().hash(&mut shared_hasher); - hash_rustc_version(bcx, &mut hasher, unit); + hash_rustc_version(bcx, &mut shared_hasher, unit); if build_runner.bcx.ws.is_member(&unit.pkg) { // This is primarily here for clippy. This ensures that the clippy // artifacts are separate from the `check` ones. if let Some(path) = &build_runner.bcx.rustc().workspace_wrapper { - path.hash(&mut hasher); + path.hash(&mut shared_hasher); } } @@ -646,7 +646,7 @@ fn compute_metadata( .gctx .get_env("__CARGO_DEFAULT_LIB_METADATA") { - channel.hash(&mut hasher); + channel.hash(&mut shared_hasher); } // std units need to be kept separate from user dependencies. std crates @@ -656,7 +656,7 @@ fn compute_metadata( // don't need unstable support. A future experiment might be to set // `is_std` to false for build dependencies so that they can be shared // with user dependencies. - unit.is_std.hash(&mut hasher); + unit.is_std.hash(&mut shared_hasher); // While we don't hash RUSTFLAGS because it may contain absolute paths that // hurts reproducibility, we track whether a unit's RUSTFLAGS is from host @@ -675,7 +675,7 @@ fn compute_metadata( .target_config(CompileKind::Host) .links_overrides != unit.links_overrides; - target_configs_are_different.hash(&mut hasher); + target_configs_are_different.hash(&mut shared_hasher); } // Mix in the target-metadata of all the dependencies of this target. @@ -684,9 +684,9 @@ fn compute_metadata( .map(|m| m.meta_hash) .collect::>(); dep_hashes.sort(); - dep_hashes.hash(&mut hasher); + dep_hashes.hash(&mut shared_hasher); - let meta_hash = UnitHash(hasher.finish()); + let meta_hash = UnitHash(shared_hasher.finish()); Metadata { meta_hash, From cdf805e50991a67e51f0ed5258593794df1cdb8f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Nov 2024 15:18:12 -0600 Subject: [PATCH 6/9] refactor(fingerprint): Generate separate c_metadata / c_extra_filename hashes For now, they should result in the same data. --- .../build_runner/compilation_files.rs | 40 +++++++++++++------ src/cargo/util/hasher.rs | 1 + 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/cargo/core/compiler/build_runner/compilation_files.rs b/src/cargo/core/compiler/build_runner/compilation_files.rs index 8a9ca9c4d11..4203a0d46b5 100644 --- a/src/cargo/core/compiler/build_runner/compilation_files.rs +++ b/src/cargo/core/compiler/build_runner/compilation_files.rs @@ -86,24 +86,25 @@ impl fmt::Debug for UnitHash { /// rebuild is needed. #[derive(Copy, Clone, Debug)] pub struct Metadata { - meta_hash: UnitHash, - use_extra_filename: bool, + unit_id: UnitHash, + c_metadata: UnitHash, + c_extra_filename: Option, } impl Metadata { /// A hash to identify a given [`Unit`] in the build graph pub fn unit_id(&self) -> UnitHash { - self.meta_hash + self.unit_id } /// A hash to add to symbol naming through `-C metadata` pub fn c_metadata(&self) -> UnitHash { - self.meta_hash + self.c_metadata } /// A hash to add to file names through `-C extra-filename` pub fn c_extra_filename(&self) -> Option { - self.use_extra_filename.then_some(self.meta_hash) + self.c_extra_filename } } @@ -678,19 +679,34 @@ fn compute_metadata( target_configs_are_different.hash(&mut shared_hasher); } + let mut c_metadata_hasher = shared_hasher.clone(); // Mix in the target-metadata of all the dependencies of this target. - let mut dep_hashes = deps_metadata + let mut dep_c_metadata_hashes = deps_metadata .iter() - .map(|m| m.meta_hash) + .map(|m| m.c_metadata) .collect::>(); - dep_hashes.sort(); - dep_hashes.hash(&mut shared_hasher); + dep_c_metadata_hashes.sort(); + dep_c_metadata_hashes.hash(&mut c_metadata_hasher); - let meta_hash = UnitHash(shared_hasher.finish()); + let mut c_extra_filename_hasher = shared_hasher.clone(); + // Mix in the target-metadata of all the dependencies of this target. + let mut dep_c_extra_filename_hashes = deps_metadata + .iter() + .map(|m| m.c_extra_filename) + .collect::>(); + dep_c_extra_filename_hashes.sort(); + dep_c_extra_filename_hashes.hash(&mut c_extra_filename_hasher); + + let c_metadata = UnitHash(c_metadata_hasher.finish()); + let c_extra_filename = UnitHash(c_extra_filename_hasher.finish()); + let unit_id = c_extra_filename; + + let c_extra_filename = use_extra_filename.then_some(c_extra_filename); Metadata { - meta_hash, - use_extra_filename, + unit_id, + c_metadata, + c_extra_filename, } } diff --git a/src/cargo/util/hasher.rs b/src/cargo/util/hasher.rs index 01e15ae2c04..87586c0900f 100644 --- a/src/cargo/util/hasher.rs +++ b/src/cargo/util/hasher.rs @@ -6,6 +6,7 @@ use std::hash::{Hasher, SipHasher}; +#[derive(Clone)] pub struct StableHasher(SipHasher); impl StableHasher { From 2bf35daf947ca25032161c6702b2ca7c66334fc9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Nov 2024 15:31:20 -0600 Subject: [PATCH 7/9] docs(fingerprint): Track each use of Metadata separately --- .../build_runner/compilation_files.rs | 36 +++++----- src/cargo/core/compiler/fingerprint/mod.rs | 66 ++++++++++--------- 2 files changed, 56 insertions(+), 46 deletions(-) diff --git a/src/cargo/core/compiler/build_runner/compilation_files.rs b/src/cargo/core/compiler/build_runner/compilation_files.rs index 4203a0d46b5..b07b59aac4c 100644 --- a/src/cargo/core/compiler/build_runner/compilation_files.rs +++ b/src/cargo/core/compiler/build_runner/compilation_files.rs @@ -41,24 +41,25 @@ impl fmt::Debug for UnitHash { } } -/// The `Metadata` is a hash used to make unique file names for each unit in a -/// build. It is also used for symbol mangling. +/// [`Metadata`] tracks several [`UnitHash`]s, including +/// [`Metadata::unit_id`], [`Metadata::c_metadata`], and [`Metadata::c_extra_filename`]. /// -/// For example: +/// We use a hash because it is an easy way to guarantee +/// that all the inputs can be converted to a valid path. +/// +/// [`Metadata::unit_id`] is used to uniquely identify a unit in the build graph. +/// This serves as a similar role as [`Metadata::c_extra_filename`] in that it uniquely identifies output +/// on the filesystem except that its always present. +/// +/// [`Metadata::c_extra_filename`] is needed for cases like: /// - A project may depend on crate `A` and crate `B`, so the package name must be in the file name. /// - Similarly a project may depend on two versions of `A`, so the version must be in the file name. /// -/// In general this must include all things that need to be distinguished in different parts of +/// This also acts as the main layer of caching provided by Cargo +/// so this must include all things that need to be distinguished in different parts of /// the same build. This is absolutely required or we override things before /// we get chance to use them. /// -/// It is also used for symbol mangling, because if you have two versions of -/// the same crate linked together, their symbols need to be differentiated. -/// -/// We use a hash because it is an easy way to guarantee -/// that all the inputs can be converted to a valid path. -/// -/// This also acts as the main layer of caching provided by Cargo. /// For example, we want to cache `cargo build` and `cargo doc` separately, so that running one /// does not invalidate the artifacts for the other. We do this by including [`CompileMode`] in the /// hash, thus the artifacts go in different folders and do not override each other. @@ -73,17 +74,20 @@ impl fmt::Debug for UnitHash { /// more space than needed. This makes not including something in `Metadata` /// a form of cache invalidation. /// -/// You should also avoid anything that would interfere with reproducible +/// Note that the `Fingerprint` is in charge of tracking everything needed to determine if a +/// rebuild is needed. +/// +/// [`Metadata::c_metadata`] is used for symbol mangling, because if you have two versions of +/// the same crate linked together, their symbols need to be differentiated. +/// +/// You should avoid anything that would interfere with reproducible /// builds. For example, *any* absolute path should be avoided. This is one -/// reason that `RUSTFLAGS` is not in `Metadata`, because it often has +/// reason that `RUSTFLAGS` is not in [`Metadata::c_metadata`], because it often has /// absolute paths (like `--remap-path-prefix` which is fundamentally used for /// reproducible builds and has absolute paths in it). Also, in some cases the /// mangled symbols need to be stable between different builds with different /// settings. For example, profile-guided optimizations need to swap /// `RUSTFLAGS` between runs, but needs to keep the same symbol names. -/// -/// Note that the `Fingerprint` is in charge of tracking everything needed to determine if a -/// rebuild is needed. #[derive(Copy, Clone, Debug)] pub struct Metadata { unit_id: UnitHash, diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs index f4cb2e0c3e2..7552b534f39 100644 --- a/src/cargo/core/compiler/fingerprint/mod.rs +++ b/src/cargo/core/compiler/fingerprint/mod.rs @@ -47,46 +47,48 @@ //! reliable and reproducible builds at the cost of being complex, slow, and //! platform-dependent. //! -//! ## Fingerprints and Metadata +//! ## Fingerprints and [`UnitHash`]s +//! +//! [`Metadata`] tracks several [`UnitHash`]s, including +//! [`Metadata::unit_id`], [`Metadata::c_metadata`], and [`Metadata::c_extra_filename`]. +//! See its documentation for more details. //! -//! The [`Metadata`] hash is a hash added to the output filenames to isolate -//! each unit. See its documentationfor more details. //! NOTE: Not all output files are isolated via filename hashes (like dylibs). //! The fingerprint directory uses a hash, but sometimes units share the same //! fingerprint directory (when they don't have Metadata) so care should be //! taken to handle this! //! -//! Fingerprints and Metadata are similar, and track some of the same things. -//! The Metadata contains information that is required to keep Units separate. +//! Fingerprints and [`UnitHash`]s are similar, and track some of the same things. +//! [`UnitHash`]s contains information that is required to keep Units separate. //! The Fingerprint includes additional information that should cause a //! recompile, but it is desired to reuse the same filenames. A comparison //! of what is tracked: //! -//! Value | Fingerprint | Metadata -//! -------------------------------------------|-------------|---------- -//! rustc | ✓ | ✓ -//! [`Profile`] | ✓ | ✓ -//! `cargo rustc` extra args | ✓ | -//! [`CompileMode`] | ✓ | ✓ -//! Target Name | ✓ | ✓ -//! `TargetKind` (bin/lib/etc.) | ✓ | ✓ -//! Enabled Features | ✓ | ✓ -//! Declared Features | ✓ | -//! Immediate dependency’s hashes | ✓[^1] | ✓ -//! [`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 | ✓ | -//! mtime of sources | ✓[^3] | -//! RUSTFLAGS/RUSTDOCFLAGS | ✓ | -//! [`Lto`] flags | ✓ | ✓ -//! config settings[^5] | ✓ | -//! `is_std` | | ✓ -//! `[lints]` table[^6] | ✓ | -//! `[lints.rust.unexpected_cfgs.check-cfg]` | ✓ | +//! Value | Fingerprint | `Metadata::unit_id` | `Metadata::c_metadata` | `Metadata::c_extra_filename` +//! -------------------------------------------|-------------|---------------------|------------------------|---------- +//! rustc | ✓ | ✓ | ✓ | ✓ +//! [`Profile`] | ✓ | ✓ | ✓ | ✓ +//! `cargo rustc` extra args | ✓ | | | +//! [`CompileMode`] | ✓ | ✓ | ✓ | ✓ +//! Target Name | ✓ | ✓ | ✓ | ✓ +//! `TargetKind` (bin/lib/etc.) | ✓ | ✓ | ✓ | ✓ +//! Enabled Features | ✓ | ✓ | ✓ | ✓ +//! Declared Features | ✓ | | | +//! Immediate dependency’s hashes | ✓[^1] | ✓ | ✓ | ✓ +//! [`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 | ✓ | | | +//! mtime of sources | ✓[^3] | | | +//! RUSTFLAGS/RUSTDOCFLAGS | ✓ | | | +//! [`Lto`] flags | ✓ | ✓ | ✓ | ✓ +//! config settings[^5] | ✓ | | | +//! `is_std` | | ✓ | ✓ | ✓ +//! `[lints]` table[^6] | ✓ | | | +//! `[lints.rust.unexpected_cfgs.check-cfg]` | ✓ | | | //! //! [^1]: Build script and bin dependencies are not included. //! @@ -348,6 +350,10 @@ //! //! [`check_filesystem`]: Fingerprint::check_filesystem //! [`Metadata`]: crate::core::compiler::Metadata +//! [`Metadata::unit_id`]: crate::core::compiler::Metadata::unit_id +//! [`Metadata::c_metadata`]: crate::core::compiler::Metadata::c_metadata +//! [`Metadata::c_extra_filename`]: crate::core::compiler::Metadata::c_extra_filename +//! [`UnitHash`]: crate::core::compiler::UnitHash //! [`Profile`]: crate::core::profiles::Profile //! [`CompileMode`]: crate::core::compiler::CompileMode //! [`Lto`]: crate::core::compiler::Lto From 306d515c081cebb95d989fd6698560d8ad4e1dd0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Nov 2024 15:51:47 -0600 Subject: [PATCH 8/9] fix(fingerprint): Don't throwaway the cache on RUSTFLAGS changes Fixes #8716 --- .../compiler/build_runner/compilation_files.rs | 12 ++++++++++++ src/cargo/core/compiler/fingerprint/mod.rs | 4 ++-- tests/testsuite/config_include.rs | 1 - tests/testsuite/freshness.rs | 18 ++++-------------- tests/testsuite/freshness_checksum.rs | 18 ++++-------------- tests/testsuite/pgo.rs | 1 - tests/testsuite/rustc.rs | 1 - tests/testsuite/rustflags.rs | 4 ++-- 8 files changed, 24 insertions(+), 35 deletions(-) diff --git a/src/cargo/core/compiler/build_runner/compilation_files.rs b/src/cargo/core/compiler/build_runner/compilation_files.rs index b07b59aac4c..363529ac442 100644 --- a/src/cargo/core/compiler/build_runner/compilation_files.rs +++ b/src/cargo/core/compiler/build_runner/compilation_files.rs @@ -700,6 +700,18 @@ fn compute_metadata( .collect::>(); dep_c_extra_filename_hashes.sort(); dep_c_extra_filename_hashes.hash(&mut c_extra_filename_hasher); + // Avoid trashing the caches on RUSTFLAGS changing via `c_extra_filename` + // + // Limited to `c_extra_filename` to help with reproducible build / PGO issues. + build_runner + .bcx + .extra_args_for(unit) + .hash(&mut c_extra_filename_hasher); + if unit.mode.is_doc() || unit.mode.is_doc_scrape() { + unit.rustdocflags.hash(&mut c_extra_filename_hasher); + } else { + unit.rustflags.hash(&mut c_extra_filename_hasher); + } let c_metadata = UnitHash(c_metadata_hasher.finish()); let c_extra_filename = UnitHash(c_extra_filename_hasher.finish()); diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs index 7552b534f39..c92dedccdd8 100644 --- a/src/cargo/core/compiler/fingerprint/mod.rs +++ b/src/cargo/core/compiler/fingerprint/mod.rs @@ -68,7 +68,7 @@ //! -------------------------------------------|-------------|---------------------|------------------------|---------- //! rustc | ✓ | ✓ | ✓ | ✓ //! [`Profile`] | ✓ | ✓ | ✓ | ✓ -//! `cargo rustc` extra args | ✓ | | | +//! `cargo rustc` extra args | ✓ | ✓ | | ✓ //! [`CompileMode`] | ✓ | ✓ | ✓ | ✓ //! Target Name | ✓ | ✓ | ✓ | ✓ //! `TargetKind` (bin/lib/etc.) | ✓ | ✓ | ✓ | ✓ @@ -83,7 +83,7 @@ //! Target flags (test/bench/for_host/edition) | ✓ | | | //! -C incremental=… flag | ✓ | | | //! mtime of sources | ✓[^3] | | | -//! RUSTFLAGS/RUSTDOCFLAGS | ✓ | | | +//! RUSTFLAGS/RUSTDOCFLAGS | ✓ | ✓ | | ✓ //! [`Lto`] flags | ✓ | ✓ | ✓ | ✓ //! config settings[^5] | ✓ | | | //! `is_std` | | ✓ | ✓ | ✓ diff --git a/tests/testsuite/config_include.rs b/tests/testsuite/config_include.rs index 1bb7e717a57..468115dd508 100644 --- a/tests/testsuite/config_include.rs +++ b/tests/testsuite/config_include.rs @@ -244,7 +244,6 @@ fn works_with_cli() { p.cargo("check -v -Z config-include") .masquerade_as_nightly_cargo(&["config-include"]) .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the rustflags changed [CHECKING] foo v0.0.1 ([ROOT]/foo) [RUNNING] `rustc [..]-W unsafe-code -W unused` [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s diff --git a/tests/testsuite/freshness.rs b/tests/testsuite/freshness.rs index 7e5a68a9407..d2a6135e8a5 100644 --- a/tests/testsuite/freshness.rs +++ b/tests/testsuite/freshness.rs @@ -1337,7 +1337,6 @@ fn changing_rustflags_is_cached() { p.cargo("build -v") .env("RUSTFLAGS", "-C linker=cc") .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the rustflags changed [COMPILING] foo v0.0.1 ([ROOT]/foo) [RUNNING] `rustc [..] [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -1347,9 +1346,7 @@ fn changing_rustflags_is_cached() { p.cargo("build -v") .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the rustflags changed -[COMPILING] foo v0.0.1 ([ROOT]/foo) -[RUNNING] `rustc [..] src/lib.rs [..] +[FRESH] foo v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) @@ -1357,9 +1354,7 @@ fn changing_rustflags_is_cached() { p.cargo("build -v") .env("RUSTFLAGS", "-C linker=cc") .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the rustflags changed -[COMPILING] foo v0.0.1 ([ROOT]/foo) -[RUNNING] `rustc [..] +[FRESH] foo v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) @@ -1380,7 +1375,6 @@ fn changing_rustc_extra_flags_is_cached() { .run(); p.cargo("rustc -v -- -C linker=cc") .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed [COMPILING] foo v0.0.1 ([ROOT]/foo) [RUNNING] `rustc [..] [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -1390,18 +1384,14 @@ fn changing_rustc_extra_flags_is_cached() { p.cargo("rustc -v") .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed -[COMPILING] foo v0.0.1 ([ROOT]/foo) -[RUNNING] `rustc [..] +[FRESH] foo v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) .run(); p.cargo("rustc -v -- -C linker=cc") .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed -[COMPILING] foo v0.0.1 ([ROOT]/foo) -[RUNNING] `rustc [..] +[FRESH] foo v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) diff --git a/tests/testsuite/freshness_checksum.rs b/tests/testsuite/freshness_checksum.rs index c2f8b65b139..5c4b88fdbc3 100644 --- a/tests/testsuite/freshness_checksum.rs +++ b/tests/testsuite/freshness_checksum.rs @@ -1509,7 +1509,6 @@ fn changing_rustflags_is_cached() { .masquerade_as_nightly_cargo(&["checksum-freshness"]) .env("RUSTFLAGS", "-C linker=cc") .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the rustflags changed [COMPILING] foo v0.0.1 ([ROOT]/foo) [RUNNING] `rustc [..] [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -1520,9 +1519,7 @@ fn changing_rustflags_is_cached() { p.cargo("build -Zchecksum-freshness -v") .masquerade_as_nightly_cargo(&["checksum-freshness"]) .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the rustflags changed -[COMPILING] foo v0.0.1 ([ROOT]/foo) -[RUNNING] `rustc [..] src/lib.rs [..] +[FRESH] foo v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) @@ -1531,9 +1528,7 @@ fn changing_rustflags_is_cached() { .masquerade_as_nightly_cargo(&["checksum-freshness"]) .env("RUSTFLAGS", "-C linker=cc") .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the rustflags changed -[COMPILING] foo v0.0.1 ([ROOT]/foo) -[RUNNING] `rustc [..] +[FRESH] foo v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) @@ -1556,7 +1551,6 @@ fn changing_rustc_extra_flags_is_cached() { p.cargo("rustc -Zchecksum-freshness -v -- -C linker=cc") .masquerade_as_nightly_cargo(&["checksum-freshness"]) .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed [COMPILING] foo v0.0.1 ([ROOT]/foo) [RUNNING] `rustc [..] [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s @@ -1567,9 +1561,7 @@ fn changing_rustc_extra_flags_is_cached() { p.cargo("rustc -Zchecksum-freshness -v") .masquerade_as_nightly_cargo(&["checksum-freshness"]) .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed -[COMPILING] foo v0.0.1 ([ROOT]/foo) -[RUNNING] `rustc [..] src/lib.rs [..] +[FRESH] foo v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) @@ -1577,9 +1569,7 @@ fn changing_rustc_extra_flags_is_cached() { p.cargo("rustc -Zchecksum-freshness -v -- -C linker=cc") .masquerade_as_nightly_cargo(&["checksum-freshness"]) .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.1 ([ROOT]/foo): the profile configuration changed -[COMPILING] foo v0.0.1 ([ROOT]/foo) -[RUNNING] `rustc [..] +[FRESH] foo v0.0.1 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) diff --git a/tests/testsuite/pgo.rs b/tests/testsuite/pgo.rs index 32748af1732..8676e73f55f 100644 --- a/tests/testsuite/pgo.rs +++ b/tests/testsuite/pgo.rs @@ -103,7 +103,6 @@ fn pgo_works() { ), ) .with_stderr_data(str![[r#" -[DIRTY] foo v0.0.0 ([ROOT]/foo): the rustflags changed [COMPILING] foo v0.0.0 ([ROOT]/foo) [RUNNING] `rustc [..]-Cprofile-use=[ROOT]/foo/target/merged.profdata -Cllvm-args=-pgo-warn-missing-function` [FINISHED] `release` profile [optimized] target(s) in [ELAPSED]s diff --git a/tests/testsuite/rustc.rs b/tests/testsuite/rustc.rs index 599438bc09e..2041c006706 100644 --- a/tests/testsuite/rustc.rs +++ b/tests/testsuite/rustc.rs @@ -616,7 +616,6 @@ fn rustc_fingerprint() { p.cargo("rustc -v") .with_stderr_does_not_contain("-C debug-assertions") .with_stderr_data(str![[r#" -[DIRTY] foo v0.5.0 ([ROOT]/foo): the profile configuration changed [COMPILING] foo v0.5.0 ([ROOT]/foo) [RUNNING] `rustc --crate-name foo [..]` [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s diff --git a/tests/testsuite/rustflags.rs b/tests/testsuite/rustflags.rs index 195d7a07fea..9a24371a30b 100644 --- a/tests/testsuite/rustflags.rs +++ b/tests/testsuite/rustflags.rs @@ -1592,7 +1592,7 @@ fn rustflags_remap_path_prefix_ignored_for_c_extra_filename() { .run(); let second_c_extra_filename = dbg!(get_c_extra_filename(build_output)); - assert_data_eq!(first_c_extra_filename, second_c_extra_filename); + assert_ne!(first_c_extra_filename, second_c_extra_filename); } // `--remap-path-prefix` is meant to take two different binaries and make them the same but the @@ -1613,7 +1613,7 @@ fn rustc_remap_path_prefix_ignored_for_c_extra_filename() { .run(); let second_c_extra_filename = dbg!(get_c_extra_filename(build_output)); - assert_data_eq!(first_c_extra_filename, second_c_extra_filename); + assert_ne!(first_c_extra_filename, second_c_extra_filename); } fn get_c_metadata(output: RawOutput) -> String { From 29a267a14388b53909eb9c482043242f311da1ea Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 5 Dec 2024 16:30:09 -0600 Subject: [PATCH 9/9] fix(fingerprint): Hackily keep --remap-path-prefix binaries reproducible --- .../build_runner/compilation_files.rs | 31 +++++++++++++++---- src/cargo/core/compiler/fingerprint/mod.rs | 7 +++-- tests/testsuite/rustflags.rs | 4 +-- 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/src/cargo/core/compiler/build_runner/compilation_files.rs b/src/cargo/core/compiler/build_runner/compilation_files.rs index 363529ac442..80100f322ed 100644 --- a/src/cargo/core/compiler/build_runner/compilation_files.rs +++ b/src/cargo/core/compiler/build_runner/compilation_files.rs @@ -703,14 +703,19 @@ fn compute_metadata( // Avoid trashing the caches on RUSTFLAGS changing via `c_extra_filename` // // Limited to `c_extra_filename` to help with reproducible build / PGO issues. - build_runner - .bcx - .extra_args_for(unit) - .hash(&mut c_extra_filename_hasher); + let default = Vec::new(); + let extra_args = build_runner.bcx.extra_args_for(unit).unwrap_or(&default); + if !has_remap_path_prefix(&extra_args) { + extra_args.hash(&mut c_extra_filename_hasher); + } if unit.mode.is_doc() || unit.mode.is_doc_scrape() { - unit.rustdocflags.hash(&mut c_extra_filename_hasher); + if !has_remap_path_prefix(&unit.rustdocflags) { + unit.rustdocflags.hash(&mut c_extra_filename_hasher); + } } else { - unit.rustflags.hash(&mut c_extra_filename_hasher); + if !has_remap_path_prefix(&unit.rustflags) { + unit.rustflags.hash(&mut c_extra_filename_hasher); + } } let c_metadata = UnitHash(c_metadata_hasher.finish()); @@ -726,6 +731,20 @@ fn compute_metadata( } } +/// HACK: Detect the *potential* presence of `--remap-path-prefix` +/// +/// As CLI parsing is contextual and dependent on the CLI definition to understand the context, we +/// can't say for sure whether `--remap-path-prefix` is present, so we guess if anything looks like +/// it. +/// If we could, we'd strip it out for hashing. +/// Instead, we use this to avoid hashing rustflags if it might be present to avoid the risk of taking +/// a flag that is trying to make things reproducible and making things less reproducible by the +/// `-Cextra-filename` showing up in the rlib, even with `split-debuginfo`. +fn has_remap_path_prefix(args: &[String]) -> bool { + args.iter() + .any(|s| s.starts_with("--remap-path-prefix=") || s == "--remap-path-prefix") +} + /// Hash the version of rustc being used during the build process. fn hash_rustc_version(bcx: &BuildContext<'_, '_>, hasher: &mut StableHasher, unit: &Unit) { let vers = &bcx.rustc().version; diff --git a/src/cargo/core/compiler/fingerprint/mod.rs b/src/cargo/core/compiler/fingerprint/mod.rs index c92dedccdd8..67e4aaab438 100644 --- a/src/cargo/core/compiler/fingerprint/mod.rs +++ b/src/cargo/core/compiler/fingerprint/mod.rs @@ -68,7 +68,7 @@ //! -------------------------------------------|-------------|---------------------|------------------------|---------- //! rustc | ✓ | ✓ | ✓ | ✓ //! [`Profile`] | ✓ | ✓ | ✓ | ✓ -//! `cargo rustc` extra args | ✓ | ✓ | | ✓ +//! `cargo rustc` extra args | ✓ | ✓[^7] | | ✓[^7] //! [`CompileMode`] | ✓ | ✓ | ✓ | ✓ //! Target Name | ✓ | ✓ | ✓ | ✓ //! `TargetKind` (bin/lib/etc.) | ✓ | ✓ | ✓ | ✓ @@ -83,7 +83,7 @@ //! Target flags (test/bench/for_host/edition) | ✓ | | | //! -C incremental=… flag | ✓ | | | //! mtime of sources | ✓[^3] | | | -//! RUSTFLAGS/RUSTDOCFLAGS | ✓ | ✓ | | ✓ +//! RUSTFLAGS/RUSTDOCFLAGS | ✓ | ✓[^7] | | ✓[^7] //! [`Lto`] flags | ✓ | ✓ | ✓ | ✓ //! config settings[^5] | ✓ | | | //! `is_std` | | ✓ | ✓ | ✓ @@ -102,6 +102,9 @@ //! //! [^6]: Via [`Manifest::lint_rustflags`][crate::core::Manifest::lint_rustflags] //! +//! [^7]: extra-flags and RUSTFLAGS are conditionally excluded when `--remap-path-prefix` is +//! present to avoid breaking build reproducibility while we wait for trim-paths +//! //! When deciding what should go in the Metadata vs the Fingerprint, consider //! that some files (like dylibs) do not have a hash in their filename. Thus, //! if a value changes, only the fingerprint will detect the change (consider, diff --git a/tests/testsuite/rustflags.rs b/tests/testsuite/rustflags.rs index 9a24371a30b..195d7a07fea 100644 --- a/tests/testsuite/rustflags.rs +++ b/tests/testsuite/rustflags.rs @@ -1592,7 +1592,7 @@ fn rustflags_remap_path_prefix_ignored_for_c_extra_filename() { .run(); let second_c_extra_filename = dbg!(get_c_extra_filename(build_output)); - assert_ne!(first_c_extra_filename, second_c_extra_filename); + assert_data_eq!(first_c_extra_filename, second_c_extra_filename); } // `--remap-path-prefix` is meant to take two different binaries and make them the same but the @@ -1613,7 +1613,7 @@ fn rustc_remap_path_prefix_ignored_for_c_extra_filename() { .run(); let second_c_extra_filename = dbg!(get_c_extra_filename(build_output)); - assert_ne!(first_c_extra_filename, second_c_extra_filename); + assert_data_eq!(first_c_extra_filename, second_c_extra_filename); } fn get_c_metadata(output: RawOutput) -> String {