From 024bf5e48727d338a5b8e7ffdbc9bf9b87c79c9c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 27 Aug 2024 13:31:25 -0500 Subject: [PATCH] fix(resolve): With `latest` message, differentiate actionable updates Instead of always listing the absolute latest version as a warning color, we now differentiate - compatible updates are always actionable - incompatible, direct deps are always actionable These get reported and made yellow while non-actionable messages are unstyled. This is not intended as *the* solution for #13908 though it makes improvements in that direction. This is prep work for improved MSRV reporting where we will differentiate this further by only considering MSRV-compatible updates as actionable (or rustc-compatible when not using MSRV-aware reslver). I just used a broad stroke to say "compatible" in the message means "semver compatible" and use `^` - We could focus on "compatible with dependent version reqs" which is what will be most actionable but seeing if we can get away without having to track all in-coming version reqs. - We could be more nuanced in language but the more verbose we are, the more we take away from higher priority messages --- src/cargo/ops/cargo_update.rs | 46 ++++++++++++++++++++-- src/cargo/util/semver_ext.rs | 14 +++++-- tests/testsuite/direct_minimal_versions.rs | 6 +-- tests/testsuite/minimal_versions.rs | 2 +- tests/testsuite/rust_version.rs | 16 ++++---- tests/testsuite/update.rs | 4 +- tests/testsuite/workspaces.rs | 2 +- 7 files changed, 69 insertions(+), 21 deletions(-) diff --git a/src/cargo/ops/cargo_update.rs b/src/cargo/ops/cargo_update.rs index 9628d449daca..a73a829babaf 100644 --- a/src/cargo/ops/cargo_update.rs +++ b/src/cargo/ops/cargo_update.rs @@ -756,14 +756,31 @@ fn report_latest(possibilities: &[IndexSummary], change: &PackageChange) -> Opti return None; } + let version_req = package_id.version().to_caret_req(); if let Some(version) = possibilities .iter() .map(|s| s.as_summary()) - .filter(|s| is_latest(s.version(), package_id.version())) + .filter(|s| package_id.version() != s.version() && version_req.matches(s.version())) .map(|s| s.version().clone()) .max() { let warn = style::WARN; + let report = format!(" {warn}(latest compatible: v{version}){warn:#}"); + return Some(report); + } + + if let Some(version) = possibilities + .iter() + .map(|s| s.as_summary()) + .filter(|s| is_latest(s.version(), package_id.version())) + .map(|s| s.version().clone()) + .max() + { + let warn = if change.is_transitive.unwrap_or(true) { + Default::default() + } else { + style::WARN + }; let report = format!(" {warn}(latest: v{version}){warn:#}"); return Some(report); } @@ -801,13 +818,14 @@ struct PackageChange { previous_id: Option, kind: PackageChangeKind, is_member: Option, + is_transitive: Option, required_rust_version: Option, } impl PackageChange { pub fn new(ws: &Workspace<'_>, resolve: &Resolve) -> IndexMap { let diff = PackageDiff::new(resolve); - Self::with_diff(diff, ws) + Self::with_diff(diff, ws, resolve) } pub fn diff( @@ -816,12 +834,13 @@ impl PackageChange { resolve: &Resolve, ) -> IndexMap { let diff = PackageDiff::diff(previous_resolve, resolve); - Self::with_diff(diff, ws) + Self::with_diff(diff, ws, resolve) } fn with_diff( diff: impl Iterator, ws: &Workspace<'_>, + resolve: &Resolve, ) -> IndexMap { let member_ids: HashSet<_> = ws.members().map(|p| p.package_id()).collect(); @@ -840,11 +859,13 @@ impl PackageChange { PackageChangeKind::Upgraded }; let is_member = Some(member_ids.contains(&package_id)); + let is_transitive = Some(true); let change = Self { package_id, previous_id: Some(previous_id), kind, is_member, + is_transitive, required_rust_version: None, }; changes.insert(change.package_id, change); @@ -852,11 +873,13 @@ impl PackageChange { for package_id in diff.removed { let kind = PackageChangeKind::Removed; let is_member = None; + let is_transitive = None; let change = Self { package_id, previous_id: None, kind, is_member, + is_transitive, required_rust_version: None, }; changes.insert(change.package_id, change); @@ -864,11 +887,13 @@ impl PackageChange { for package_id in diff.added { let kind = PackageChangeKind::Added; let is_member = Some(member_ids.contains(&package_id)); + let is_transitive = Some(true); let change = Self { package_id, previous_id: None, kind, is_member, + is_transitive, required_rust_version: None, }; changes.insert(change.package_id, change); @@ -877,17 +902,32 @@ impl PackageChange { for package_id in diff.unchanged { let kind = PackageChangeKind::Unchanged; let is_member = Some(member_ids.contains(&package_id)); + let is_transitive = Some(true); let change = Self { package_id, previous_id: None, kind, is_member, + is_transitive, required_rust_version: None, }; changes.insert(change.package_id, change); } } + for member_id in &member_ids { + let Some(change) = changes.get_mut(member_id) else { + continue; + }; + change.is_transitive = Some(false); + for (direct_dep_id, _) in resolve.deps(*member_id) { + let Some(change) = changes.get_mut(&direct_dep_id) else { + continue; + }; + change.is_transitive = Some(false); + } + } + changes } diff --git a/src/cargo/util/semver_ext.rs b/src/cargo/util/semver_ext.rs index 86bb4199a99f..1029f88aec37 100644 --- a/src/cargo/util/semver_ext.rs +++ b/src/cargo/util/semver_ext.rs @@ -5,7 +5,15 @@ use std::fmt::{self, Display}; pub trait VersionExt { fn is_prerelease(&self) -> bool; - fn to_exact_req(&self) -> VersionReq; + fn to_req(&self, op: Op) -> VersionReq; + + fn to_exact_req(&self) -> VersionReq { + self.to_req(Op::Exact) + } + + fn to_caret_req(&self) -> VersionReq { + self.to_req(Op::Caret) + } } impl VersionExt for Version { @@ -13,10 +21,10 @@ impl VersionExt for Version { !self.pre.is_empty() } - fn to_exact_req(&self) -> VersionReq { + fn to_req(&self, op: Op) -> VersionReq { VersionReq { comparators: vec![Comparator { - op: Op::Exact, + op, major: self.major, minor: Some(self.minor), patch: Some(self.patch), diff --git a/tests/testsuite/direct_minimal_versions.rs b/tests/testsuite/direct_minimal_versions.rs index f43fe911af06..fb750f654d9d 100644 --- a/tests/testsuite/direct_minimal_versions.rs +++ b/tests/testsuite/direct_minimal_versions.rs @@ -33,7 +33,7 @@ fn simple() { .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 1 package -[ADDING] dep v1.0.0 (latest: v1.1.0) +[ADDING] dep v1.0.0 (latest compatible: v1.1.0) "#]]) .run(); @@ -122,7 +122,7 @@ fn yanked() { .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 1 package -[ADDING] dep v1.1.0 (latest: v1.2.0) +[ADDING] dep v1.1.0 (latest compatible: v1.2.0) "#]]) .run(); @@ -176,7 +176,7 @@ fn indirect() { .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 2 packages -[ADDING] direct v1.0.0 (latest: v1.1.0) +[ADDING] direct v1.0.0 (latest compatible: v1.1.0) "#]]) .run(); diff --git a/tests/testsuite/minimal_versions.rs b/tests/testsuite/minimal_versions.rs index 70a57c6c4284..a7fea29026b1 100644 --- a/tests/testsuite/minimal_versions.rs +++ b/tests/testsuite/minimal_versions.rs @@ -35,7 +35,7 @@ fn minimal_version_cli() { .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 1 package to earliest compatible version -[ADDING] dep v1.0.0 (latest: v1.1.0) +[ADDING] dep v1.0.0 (latest compatible: v1.1.0) "#]]) .run(); diff --git a/tests/testsuite/rust_version.rs b/tests/testsuite/rust_version.rs index e0e92e5bef6f..1129a1928e8a 100644 --- a/tests/testsuite/rust_version.rs +++ b/tests/testsuite/rust_version.rs @@ -242,7 +242,7 @@ foo v0.0.1 ([ROOT]/foo) .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 2 packages to latest Rust 1.60.0 compatible versions -[ADDING] newer-and-older v1.5.0 (latest: v1.6.0) +[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0) [ADDING] only-newer v1.6.0 (requires Rust 1.65.0) "#]]) @@ -319,7 +319,7 @@ foo v0.0.1 ([ROOT]/foo) .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 2 packages to latest Rust 1.60.0 compatible versions -[ADDING] newer-and-older v1.5.0 (latest: v1.6.0) +[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0) [ADDING] only-newer v1.6.0 (requires Rust 1.2345) "#]]) @@ -490,7 +490,7 @@ higher v0.0.1 ([ROOT]/foo) .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 2 packages to latest Rust 1.50.0 compatible versions -[ADDING] newer-and-older v1.5.0 (latest: v1.6.0) +[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0) [ADDING] only-newer v1.6.0 (requires Rust 1.65.0) "#]]) @@ -619,7 +619,7 @@ fn resolve_edition2024() { .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 2 packages to latest Rust 1.60.0 compatible versions -[ADDING] newer-and-older v1.5.0 (latest: v1.6.0) +[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0) [ADDING] only-newer v1.6.0 (requires Rust 1.65.0) "#]]) @@ -723,7 +723,7 @@ fn resolve_v3() { .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 2 packages to latest Rust 1.60.0 compatible versions -[ADDING] newer-and-older v1.5.0 (latest: v1.6.0) +[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0) [ADDING] only-newer v1.6.0 (requires Rust 1.65.0) "#]]) @@ -871,7 +871,7 @@ fn update_msrv_resolve() { .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 1 package to latest Rust 1.60.0 compatible version -[ADDING] bar v1.5.0 (latest: v1.6.0) +[ADDING] bar v1.5.0 (latest compatible: v1.6.0) "#]]) .run(); @@ -932,7 +932,7 @@ fn update_precise_overrides_msrv_resolver() { .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 1 package to latest Rust 1.60.0 compatible version -[ADDING] bar v1.5.0 (latest: v1.6.0) +[ADDING] bar v1.5.0 (latest compatible: v1.6.0) "#]]) .run(); @@ -1019,7 +1019,7 @@ foo v0.0.1 ([ROOT]/foo) .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 2 packages to latest Rust 1.60.0 compatible versions -[ADDING] newer-and-older v1.5.0 (latest: v1.6.0) +[ADDING] newer-and-older v1.5.0 (latest compatible: v1.6.0) [ADDING] only-newer v1.6.0 (requires Rust 1.65.0) [DOWNLOADING] crates ... [DOWNLOADED] newer-and-older v1.5.0 (registry `dummy-registry`) diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index 499a4e73becb..1d2c937b5961 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -1534,7 +1534,7 @@ fn report_behind() { [UPDATING] `dummy-registry` index [LOCKING] 1 package to latest compatible version [UPDATING] breaking v0.1.0 -> v0.1.1 (latest: v0.2.0) -[UNCHANGED] pre v1.0.0-alpha.0 (latest: v1.0.0-alpha.1) +[UNCHANGED] pre v1.0.0-alpha.0 (latest compatible: v1.0.0-alpha.1) [UNCHANGED] two-ver v0.1.0 (latest: v0.2.0) [NOTE] to see how you depend on a package, run `cargo tree --invert --package @` [WARNING] not updating lockfile due to dry run @@ -1559,7 +1559,7 @@ fn report_behind() { [UPDATING] `dummy-registry` index [LOCKING] 0 packages to latest compatible versions [UNCHANGED] breaking v0.1.1 (latest: v0.2.0) -[UNCHANGED] pre v1.0.0-alpha.0 (latest: v1.0.0-alpha.1) +[UNCHANGED] pre v1.0.0-alpha.0 (latest compatible: v1.0.0-alpha.1) [UNCHANGED] two-ver v0.1.0 (latest: v0.2.0) [NOTE] to see how you depend on a package, run `cargo tree --invert --package @` [WARNING] not updating lockfile due to dry run diff --git a/tests/testsuite/workspaces.rs b/tests/testsuite/workspaces.rs index 2a89e01bfb92..b03fa0e60bb8 100644 --- a/tests/testsuite/workspaces.rs +++ b/tests/testsuite/workspaces.rs @@ -697,7 +697,7 @@ fn share_dependencies() { .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index [LOCKING] 1 package to latest compatible version -[ADDING] dep1 v0.1.3 (latest: v0.1.8) +[ADDING] dep1 v0.1.3 (latest compatible: v0.1.8) [DOWNLOADING] crates ... [DOWNLOADED] dep1 v0.1.3 (registry `dummy-registry`) [CHECKING] dep1 v0.1.3