From d8b7c072c5d471ec55a5efe0b8122dc23af8d27d Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 7 Jan 2025 22:11:59 +0000 Subject: [PATCH] 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 f79a3d70f..831e1840f 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 7821ea953..8a3152ff1 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 7b2ecaeec..a67c80aee 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) {