From dd59760f8ebb8c81358f746642728cff9f257082 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 7 Jan 2025 21:57:01 +0000 Subject: [PATCH 1/3] test(source_id): make hash snapshot aware --- src/cargo/core/source_id.rs | 60 +++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/src/cargo/core/source_id.rs b/src/cargo/core/source_id.rs index d3578d98b9a..0d3a817d4e4 100644 --- a/src/cargo/core/source_id.rs +++ b/src/cargo/core/source_id.rs @@ -762,7 +762,7 @@ mod tests { // value. // // Note that the hash value matches what the crates.io source id has hashed - // since long before Rust 1.30. We strive to keep this value the same across + // since Rust 1.84.0. We strive to keep this value the same across // versions of Cargo because changing it means that users will need to // redownload the index and all crates they use when using a new Cargo version. // @@ -771,6 +771,11 @@ mod tests { // you're able to restore the hash to its original value, please do so! // Otherwise please just leave a comment in your PR as to why the hash value is // changing and why the old value can't be easily preserved. + // If it takes an ugly hack to restore it, + // then leave a link here so we can remove the hack next time we change the hash. + // + // Hacks to remove next time the hash changes: + // - (fill in your code here) // // The hash value should be stable across platforms, and doesn't depend on // endianness and bit-width. One caveat is that absolute paths on Windows @@ -782,6 +787,11 @@ mod tests { use std::hash::Hasher; use std::path::Path; + use snapbox::assert_data_eq; + use snapbox::str; + use snapbox::IntoData as _; + + use crate::util::hex::short_hash; use crate::util::StableHasher; #[cfg(not(windows))] @@ -792,68 +802,68 @@ mod tests { let gen_hash = |source_id: SourceId| { let mut hasher = StableHasher::new(); source_id.stable_hash(ws_root, &mut hasher); - Hasher::finish(&hasher) + Hasher::finish(&hasher).to_string() }; let source_id = SourceId::crates_io(&GlobalContext::default().unwrap()).unwrap(); - assert_eq!(gen_hash(source_id), 7062945687441624357); - assert_eq!(crate::util::hex::short_hash(&source_id), "25cdd57fae9f0462"); + assert_data_eq!(gen_hash(source_id), str!["7062945687441624357"].raw()); + assert_data_eq!(short_hash(&source_id), str!["25cdd57fae9f0462"].raw()); let url = "https://my-crates.io".into_url().unwrap(); let source_id = SourceId::for_registry(&url).unwrap(); - assert_eq!(gen_hash(source_id), 8310250053664888498); - assert_eq!(crate::util::hex::short_hash(&source_id), "b2d65deb64f05373"); + assert_data_eq!(gen_hash(source_id), str!["8310250053664888498"].raw()); + assert_data_eq!(short_hash(&source_id), str!["b2d65deb64f05373"].raw()); let url = "https://your-crates.io".into_url().unwrap(); let source_id = SourceId::for_alt_registry(&url, "alt").unwrap(); - assert_eq!(gen_hash(source_id), 14149534903000258933); - assert_eq!(crate::util::hex::short_hash(&source_id), "755952de063f5dc4"); + assert_data_eq!(gen_hash(source_id), str!["14149534903000258933"].raw()); + assert_data_eq!(short_hash(&source_id), str!["755952de063f5dc4"].raw()); let url = "sparse+https://my-crates.io".into_url().unwrap(); let source_id = SourceId::for_registry(&url).unwrap(); - assert_eq!(gen_hash(source_id), 16249512552851930162); - assert_eq!(crate::util::hex::short_hash(&source_id), "327cfdbd92dd81e1"); + assert_data_eq!(gen_hash(source_id), str!["16249512552851930162"].raw()); + assert_data_eq!(short_hash(&source_id), str!["327cfdbd92dd81e1"].raw()); let url = "sparse+https://your-crates.io".into_url().unwrap(); let source_id = SourceId::for_alt_registry(&url, "alt").unwrap(); - assert_eq!(gen_hash(source_id), 6156697384053352292); - assert_eq!(crate::util::hex::short_hash(&source_id), "64a713b6a6fb7055"); + assert_data_eq!(gen_hash(source_id), str!["6156697384053352292"].raw()); + assert_data_eq!(short_hash(&source_id), str!["64a713b6a6fb7055"].raw()); let url = "file:///tmp/ws/crate".into_url().unwrap(); let source_id = SourceId::for_git(&url, GitReference::DefaultBranch).unwrap(); - assert_eq!(gen_hash(source_id), 473480029881867801); - assert_eq!(crate::util::hex::short_hash(&source_id), "199e591d94239206"); + assert_data_eq!(gen_hash(source_id), str!["473480029881867801"].raw()); + assert_data_eq!(short_hash(&source_id), str!["199e591d94239206"].raw()); let path = &ws_root.join("crate"); let source_id = SourceId::for_local_registry(path).unwrap(); #[cfg(not(windows))] { - assert_eq!(gen_hash(source_id), 11515846423845066584); - assert_eq!(crate::util::hex::short_hash(&source_id), "58d73c154f81d09f"); + assert_data_eq!(gen_hash(source_id), str!["11515846423845066584"].raw()); + assert_data_eq!(short_hash(&source_id), str!["58d73c154f81d09f"].raw()); } #[cfg(windows)] { - assert_eq!(gen_hash(source_id), 6146331155906064276); - assert_eq!(crate::util::hex::short_hash(&source_id), "946fb2239f274c55"); + assert_data_eq!(gen_hash(source_id), str!["6146331155906064276"].raw()); + assert_data_eq!(short_hash(&source_id), str!["946fb2239f274c55"].raw()); } let source_id = SourceId::for_path(path).unwrap(); - assert_eq!(gen_hash(source_id), 215644081443634269); + assert_data_eq!(gen_hash(source_id), str!["215644081443634269"].raw()); #[cfg(not(windows))] - assert_eq!(crate::util::hex::short_hash(&source_id), "64bace89c92b101f"); + assert_data_eq!(short_hash(&source_id), str!["64bace89c92b101f"].raw()); #[cfg(windows)] - assert_eq!(crate::util::hex::short_hash(&source_id), "01e1e6c391813fb6"); + assert_data_eq!(short_hash(&source_id), str!["01e1e6c391813fb6"].raw()); let source_id = SourceId::for_directory(path).unwrap(); #[cfg(not(windows))] { - assert_eq!(gen_hash(source_id), 6127590343904940368); - assert_eq!(crate::util::hex::short_hash(&source_id), "505191d1f3920955"); + assert_data_eq!(gen_hash(source_id), str!["6127590343904940368"].raw()); + assert_data_eq!(short_hash(&source_id), str!["505191d1f3920955"].raw()); } #[cfg(windows)] { - assert_eq!(gen_hash(source_id), 10423446877655960172); - assert_eq!(crate::util::hex::short_hash(&source_id), "6c8ad69db585a790"); + assert_data_eq!(gen_hash(source_id), str!["10423446877655960172"].raw()); + assert_data_eq!(short_hash(&source_id), str!["6c8ad69db585a790"].raw()); } } From d8b7c072c5d471ec55a5efe0b8122dc23af8d27d Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 7 Jan 2025 22:11:59 +0000 Subject: [PATCH 2/3] shortening the comment --- Cargo.lock | 2 +- crates/cargo-util-schemas/Cargo.toml | 2 +- .../src/core/source_kind.rs | 56 +------------------ 3 files changed, 5 insertions(+), 55 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f79a3d70f26..831e1840f5f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -505,7 +505,7 @@ dependencies = [ [[package]] name = "cargo-util-schemas" -version = "0.7.2" +version = "0.7.3" dependencies = [ "schemars", "semver", diff --git a/crates/cargo-util-schemas/Cargo.toml b/crates/cargo-util-schemas/Cargo.toml index 7821ea9537e..8a3152ff190 100644 --- a/crates/cargo-util-schemas/Cargo.toml +++ b/crates/cargo-util-schemas/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cargo-util-schemas" -version = "0.7.2" +version = "0.7.3" rust-version = "1.83" # MSRV:1 edition.workspace = true license.workspace = true diff --git a/crates/cargo-util-schemas/src/core/source_kind.rs b/crates/cargo-util-schemas/src/core/source_kind.rs index 7b2ecaeec8c..a67c80aee3e 100644 --- a/crates/cargo-util-schemas/src/core/source_kind.rs +++ b/crates/cargo-util-schemas/src/core/source_kind.rs @@ -31,59 +31,9 @@ impl SourceKind { } } -/// Note that this is specifically not derived on `SourceKind` although the -/// implementation here is very similar to what it might look like if it were -/// otherwise derived. -/// -/// The reason for this is somewhat obtuse. First of all the hash value of -/// `SourceKind` makes its way into `~/.cargo/registry/index/github.com-XXXX` -/// which means that changes to the hash means that all Rust users need to -/// redownload the crates.io index and all their crates. If possible we strive -/// to not change this to make this redownloading behavior happen as little as -/// possible. How is this connected to `Ord` you might ask? That's a good -/// question! -/// -/// Since the beginning of time `SourceKind` has had `#[derive(Hash)]`. It for -/// the longest time *also* derived the `Ord` and `PartialOrd` traits. In #8522, -/// however, the implementation of `Ord` changed. This handwritten implementation -/// forgot to sync itself with the originally derived implementation, namely -/// placing git dependencies as sorted after all other dependencies instead of -/// first as before. -/// -/// This regression in #8522 (Rust 1.47) went unnoticed. When we switched back -/// to a derived implementation in #9133 (Rust 1.52 beta) we only then ironically -/// saw an issue (#9334). In #9334 it was observed that stable Rust at the time -/// (1.51) was sorting git dependencies last, whereas Rust 1.52 beta would sort -/// git dependencies first. This is because the `PartialOrd` implementation in -/// 1.51 used #8522, the buggy implementation, which put git deps last. In 1.52 -/// it was (unknowingly) restored to the pre-1.47 behavior with git dependencies -/// first. -/// -/// Because the breakage was only witnessed after the original breakage, this -/// trait implementation is preserving the "broken" behavior. Put a different way: -/// -/// * Rust pre-1.47 sorted git deps first. -/// * Rust 1.47 to Rust 1.51 sorted git deps last, a breaking change (#8522) that -/// was never noticed. -/// * Rust 1.52 restored the pre-1.47 behavior (#9133, without knowing it did -/// so), and breakage was witnessed by actual users due to difference with -/// 1.51. -/// * Rust 1.52 (the source as it lives now) was fixed to match the 1.47-1.51 -/// behavior (#9383), which is now considered intentionally breaking from the -/// pre-1.47 behavior. -/// -/// Note that this was all discovered when Rust 1.53 was in nightly and 1.52 was -/// in beta. #9133 was in both beta and nightly at the time of discovery. For -/// 1.52 #9383 reverted #9133, meaning 1.52 is the same as 1.51. On nightly -/// (1.53) #9397 was created to fix the regression introduced by #9133 relative -/// to the current stable (1.51). -/// -/// That's all a long winded way of saying "it's weird that git deps hash first -/// and are sorted last, but it's the way it is right now". The author of this -/// comment chose to handwrite the `Ord` implementation instead of the `Hash` -/// implementation, but it's only required that at most one of them is -/// hand-written because the other can be derived. Perhaps one day in -/// the future someone can figure out how to remove this behavior. +// The ordering here is important for how packages are serialized into lock files. +// We implement it manually to callout the stability guarantee. +// See https://github.com/rust-lang/cargo/pull/9397 for the history. impl Ord for SourceKind { fn cmp(&self, other: &SourceKind) -> Ordering { match (self, other) { From 06811d8594826611f19dc6b97404ae8cedafb167 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 7 Jan 2025 22:12:22 +0000 Subject: [PATCH 3/3] manual impl of hash --- crates/cargo-util-schemas/src/core/source_kind.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/crates/cargo-util-schemas/src/core/source_kind.rs b/crates/cargo-util-schemas/src/core/source_kind.rs index a67c80aee3e..3794791114d 100644 --- a/crates/cargo-util-schemas/src/core/source_kind.rs +++ b/crates/cargo-util-schemas/src/core/source_kind.rs @@ -1,7 +1,7 @@ use std::cmp::Ordering; /// The possible kinds of code source. -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, Eq)] pub enum SourceKind { /// A git repository. Git(GitReference), @@ -17,6 +17,19 @@ pub enum SourceKind { Directory, } +// The hash here is important for what folder packages get downloaded into. +// Changes trigger all users to download another copy of their crates. +// So the `stable_hash` test checks that we only change it intentionally. +// We implement hash manually to callout the stability impact. +impl std::hash::Hash for SourceKind { + fn hash(&self, state: &mut H) { + core::mem::discriminant(self).hash(state); + if let SourceKind::Git(git) = self { + git.hash(state); + } + } +} + impl SourceKind { pub fn protocol(&self) -> Option<&str> { match self {