fix(fingerprint): Hackily keep --remap-path-prefix binaries reproducible

rust-lang · Dec 5, 2024 · 29a267a · 29a267a
1 parent 306d515
commit 29a267a
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 10 deletions.
diff --git 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
@@ -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
@@ -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 {