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.

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(update): Tell users when they are still behind #13372

Merged
merged 19 commits into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions crates/cargo-test-support/src/compare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ fn substitute_macros(input: &str) -> String {
("[ADDING]", " Adding"),
("[REMOVING]", " Removing"),
("[REMOVED]", " Removed"),
("[UNCHANGED]", " Unchanged"),
("[DOCTEST]", " Doc-tests"),
("[PACKAGING]", " Packaging"),
("[PACKAGED]", " Packaged"),
Expand Down
210 changes: 164 additions & 46 deletions src/cargo/ops/cargo_generate_lockfile.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
use crate::core::registry::PackageRegistry;
use crate::core::resolver::features::{CliFeatures, HasDevUnits};
use crate::core::shell::Verbosity;
use crate::core::Registry as _;
use crate::core::{PackageId, PackageIdSpec, PackageIdSpecQuery};
use crate::core::{Resolve, SourceId, Workspace};
use crate::ops;
use crate::sources::source::QueryKind;
use crate::util::cache_lock::CacheLockMode;
use crate::util::config::Config;
use crate::util::style;
Expand Down Expand Up @@ -161,36 +164,137 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
let print_change = |status: &str, msg: String, color: &Style| {
opts.config.shell().status_with_color(status, msg, color)
};
for (removed, added) in compare_dependency_graphs(&previous_resolve, &resolve) {
let mut unchanged_behind = 0;
for ResolvedPackageVersions {
removed,
added,
unchanged,
} in compare_dependency_graphs(&previous_resolve, &resolve)
{
fn format_latest(version: semver::Version) -> String {
let warn = style::WARN;
format!(" {warn}(latest: v{version}){warn:#}")
}
fn is_latest(candidate: &semver::Version, current: &semver::Version) -> bool {
current < candidate
// Only match pre-release if major.minor.patch are the same
&& (candidate.pre.is_empty()
|| (candidate.major == current.major
&& candidate.minor == current.minor
&& candidate.patch == current.patch))
}
let possibilities = if let Some(query) = [added.iter(), unchanged.iter()]
.into_iter()
.flatten()
.next()
.filter(|s| s.source_id().is_registry())
{
let query =
crate::core::dependency::Dependency::parse(query.name(), None, query.source_id())?;
loop {
match registry.query_vec(&query, QueryKind::Exact) {
std::task::Poll::Ready(res) => {
break res?;
}
std::task::Poll::Pending => registry.block_until_ready()?,
}
}
} else {
vec![]
};

if removed.len() == 1 && added.len() == 1 {
let msg = if removed[0].source_id().is_git() {
let added = added.into_iter().next().unwrap();
let removed = removed.into_iter().next().unwrap();

let latest = if !possibilities.is_empty() {
possibilities
.iter()
.map(|s| s.as_summary())
.filter(|s| is_latest(s.version(), added.version()))
.map(|s| s.version().clone())
.max()
.map(format_latest)
} else {
None
}
.unwrap_or_default();

let msg = if removed.source_id().is_git() {
format!(
"{} -> #{}",
removed[0],
&added[0].source_id().precise_git_fragment().unwrap()[..8],
"{removed} -> #{}",
&added.source_id().precise_git_fragment().unwrap()[..8],
)
} else {
format!("{} -> v{}", removed[0], added[0].version())
format!("{removed} -> v{}{latest}", added.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.
if removed[0].version().cmp_precedence(added[0].version()) == Ordering::Greater {
if removed.version().cmp_precedence(added.version()) == Ordering::Greater {
print_change("Downgrading", msg, &style::WARN)?;
} else {
print_change("Updating", msg, &style::GOOD)?;
}
} else {
for package in removed.iter() {
print_change("Removing", format!("{}", package), &style::ERROR)?;
print_change("Removing", format!("{package}"), &style::ERROR)?;
}
for package in added.iter() {
print_change("Adding", format!("{}", package), &style::NOTE)?;
let latest = if !possibilities.is_empty() {
possibilities
.iter()
.map(|s| s.as_summary())
.filter(|s| is_latest(s.version(), package.version()))
.map(|s| s.version().clone())
.max()
.map(format_latest)
} else {
None
}
.unwrap_or_default();

print_change("Adding", format!("{package}{latest}"), &style::NOTE)?;
}
}
for package in &unchanged {
let latest = if !possibilities.is_empty() {
possibilities
.iter()
.map(|s| s.as_summary())
.filter(|s| is_latest(s.version(), package.version()))
.map(|s| s.version().clone())
.max()
.map(format_latest)
} else {
None
};

if let Some(latest) = latest {
unchanged_behind += 1;
if opts.config.shell().verbosity() == Verbosity::Verbose {
opts.config.shell().status_with_color(
"Unchanged",
format!("{package}{latest}"),
&anstyle::Style::new().bold(),
)?;
}
}
}
}
if opts.config.shell().verbosity() == Verbosity::Verbose {
opts.config.shell().note(
"to see how you depend on a package, run `cargo tree --invert --package <dep>@<ver>`",
)?;
} else {
if 0 < unchanged_behind {
Copy link
Contributor

@djc djc Jan 31, 2024

Choose a reason for hiding this comment

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

Suggestion: I think writing unchanged_behind > 0 would be more idiomatic (and easier to parse for me, at least)?

And even then, it's a little unclear to me what this condition is about. Maybe add a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While there is yoda speak (constant == variable) and more general speak (variable == constant) for equality, I see comparison operators differently and prefer to write them in number line order as I find that easier to read.

Copy link
Member

Choose a reason for hiding this comment

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

I am on the same side of djc regarding the order, but not really a blocker.

opts.config.shell().note(format!(
"pass `--verbose` to see {unchanged_behind} unchanged dependencies behind latest"
))?;
}
}
if opts.dry_run {
opts.config
.shell()
Expand All @@ -215,73 +319,87 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes
}
}

#[derive(Default, Clone, Debug)]
struct ResolvedPackageVersions {
removed: Vec<PackageId>,
added: Vec<PackageId>,
unchanged: Vec<PackageId>,
}
fn compare_dependency_graphs(
epage marked this conversation as resolved.
Show resolved Hide resolved
previous_resolve: &Resolve,
resolve: &Resolve,
) -> Vec<(Vec<PackageId>, Vec<PackageId>)> {
) -> Vec<ResolvedPackageVersions> {
fn key(dep: PackageId) -> (&'static str, SourceId) {
(dep.name().as_str(), dep.source_id())
}

// Removes all package IDs in `b` from `a`. Note that this is somewhat
// more complicated because the equality for source IDs does not take
// precise versions into account (e.g., git shas), but we want to take
// that into account here.
fn vec_subtract(a: &[PackageId], b: &[PackageId]) -> Vec<PackageId> {
a.iter()
.filter(|a| {
// If this package ID is not found in `b`, then it's definitely
// in the subtracted set.
let Ok(i) = b.binary_search(a) else {
return true;
};
fn vec_subset(a: &[PackageId], b: &[PackageId]) -> Vec<PackageId> {
a.iter().filter(|a| !contains_id(b, a)).cloned().collect()
}

// If we've found `a` in `b`, then we iterate over all instances
// (we know `b` is sorted) and see if they all have different
// precise versions. If so, then `a` isn't actually in `b` so
// we'll let it through.
//
// Note that we only check this for non-registry sources,
// however, as registries contain enough version information in
// the package ID to disambiguate.
if a.source_id().is_registry() {
return false;
}
b[i..]
.iter()
.take_while(|b| a == b)
.all(|b| !a.source_id().has_same_precise_as(b.source_id()))
})
.cloned()
.collect()
fn vec_intersection(a: &[PackageId], b: &[PackageId]) -> Vec<PackageId> {
a.iter().filter(|a| contains_id(b, a)).cloned().collect()
}

// Check if a PackageId is present `b` from `a`.
//
// Note that this is somewhat more complicated because the equality for source IDs does not
// take precise versions into account (e.g., git shas), but we want to take that into
// account here.
fn contains_id(haystack: &[PackageId], needle: &PackageId) -> bool {
let Ok(i) = haystack.binary_search(needle) else {
return false;
};

// If we've found `a` in `b`, then we iterate over all instances
// (we know `b` is sorted) and see if they all have different
// precise versions. If so, then `a` isn't actually in `b` so
// we'll let it through.
//
// Note that we only check this for non-registry sources,
// however, as registries contain enough version information in
// the package ID to disambiguate.
if needle.source_id().is_registry() {
return true;
}
haystack[i..]
.iter()
.take_while(|b| &needle == b)
.any(|b| needle.source_id().has_same_precise_as(b.source_id()))
}

// Map `(package name, package source)` to `(removed versions, added versions)`.
let mut changes = BTreeMap::new();
let empty = (Vec::new(), Vec::new());
let empty = ResolvedPackageVersions::default();
for dep in previous_resolve.iter() {
changes
.entry(key(dep))
.or_insert_with(|| empty.clone())
.0
.removed
.push(dep);
}
for dep in resolve.iter() {
changes
.entry(key(dep))
.or_insert_with(|| empty.clone())
.1
.added
.push(dep);
}

for v in changes.values_mut() {
let (ref mut old, ref mut new) = *v;
let ResolvedPackageVersions {
removed: ref mut old,
added: ref mut new,
unchanged: ref mut other,
} = *v;
old.sort();
new.sort();
let removed = vec_subtract(old, new);
let added = vec_subtract(new, old);
let removed = vec_subset(old, new);
let added = vec_subset(new, old);
let unchanged = vec_intersection(new, old);
*old = removed;
*new = added;
*other = unchanged;
}
debug!("{:#?}", changes);

Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1372,7 +1372,7 @@ fn dep_with_changed_submodule() {
sleep_ms(1000);
// Update the dependency and carry on!
println!("update");
p.cargo("update -v")
p.cargo("update")
.with_stderr("")
.with_stderr(&format!(
"[UPDATING] git repository `{}`\n\
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/local_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ fn multiple_versions() {
.file("src/lib.rs", "pub fn bar() {}")
.publish();

p.cargo("update -v")
p.cargo("update")
.with_stderr("[UPDATING] bar v0.1.0 -> v0.2.0")
.run();
}
Expand Down
1 change: 1 addition & 0 deletions tests/testsuite/patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2430,6 +2430,7 @@ fn can_update_with_alt_reg() {
"\
[UPDATING] `alternative` index
[UPDATING] `dummy-registry` index
[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
",
)
.run();
Expand Down
1 change: 1 addition & 0 deletions tests/testsuite/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1572,6 +1572,7 @@ fn update_multiple_packages() {
[UPDATING] `[..]` index
[UPDATING] a v0.1.0 -> v0.1.1
[UPDATING] b v0.1.0 -> v0.1.1
[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
",
)
.run();
Expand Down
2 changes: 2 additions & 0 deletions tests/testsuite/replace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -539,13 +539,15 @@ fn override_adds_some_deps() {
"\
[UPDATING] git repository `file://[..]`
[UPDATING] `dummy-registry` index
[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
",
)
.run();
p.cargo("update https://github.com/rust-lang/crates.io-index#bar")
.with_stderr(
"\
[UPDATING] `dummy-registry` index
[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
",
)
.run();
Expand Down
Loading