From 7a1b3d3726b7a08630fd0afcdce9fc43df757dfd Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 5 Dec 2024 16:30:09 -0600 Subject: [PATCH] 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 363529ac442c..80100f322ed2 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 c92dedccdd8b..67e4aaab438d 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 9a24371a30b4..195d7a07fea7 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 {