From 4d1bf29a2c22a86abc78a7b3630d246fa67332c4 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Mon, 9 Oct 2023 10:34:44 -0700 Subject: [PATCH 1/4] Add test of update with version that differ only in build metadata --- tests/testsuite/update.rs | 42 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index fe1d86bd7..63da173ae 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -391,6 +391,48 @@ fn update_precise() { .run(); } +#[cargo_test] +fn update_precise_build_metadata() { + Package::new("serde", "0.0.1+first").publish(); + Package::new("serde", "0.0.1+second").publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.0" + + [dependencies] + serde = "0.0.1" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile").run(); + p.cargo("update serde --precise 0.0.1+first").run(); + + p.cargo("update serde --precise 0.0.1+second") + .with_stderr( + "\ +[UPDATING] `[..]` index +[UPDATING] serde v0.0.1+first -> v0.0.1+second +", + ) + .run(); + + p.cargo("update serde --precise 0.0.1+first") + .with_stderr( + "\ +[UPDATING] `[..]` index +[DOWNGRADING] serde v0.0.1+second -> v0.0.1+first +", + ) + .run(); +} + #[cargo_test] fn update_precise_do_not_force_update_deps() { Package::new("log", "0.1.0").publish(); From 985e1eeef576b0b3db814f4fa25a1b16663b9428 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Mon, 9 Oct 2023 10:40:43 -0700 Subject: [PATCH 2/4] Do not call it "Downgrading" when difference is only build metadata --- src/cargo/ops/cargo_generate_lockfile.rs | 10 +++++++++- tests/testsuite/update.rs | 4 +++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index 832261a41..192cf50fc 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -152,7 +152,15 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes format!("{} -> v{}", removed[0], added[0].version()) }; - if removed[0].version() > added[0].version() { + // If versions differ only in build metadata, we call it an "update" + // regardless of whether the build metadata has gone up or down. + // This metadata is often stuff like git commit hashes, which are + // not meaningfully ordered. + let removed = removed[0].version(); + let added = added[0].version(); + if (removed.major, removed.minor, removed.patch, &removed.pre) + > (added.major, added.minor, added.patch, &added.pre) + { print_change("Downgrading", msg, &style::WARN)?; } else { print_change("Updating", msg, &style::GOOD)?; diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index 63da173ae..8a58e4c80 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -423,11 +423,13 @@ fn update_precise_build_metadata() { ) .run(); + // This is not considered "Downgrading". Build metadata are not assumed to + // be ordered. p.cargo("update serde --precise 0.0.1+first") .with_stderr( "\ [UPDATING] `[..]` index -[DOWNGRADING] serde v0.0.1+second -> v0.0.1+first +[UPDATING] serde v0.0.1+second -> v0.0.1+first ", ) .run(); From c0ed70ef5ee2e2b1c251e54710618df31c66f54a Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Mon, 9 Oct 2023 15:19:38 -0700 Subject: [PATCH 3/4] Use semver::Version's cmp_precedence for deciding what is downgrade --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- src/cargo/ops/cargo_generate_lockfile.rs | 7 ++----- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8affbedbc..b96ea193a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2886,9 +2886,9 @@ dependencies = [ [[package]] name = "semver" -version = "1.0.19" +version = "1.0.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ad977052201c6de01a8ef2aa3378c4bd23217a056337d1d6da40468d267a4fb0" +checksum = "836fa6a3e1e547f9a2c4040802ec865b5d85f4014efe00555d7090a3dcaa1090" dependencies = [ "serde", ] diff --git a/Cargo.toml b/Cargo.toml index 1af7cce75..7117302ac 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,7 +76,7 @@ rand = "0.8.5" rustfix = "0.6.1" same-file = "1.0.6" security-framework = "2.9.2" -semver = { version = "1.0.19", features = ["serde"] } +semver = { version = "1.0.20", features = ["serde"] } serde = "1.0.188" serde-untagged = "0.1.1" serde-value = "0.7.0" diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index 192cf50fc..cbacfd8a9 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -8,6 +8,7 @@ use crate::util::config::Config; use crate::util::style; use crate::util::CargoResult; use anstyle::Style; +use std::cmp::Ordering; use std::collections::{BTreeMap, HashSet}; use tracing::debug; @@ -156,11 +157,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes // regardless of whether the build metadata has gone up or down. // This metadata is often stuff like git commit hashes, which are // not meaningfully ordered. - let removed = removed[0].version(); - let added = added[0].version(); - if (removed.major, removed.minor, removed.patch, &removed.pre) - > (added.major, added.minor, added.patch, &added.pre) - { + if removed[0].version().cmp_precedence(added[0].version()) == Ordering::Greater { print_change("Downgrading", msg, &style::WARN)?; } else { print_change("Updating", msg, &style::GOOD)?; From f1d2237b7242dba1a7716ac3586ea0e89461dc4d Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Tue, 10 Oct 2023 20:45:25 -0700 Subject: [PATCH 4/4] Call cmp_precedence where applicable --- src/cargo/core/package_id.rs | 35 +++++++++++++++++++---------- src/cargo/sources/registry/index.rs | 8 +++---- src/cargo/util/semver_ext.rs | 8 ++----- 3 files changed, 28 insertions(+), 23 deletions(-) diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs index ca126172c..c97e12155 100644 --- a/src/cargo/core/package_id.rs +++ b/src/cargo/core/package_id.rs @@ -29,8 +29,7 @@ struct PackageIdInner { source_id: SourceId, } -// Custom equality that uses full equality of SourceId, rather than its custom equality, -// and Version, which usually ignores `build` metadata. +// Custom equality that uses full equality of SourceId, rather than its custom equality. // // The `build` part of the version is usually ignored (like a "comment"). // However, there are some cases where it is important. The download path from @@ -40,11 +39,7 @@ struct PackageIdInner { impl PartialEq for PackageIdInner { fn eq(&self, other: &Self) -> bool { self.name == other.name - && self.version.major == other.version.major - && self.version.minor == other.version.minor - && self.version.patch == other.version.patch - && self.version.pre == other.version.pre - && self.version.build == other.version.build + && self.version == other.version && self.source_id.full_eq(other.source_id) } } @@ -53,11 +48,7 @@ impl PartialEq for PackageIdInner { impl Hash for PackageIdInner { fn hash(&self, into: &mut S) { self.name.hash(into); - self.version.major.hash(into); - self.version.minor.hash(into); - self.version.patch.hash(into); - self.version.pre.hash(into); - self.version.build.hash(into); + self.version.hash(into); self.source_id.full_hash(into); } } @@ -233,6 +224,16 @@ impl fmt::Debug for PackageId { } } +impl fmt::Debug for PackageIdInner { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + f.debug_struct("PackageIdInner") + .field("name", &self.name) + .field("version", &self.version.to_string()) + .field("source", &self.source_id.to_string()) + .finish() + } +} + #[cfg(test)] mod tests { use super::PackageId; @@ -257,4 +258,14 @@ mod tests { let pkg_id = PackageId::new("foo", "1.0.0", SourceId::for_registry(&loc).unwrap()).unwrap(); assert_eq!("foo v1.0.0", pkg_id.to_string()); } + + #[test] + fn unequal_build_metadata() { + let loc = CRATES_IO_INDEX.into_url().unwrap(); + let repo = SourceId::for_registry(&loc).unwrap(); + let first = PackageId::new("foo", "0.0.1+first", repo).unwrap(); + let second = PackageId::new("foo", "0.0.1+second", repo).unwrap(); + assert_ne!(first, second); + assert_ne!(first.inner, second.inner); + } } diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index 7a9ba0e15..617f0b09c 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -98,6 +98,7 @@ use cargo_util::{paths, registry::make_dep_path}; use semver::Version; use serde::Deserialize; use std::borrow::Cow; +use std::cmp::Ordering; use std::collections::BTreeMap; use std::collections::{HashMap, HashSet}; use std::fs; @@ -674,11 +675,8 @@ impl<'cfg> RegistryIndex<'cfg> { (true, true) => s_vers == requested, (true, false) => false, (false, true) => { - // Strip out the metadata. - s_vers.major == requested.major - && s_vers.minor == requested.minor - && s_vers.patch == requested.patch - && s_vers.pre == requested.pre + // Compare disregarding the metadata. + s_vers.cmp_precedence(requested) == Ordering::Equal } (false, false) => s_vers == requested, } diff --git a/src/cargo/util/semver_ext.rs b/src/cargo/util/semver_ext.rs index bee3b2da3..0be5bca68 100644 --- a/src/cargo/util/semver_ext.rs +++ b/src/cargo/util/semver_ext.rs @@ -1,5 +1,6 @@ use semver::{Comparator, Op, Version, VersionReq}; use serde_untagged::UntaggedEnumVisitor; +use std::cmp::Ordering; use std::fmt::{self, Display}; #[derive(PartialEq, Eq, Hash, Clone, Debug)] @@ -83,12 +84,7 @@ impl OptVersionReq { match self { OptVersionReq::Any => true, OptVersionReq::Req(req) => req.matches(version), - OptVersionReq::Locked(v, _) => { - v.major == version.major - && v.minor == version.minor - && v.patch == version.patch - && v.pre == version.pre - } + OptVersionReq::Locked(v, _) => v.cmp_precedence(version) == Ordering::Equal, } } }