Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(resolve): Direct people to working around less optimal MSRV-resolver results #14543

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

feat(resolve): Direct people to working around less optimal MSRV-resolver results #14543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0d2349d
refactor(update): Tighten scope of transitive reporting checks
epage Sep 30, 2024
1b3c91b
refactor(update): Remove redundant scopes
epage Sep 30, 2024
63e27c6
refactor(update): Calculate all reporting states
epage Sep 30, 2024
fbe79f5
refactor(update): Clarify reporting states are mutually exclusive
epage Sep 30, 2024
a246fd5
refactor(resolve): Change how we calculate unchanged
epage Sep 13, 2024
be2e7f4
test(resolve): Show more reporting cases
epage Sep 14, 2024
818eead
feat(resolve): Direct people to working around less optimal MSRV-reso…
epage Sep 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 141 additions & 46 deletions src/cargo/ops/cargo_update.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,7 @@ fn print_lockfile_generation(
annotate_required_rust_version(ws, resolve, &mut changes);

status_locking(ws, num_pkgs)?;
let mut changed_stats = UpdateStats::default();
for change in changes.values() {
if change.is_member.unwrap_or(false) {
continue;
Expand All @@ -538,8 +539,9 @@ fn print_lockfile_generation(
vec![]
};

let required_rust_version = report_required_rust_version(resolve, change);
let latest = report_latest(&possibilities, change);
let required_rust_version =
report_required_rust_version(resolve, change, Some(&mut changed_stats));
let latest = report_latest(&possibilities, change, Some(&mut changed_stats));
let note = required_rust_version.or(latest);

if let Some(note) = note {
Expand All @@ -559,6 +561,22 @@ fn print_lockfile_generation(
}
}

let compat_ver_compat_msrv = changed_stats.compat_ver_compat_msrv;
if 0 < compat_ver_compat_msrv
&& !ws.gctx().cli_unstable().direct_minimal_versions
&& !ws.gctx().cli_unstable().minimal_versions
{
if compat_ver_compat_msrv == 1 {
ws.gctx().shell().note(format!(
"{compat_ver_compat_msrv} package may have a higher, compatible version. To update it, run `cargo update <name> --precise <version>"
))?;
} else {
ws.gctx().shell().note(format!(
"{compat_ver_compat_msrv} packages may have a higher, compatible version. To update them, run `cargo update <name> --precise <version>"
))?;
}
}

Ok(())
}

Expand All @@ -580,6 +598,7 @@ fn print_lockfile_sync(
annotate_required_rust_version(ws, resolve, &mut changes);

status_locking(ws, num_pkgs)?;
let mut changed_stats = UpdateStats::default();
for change in changes.values() {
if change.is_member.unwrap_or(false) {
continue;
Expand All @@ -601,8 +620,9 @@ fn print_lockfile_sync(
vec![]
};

let required_rust_version = report_required_rust_version(resolve, change);
let latest = report_latest(&possibilities, change);
let required_rust_version =
report_required_rust_version(resolve, change, Some(&mut changed_stats));
let latest = report_latest(&possibilities, change, Some(&mut changed_stats));
let note = required_rust_version.or(latest).unwrap_or_default();

ws.gctx().shell().status_with_color(
Expand All @@ -615,6 +635,16 @@ fn print_lockfile_sync(
}
}

let compat_ver_compat_msrv = changed_stats.compat_ver_compat_msrv;
if 0 < compat_ver_compat_msrv
&& !ws.gctx().cli_unstable().direct_minimal_versions
&& !ws.gctx().cli_unstable().minimal_versions
{
ws.gctx().shell().note(format!(
"{compat_ver_compat_msrv} were updated but higher versions may be available by manually updating with `cargo update <name> --precise <version>"
))?;
}

Ok(())
}

Expand All @@ -636,6 +666,7 @@ fn print_lockfile_updates(
status_locking(ws, num_pkgs)?;
}
let mut unchanged_behind = 0;
let mut changed_stats = UpdateStats::default();
for change in changes.values() {
let possibilities = if let Some(query) = change.alternatives_query() {
loop {
Expand All @@ -654,8 +685,9 @@ fn print_lockfile_updates(
PackageChangeKind::Added
| PackageChangeKind::Upgraded
| PackageChangeKind::Downgraded => {
let required_rust_version = report_required_rust_version(resolve, change);
let latest = report_latest(&possibilities, change);
let required_rust_version =
report_required_rust_version(resolve, change, Some(&mut changed_stats));
let latest = report_latest(&possibilities, change, Some(&mut changed_stats));
let note = required_rust_version.or(latest).unwrap_or_default();

ws.gctx().shell().status_with_color(
Expand All @@ -672,14 +704,16 @@ fn print_lockfile_updates(
)?;
}
PackageChangeKind::Unchanged => {
let required_rust_version = report_required_rust_version(resolve, change);
let latest = report_latest(&possibilities, change);
let mut unchanged_stats = UpdateStats::default();
let required_rust_version =
report_required_rust_version(resolve, change, Some(&mut unchanged_stats));
let latest = report_latest(&possibilities, change, Some(&mut unchanged_stats));
let note = required_rust_version.as_deref().or(latest.as_deref());

if unchanged_stats.behind() {
unchanged_behind += 1;
}
if let Some(note) = note {
if latest.is_some() {
unchanged_behind += 1;
}
if ws.gctx().shell().verbosity() == Verbosity::Verbose {
ws.gctx().shell().status_with_color(
change.kind.status(),
Expand All @@ -692,6 +726,15 @@ fn print_lockfile_updates(
}
}

let compat_ver_compat_msrv = changed_stats.compat_ver_compat_msrv;
if 0 < compat_ver_compat_msrv
&& !ws.gctx().cli_unstable().direct_minimal_versions
&& !ws.gctx().cli_unstable().minimal_versions
{
ws.gctx().shell().note(format!(
"{compat_ver_compat_msrv} were updated but higher versions may be available by manually updating with `cargo update <name> --precise <version>"
))?;
}
if ws.gctx().shell().verbosity() == Verbosity::Verbose {
ws.gctx().shell().note(
"to see how you depend on a package, run `cargo tree --invert --package <dep>@<ver>`",
Expand Down Expand Up @@ -748,7 +791,11 @@ fn required_rust_version(ws: &Workspace<'_>) -> Option<PartialVersion> {
}
}

fn report_required_rust_version(resolve: &Resolve, change: &PackageChange) -> Option<String> {
fn report_required_rust_version(
resolve: &Resolve,
change: &PackageChange,
stats: Option<&mut UpdateStats>,
) -> Option<String> {
if change.package_id.source_id().is_path() {
return None;
}
Expand All @@ -759,13 +806,20 @@ fn report_required_rust_version(resolve: &Resolve, change: &PackageChange) -> Op
return None;
}

if let Some(stats) = stats {
stats.required_rust_version += 1;
}
let error = style::ERROR;
Some(format!(
" {error}(requires Rust {package_rust_version}){error:#}"
))
}

fn report_latest(possibilities: &[IndexSummary], change: &PackageChange) -> Option<String> {
fn report_latest(
possibilities: &[IndexSummary],
change: &PackageChange,
mut stats: Option<&mut UpdateStats>,
) -> Option<String> {
let package_id = change.package_id;
if !package_id.source_id().is_registry() {
return None;
Expand All @@ -788,15 +842,14 @@ fn report_latest(possibilities: &[IndexSummary], change: &PackageChange) -> Opti
})
.filter(|s| package_id.version() != s.version() && version_req.matches(s.version()))
.max_by_key(|s| s.version());
if let Some(summary) = compat_ver_compat_msrv_summary {
let warn = style::WARN;
let version = summary.version();
let report = format!(" {warn}(available: v{version}){warn:#}");
return Some(report);
if let Some(ref mut stats) = stats {
if compat_ver_compat_msrv_summary.is_some() {
stats.compat_ver_compat_msrv += 1;
}
}

if !change.is_transitive.unwrap_or(true) {
let incompat_ver_compat_msrv_summary = possibilities
let incompat_ver_compat_msrv_summary = if !change.is_transitive.unwrap_or(true) {
possibilities
.iter()
.map(|s| s.as_summary())
.filter(|s| {
Expand All @@ -809,12 +862,13 @@ fn report_latest(possibilities: &[IndexSummary], change: &PackageChange) -> Opti
}
})
.filter(|s| is_latest(s.version(), package_id.version()))
.max_by_key(|s| s.version());
if let Some(summary) = incompat_ver_compat_msrv_summary {
let warn = style::WARN;
let version = summary.version();
let report = format!(" {warn}(available: v{version}){warn:#}");
return Some(report);
.max_by_key(|s| s.version())
} else {
None
};
if let Some(stats) = stats.as_mut() {
if incompat_ver_compat_msrv_summary.is_some() {
stats.incompat_ver_compat_msrv += 1;
}
}

Expand All @@ -823,36 +877,77 @@ fn report_latest(possibilities: &[IndexSummary], change: &PackageChange) -> Opti
.map(|s| s.as_summary())
.filter(|s| package_id.version() != s.version() && version_req.matches(s.version()))
.max_by_key(|s| s.version());
if let Some(summary) = compat_ver_summary {
if let Some(stats) = stats.as_mut() {
if compat_ver_summary.is_some() {
stats.compat_ver += 1;
}
}

let incompat_ver_summary = if !change.is_transitive.unwrap_or(true) {
possibilities
.iter()
.map(|s| s.as_summary())
.filter(|s| is_latest(s.version(), package_id.version()))
.max_by_key(|s| s.version())
} else {
None
};
if let Some(stats) = stats.as_mut() {
if incompat_ver_summary.is_some() {
stats.incompat_ver += 1;
}
}

if let Some(summary) = compat_ver_compat_msrv_summary {
let warn = style::WARN;
let version = summary.version();
let report = format!(" {warn}(available: v{version}){warn:#}");
Some(report)
} else if let Some(summary) = incompat_ver_compat_msrv_summary {
let warn = style::WARN;
let version = summary.version();
let report = format!(" {warn}(available: v{version}){warn:#}");
Some(report)
} else if let Some(summary) = compat_ver_summary {
let msrv_note = summary
.rust_version()
.map(|rv| format!(", requires Rust {rv}"))
.unwrap_or_default();
let warn = style::NOP;
let version = summary.version();
let report = format!(" {warn}(available: v{version}{msrv_note}){warn:#}");
return Some(report);
Some(report)
} else if let Some(summary) = incompat_ver_summary {
let msrv_note = summary
.rust_version()
.map(|rv| format!(", requires Rust {rv}"))
.unwrap_or_default();
let warn = style::NOP;
let version = summary.version();
let report = format!(" {warn}(available: v{version}{msrv_note}){warn:#}");
Some(report)
} else {
None
}
}

if !change.is_transitive.unwrap_or(true) {
let incompat_ver_summary = possibilities
.iter()
.map(|s| s.as_summary())
.filter(|s| is_latest(s.version(), package_id.version()))
.max_by_key(|s| s.version());
if let Some(summary) = incompat_ver_summary {
let msrv_note = summary
.rust_version()
.map(|rv| format!(", requires Rust {rv}"))
.unwrap_or_default();
let warn = style::NOP;
let version = summary.version();
let report = format!(" {warn}(available: v{version}{msrv_note}){warn:#}");
return Some(report);
}
}
#[derive(Copy, Clone, Default, Debug)]
struct UpdateStats {
required_rust_version: usize,
compat_ver_compat_msrv: usize,
incompat_ver_compat_msrv: usize,
compat_ver: usize,
incompat_ver: usize,
}

None
impl UpdateStats {
fn behind(&self) -> bool {
self.compat_ver_compat_msrv
+ self.incompat_ver_compat_msrv
+ self.compat_ver
+ self.incompat_ver
!= 0
}
}

fn is_latest(candidate: &semver::Version, current: &semver::Version) -> bool {
Expand Down
31 changes: 29 additions & 2 deletions tests/testsuite/rust_version.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1113,33 +1113,57 @@ fn report_rust_versions() {
.rust_version("1.55.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();

Package::new("dep-only-low-incompatible", "1.55.0")
.rust_version("1.55.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();
Package::new("dep-only-low-incompatible", "1.75.0")
.rust_version("1.75.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();

Package::new("dep-only-high-compatible", "1.55.0")
.rust_version("1.55.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();
Package::new("dep-only-high-compatible", "1.65.0")
.rust_version("1.65.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();

Package::new("dep-only-high-incompatible", "1.55.0")
.rust_version("1.55.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();
Package::new("dep-only-high-incompatible", "1.75.0")
.rust_version("1.75.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();

Package::new("dep-only-unset-unset", "1.0.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();

Package::new("dep-only-unset-compatible", "1.55.0")
.rust_version("1.55.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();
Package::new("dep-only-unset-compatible", "1.75.0")
.rust_version("1.75.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();

Package::new("dep-only-unset-incompatible", "1.2345.0")
.rust_version("1.2345.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();

Package::new("dep-shared-compatible", "1.55.0")
.rust_version("1.55.0")
.file("src/lib.rs", "fn other_stuff() {}")
.publish();

Package::new("dep-shared-incompatible", "1.75.0")
.rust_version("1.75.0")
.file("src/lib.rs", "fn other_stuff() {}")
Expand Down Expand Up @@ -1210,10 +1234,13 @@ fn report_rust_versions() {
.with_stderr_data(str![[r#"
[UPDATING] `dummy-registry` index
[LOCKING] 9 packages to latest Rust 1.60.0 compatible versions
[ADDING] dep-only-high-incompatible v1.75.0 (requires Rust 1.75.0)
[ADDING] dep-only-low-incompatible v1.75.0 (requires Rust 1.75.0)
[ADDING] dep-only-high-compatible v1.55.0 (available: v1.65.0)
[ADDING] dep-only-high-incompatible v1.55.0 (available: v1.75.0, requires Rust 1.75.0)
[ADDING] dep-only-low-incompatible v1.55.0 (available: v1.75.0, requires Rust 1.75.0)
[ADDING] dep-only-unset-compatible v1.55.0 (available: v1.75.0)
[ADDING] dep-only-unset-incompatible v1.2345.0 (requires Rust 1.2345.0)
[ADDING] dep-shared-incompatible v1.75.0 (requires Rust 1.75.0)
[NOTE] 2 packages may have a higher, compatible version. To update them, run `cargo update <name> --precise <version>
Comment on lines 1236 to +1243
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, this message is vague to the point of being unhelpful.

Copy link
Member

@joshtriplett joshtriplett Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case where we're already showing verbose messages about all the individual crates, I agree. Could we show it only if there are messages about possible upgrades that we're currently hiding because of the verbosity level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats sounding like the message we already have

"pass `--verbose` to see {unchanged_behind} unchanged dependencies behind latest"


"#]])
.run();
Expand Down
1 change: 1 addition & 0 deletions tests/testsuite/workspaces.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,7 @@ fn share_dependencies() {
[UPDATING] `dummy-registry` index
[LOCKING] 1 package to latest compatible version
[ADDING] dep1 v0.1.3 (available: v0.1.8)
[NOTE] 1 package may have a higher, compatible version. To update it, run `cargo update <name> --precise <version>
[DOWNLOADING] crates ...
[DOWNLOADED] dep1 v0.1.3 (registry `dummy-registry`)
[CHECKING] dep1 v0.1.3
Expand Down