diff --git a/src/cargo/core/source/source_id.rs b/src/cargo/core/source/source_id.rs index bdd4e67a98f..4299722239d 100644 --- a/src/cargo/core/source/source_id.rs +++ b/src/cargo/core/source/source_id.rs @@ -42,12 +42,11 @@ struct SourceIdInner { /// The possible kinds of code source. Along with `SourceIdInner`, this fully defines the /// source. -#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] enum SourceKind { - // Note that the ordering here is important for how it affects the `Ord` - // implementation, notably how this affects the ordering of serialized - // packages into lock files. - /// A local path.. + /// A git repository. + Git(GitReference), + /// A local path. Path, /// A remote registry. Registry, @@ -55,8 +54,6 @@ enum SourceKind { LocalRegistry, /// A directory-based registry. Directory, - /// A git repository. - Git(GitReference), } /// Information to find a specific commit in a Git repository. @@ -486,6 +483,110 @@ impl Hash for SourceId { } } +// forward to `Ord` +impl PartialOrd for SourceKind { + fn partial_cmp(&self, other: &SourceKind) -> Option { + Some(self.cmp(other)) + } +} + +// 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 wierd 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. +impl Ord for SourceKind { + fn cmp(&self, other: &SourceKind) -> Ordering { + match (self, other) { + (SourceKind::Path, SourceKind::Path) => Ordering::Equal, + (SourceKind::Path, _) => Ordering::Less, + (_, SourceKind::Path) => Ordering::Greater, + + (SourceKind::Registry, SourceKind::Registry) => Ordering::Equal, + (SourceKind::Registry, _) => Ordering::Less, + (_, SourceKind::Registry) => Ordering::Greater, + + (SourceKind::LocalRegistry, SourceKind::LocalRegistry) => Ordering::Equal, + (SourceKind::LocalRegistry, _) => Ordering::Less, + (_, SourceKind::LocalRegistry) => Ordering::Greater, + + (SourceKind::Directory, SourceKind::Directory) => Ordering::Equal, + (SourceKind::Directory, _) => Ordering::Less, + (_, SourceKind::Directory) => Ordering::Greater, + + (SourceKind::Git(a), SourceKind::Git(b)) => a.cmp(b), + } + } +} + +// This is a test that the hash of the `SourceId` for crates.io is a well-known +// 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 +// 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. +// +// This isn't to say that this hash can *never* change, only that when changing +// this it should be explicitly done. If this hash changes accidentally and +// 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. +#[test] +fn test_cratesio_hash() { + let config = Config::default().unwrap(); + let crates_io = SourceId::crates_io(&config).unwrap(); + assert_eq!(crate::util::hex::short_hash(&crates_io), "1ecc6299db9ec823"); +} + /// A `Display`able view into a `SourceId` that will write it as a url pub struct SourceIdAsUrl<'a> { inner: &'a SourceIdInner,