Skip to content

Commit

Permalink
fix(resolve): With latest message, differentiate actionable updates
Browse files Browse the repository at this point in the history 
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 rust-lang#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
  • Loading branch information
@epage @antoniospg
epage authored and antoniospg committed Sep 6, 2024
1 parent ed66100 commit 024bf5e
Show file tree
Hide file tree
Showing 7 changed files with 69 additions and 21 deletions.
46 changes: 43 additions & 3 deletions src/cargo/ops/cargo_update.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -801,13 +818,14 @@ struct PackageChange {
previous_id: Option<PackageId>,
kind: PackageChangeKind,
is_member: Option<bool>,
is_transitive: Option<bool>,
required_rust_version: Option<PartialVersion>,
}

impl PackageChange {
pub fn new(ws: &Workspace<'_>, resolve: &Resolve) -> IndexMap<PackageId, Self> {
let diff = PackageDiff::new(resolve);
Self::with_diff(diff, ws)
Self::with_diff(diff, ws, resolve)
}

pub fn diff(
Expand All @@ -816,12 +834,13 @@ impl PackageChange {
resolve: &Resolve,
) -> IndexMap<PackageId, Self> {
let diff = PackageDiff::diff(previous_resolve, resolve);
Self::with_diff(diff, ws)
Self::with_diff(diff, ws, resolve)
}

fn with_diff(
diff: impl Iterator<Item = PackageDiff>,
ws: &Workspace<'_>,
resolve: &Resolve,
) -> IndexMap<PackageId, Self> {
let member_ids: HashSet<_> = ws.members().map(|p| p.package_id()).collect();

Expand All @@ -840,35 +859,41 @@ 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);
} else {
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);
}
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);
Expand All @@ -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
}

Expand Down
14 changes: 11 additions & 3 deletions src/cargo/util/semver_ext.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,26 @@ 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 {
fn is_prerelease(&self) -> bool {
!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),
Expand Down
6 changes: 3 additions & 3 deletions tests/testsuite/direct_minimal_versions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/minimal_versions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
16 changes: 8 additions & 8 deletions tests/testsuite/rust_version.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
"#]])
Expand Down Expand Up @@ -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)
"#]])
Expand Down Expand Up @@ -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)
"#]])
Expand Down Expand Up @@ -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)
"#]])
Expand Down Expand Up @@ -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)
"#]])
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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`)
Expand Down
4 changes: 2 additions & 2 deletions tests/testsuite/update.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <dep>@<ver>`
[WARNING] not updating lockfile due to dry run
Expand All @@ -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 <dep>@<ver>`
[WARNING] not updating lockfile due to dry run
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/workspaces.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 024bf5e

Please sign in to comment.