Skip to content

Commit

Permalink
shorten comment on Ord for SourceKind (#15029)
Browse files Browse the repository at this point in the history 
As discussed in today's cargo meeting, and in
https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/while.20we're.20breaking.20stable.20hash,
we are keeping the manual impl but shortening the comment about why it
is needed.
  • Loading branch information
@Eh2406
Eh2406 authored Jan 8, 2025
2 parents d9f65ec + 06811d8 commit 33b49c9
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 81 deletions.
2 changes: 1 addition & 1 deletion Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/cargo-util-schemas/Cargo.toml
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down
71 changes: 17 additions & 54 deletions crates/cargo-util-schemas/src/core/source_kind.rs
View file
Original file line number Diff line number Diff line change
@@ -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),
Expand All @@ -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<H: std::hash::Hasher>(&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 {
Expand All @@ -31,59 +44,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) {
Expand Down
60 changes: 35 additions & 25 deletions src/cargo/core/source_id.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand All @@ -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
Expand All @@ -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))]
Expand All @@ -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());
}
}

Expand Down

0 comments on commit 33b49c9

Please sign in to comment.