From f3f1447f663b5315714f63b3daa1531b16d0bc18 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 5 Mar 2024 12:35:21 -0600 Subject: [PATCH 1/5] refactor(lockfile): Pull out change printing --- src/cargo/ops/cargo_generate_lockfile.rs | 217 ++++++++++++----------- 1 file changed, 114 insertions(+), 103 deletions(-) diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index 574a9adcd78..2acc212d939 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -154,9 +154,26 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes true, )?; + print_lockfile_update(opts.gctx, &previous_resolve, &resolve, &mut registry)?; + if opts.dry_run { + opts.gctx + .shell() + .warn("not updating lockfile due to dry run")?; + } else { + ops::write_pkg_lockfile(ws, &mut resolve)?; + } + Ok(()) +} + +fn print_lockfile_update( + gctx: &GlobalContext, + previous_resolve: &Resolve, + resolve: &Resolve, + registry: &mut PackageRegistry<'_>, +) -> CargoResult<()> { // Summarize what is changing for the user. let print_change = |status: &str, msg: String, color: &Style| { - opts.gctx.shell().status_with_color(status, msg, color) + gctx.shell().status_with_color(status, msg, color) }; let mut unchanged_behind = 0; for ResolvedPackageVersions { @@ -268,8 +285,8 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes if let Some(latest) = latest { unchanged_behind += 1; - if opts.gctx.shell().verbosity() == Verbosity::Verbose { - opts.gctx.shell().status_with_color( + if gctx.shell().verbosity() == Verbosity::Verbose { + gctx.shell().status_with_color( "Unchanged", format!("{package}{latest}"), &anstyle::Style::new().bold(), @@ -278,125 +295,119 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes } } } - if opts.gctx.shell().verbosity() == Verbosity::Verbose { - opts.gctx.shell().note( + if gctx.shell().verbosity() == Verbosity::Verbose { + gctx.shell().note( "to see how you depend on a package, run `cargo tree --invert --package @`", )?; } else { if 0 < unchanged_behind { - opts.gctx.shell().note(format!( + gctx.shell().note(format!( "pass `--verbose` to see {unchanged_behind} unchanged dependencies behind latest" ))?; } } - if opts.dry_run { - opts.gctx - .shell() - .warn("not updating lockfile due to dry run")?; - } else { - ops::write_pkg_lockfile(ws, &mut resolve)?; + + Ok(()) +} + +fn fill_with_deps<'a>( + resolve: &'a Resolve, + dep: PackageId, + set: &mut HashSet, + visited: &mut HashSet, +) { + if !visited.insert(dep) { + return; + } + set.insert(dep); + for (dep, _) in resolve.deps_not_replaced(dep) { + fill_with_deps(resolve, dep, set, visited); } - return Ok(()); +} - fn fill_with_deps<'a>( - resolve: &'a Resolve, - dep: PackageId, - set: &mut HashSet, - visited: &mut HashSet, - ) { - if !visited.insert(dep) { - return; - } - set.insert(dep); - for (dep, _) in resolve.deps_not_replaced(dep) { - fill_with_deps(resolve, dep, set, visited); - } +#[derive(Default, Clone, Debug)] +struct ResolvedPackageVersions { + removed: Vec, + added: Vec, + unchanged: Vec, +} +fn compare_dependency_graphs( + previous_resolve: &Resolve, + resolve: &Resolve, +) -> Vec { + fn key(dep: PackageId) -> (&'static str, SourceId) { + (dep.name().as_str(), dep.source_id()) } - #[derive(Default, Clone, Debug)] - struct ResolvedPackageVersions { - removed: Vec, - added: Vec, - unchanged: Vec, + fn vec_subset(a: &[PackageId], b: &[PackageId]) -> Vec { + a.iter().filter(|a| !contains_id(b, a)).cloned().collect() } - fn compare_dependency_graphs( - previous_resolve: &Resolve, - resolve: &Resolve, - ) -> Vec { - fn key(dep: PackageId) -> (&'static str, SourceId) { - (dep.name().as_str(), dep.source_id()) - } - fn vec_subset(a: &[PackageId], b: &[PackageId]) -> Vec { - a.iter().filter(|a| !contains_id(b, a)).cloned().collect() - } + fn vec_intersection(a: &[PackageId], b: &[PackageId]) -> Vec { + a.iter().filter(|a| contains_id(b, a)).cloned().collect() + } - fn vec_intersection(a: &[PackageId], b: &[PackageId]) -> Vec { - 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; + }; - // Check if a PackageId is present `b` from `a`. + // 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 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 = ResolvedPackageVersions::default(); - for dep in previous_resolve.iter() { - changes - .entry(key(dep)) - .or_insert_with(|| empty.clone()) - .removed - .push(dep); - } - for dep in resolve.iter() { - changes - .entry(key(dep)) - .or_insert_with(|| empty.clone()) - .added - .push(dep); + // 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())) + } - for v in changes.values_mut() { - let ResolvedPackageVersions { - removed: ref mut old, - added: ref mut new, - unchanged: ref mut other, - } = *v; - old.sort(); - new.sort(); - 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); + // Map `(package name, package source)` to `(removed versions, added versions)`. + let mut changes = BTreeMap::new(); + let empty = ResolvedPackageVersions::default(); + for dep in previous_resolve.iter() { + changes + .entry(key(dep)) + .or_insert_with(|| empty.clone()) + .removed + .push(dep); + } + for dep in resolve.iter() { + changes + .entry(key(dep)) + .or_insert_with(|| empty.clone()) + .added + .push(dep); + } - changes.into_iter().map(|(_, v)| v).collect() + for v in changes.values_mut() { + let ResolvedPackageVersions { + removed: ref mut old, + added: ref mut new, + unchanged: ref mut other, + } = *v; + old.sort(); + new.sort(); + 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); + + changes.into_iter().map(|(_, v)| v).collect() } From e65e4358099c251dc250c7810ebf9fcfaec63924 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 7 Mar 2024 15:56:09 -0600 Subject: [PATCH 2/5] refactor(lockfile): Extract PackageDiff API --- src/cargo/ops/cargo_generate_lockfile.rs | 146 +++++++++++------------ 1 file changed, 73 insertions(+), 73 deletions(-) diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index 2acc212d939..536c6e84d74 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -176,11 +176,11 @@ fn print_lockfile_update( gctx.shell().status_with_color(status, msg, color) }; let mut unchanged_behind = 0; - for ResolvedPackageVersions { + for PackageDiff { removed, added, unchanged, - } in compare_dependency_graphs(&previous_resolve, &resolve) + } in PackageDiff::diff(&previous_resolve, &resolve) { fn format_latest(version: semver::Version) -> String { let warn = style::WARN; @@ -326,88 +326,88 @@ fn fill_with_deps<'a>( } #[derive(Default, Clone, Debug)] -struct ResolvedPackageVersions { +pub struct PackageDiff { removed: Vec, added: Vec, unchanged: Vec, } -fn compare_dependency_graphs( - previous_resolve: &Resolve, - resolve: &Resolve, -) -> Vec { - fn key(dep: PackageId) -> (&'static str, SourceId) { - (dep.name().as_str(), dep.source_id()) - } - fn vec_subset(a: &[PackageId], b: &[PackageId]) -> Vec { - a.iter().filter(|a| !contains_id(b, a)).cloned().collect() - } +impl PackageDiff { + pub fn diff(previous_resolve: &Resolve, resolve: &Resolve) -> Vec { + fn key(dep: PackageId) -> (&'static str, SourceId) { + (dep.name().as_str(), dep.source_id()) + } - fn vec_intersection(a: &[PackageId], b: &[PackageId]) -> Vec { - a.iter().filter(|a| contains_id(b, a)).cloned().collect() - } + fn vec_subset(a: &[PackageId], b: &[PackageId]) -> Vec { + 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; - }; + fn vec_intersection(a: &[PackageId], b: &[PackageId]) -> Vec { + 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. + // Check if a PackageId is present `b` from `a`. // - // 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; + // 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())) } - 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 = ResolvedPackageVersions::default(); - for dep in previous_resolve.iter() { - changes - .entry(key(dep)) - .or_insert_with(|| empty.clone()) - .removed - .push(dep); - } - for dep in resolve.iter() { - changes - .entry(key(dep)) - .or_insert_with(|| empty.clone()) - .added - .push(dep); - } + // Map `(package name, package source)` to `(removed versions, added versions)`. + let mut changes = BTreeMap::new(); + let empty = Self::default(); + for dep in previous_resolve.iter() { + changes + .entry(key(dep)) + .or_insert_with(|| empty.clone()) + .removed + .push(dep); + } + for dep in resolve.iter() { + changes + .entry(key(dep)) + .or_insert_with(|| empty.clone()) + .added + .push(dep); + } - for v in changes.values_mut() { - let ResolvedPackageVersions { - removed: ref mut old, - added: ref mut new, - unchanged: ref mut other, - } = *v; - old.sort(); - new.sort(); - 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); + for v in changes.values_mut() { + let Self { + removed: ref mut old, + added: ref mut new, + unchanged: ref mut other, + } = *v; + old.sort(); + new.sort(); + 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); - changes.into_iter().map(|(_, v)| v).collect() + changes.into_iter().map(|(_, v)| v).collect() + } } From 93c444664987ea6a032640fde5d7d4acea645cea Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 7 Mar 2024 16:07:44 -0600 Subject: [PATCH 3/5] refactor(lockfile): Allow print code to call into Diff logic --- src/cargo/ops/cargo_generate_lockfile.rs | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index 536c6e84d74..f80c3a3db05 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -176,12 +176,7 @@ fn print_lockfile_update( gctx.shell().status_with_color(status, msg, color) }; let mut unchanged_behind = 0; - for PackageDiff { - removed, - added, - unchanged, - } in PackageDiff::diff(&previous_resolve, &resolve) - { + for diff in PackageDiff::diff(&previous_resolve, &resolve) { fn format_latest(version: semver::Version) -> String { let warn = style::WARN; format!(" {warn}(latest: v{version}){warn:#}") @@ -194,7 +189,7 @@ fn print_lockfile_update( && candidate.minor == current.minor && candidate.patch == current.patch)) } - let possibilities = if let Some(query) = [added.iter(), unchanged.iter()] + let possibilities = if let Some(query) = [diff.added.iter(), diff.unchanged.iter()] .into_iter() .flatten() .next() @@ -214,9 +209,9 @@ fn print_lockfile_update( vec![] }; - if removed.len() == 1 && added.len() == 1 { - let added = added.into_iter().next().unwrap(); - let removed = removed.into_iter().next().unwrap(); + if diff.removed.len() == 1 && diff.added.len() == 1 { + let added = diff.added.into_iter().next().unwrap(); + let removed = diff.removed.into_iter().next().unwrap(); let latest = if !possibilities.is_empty() { possibilities @@ -250,10 +245,10 @@ fn print_lockfile_update( print_change("Updating", msg, &style::GOOD)?; } } else { - for package in removed.iter() { + for package in diff.removed.iter() { print_change("Removing", format!("{package}"), &style::ERROR)?; } - for package in added.iter() { + for package in diff.added.iter() { let latest = if !possibilities.is_empty() { possibilities .iter() @@ -270,7 +265,7 @@ fn print_lockfile_update( print_change("Adding", format!("{package}{latest}"), &style::NOTE)?; } } - for package in &unchanged { + for package in &diff.unchanged { let latest = if !possibilities.is_empty() { possibilities .iter() From 9e18941ff624cfd131a4ceba5323d3aada885cff Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 7 Mar 2024 16:10:13 -0600 Subject: [PATCH 4/5] refactor(lockfile): Pull out alternatives logic --- src/cargo/ops/cargo_generate_lockfile.rs | 31 ++++++++++++++++++------ 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index f80c3a3db05..ff1c85e75c9 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -189,14 +189,7 @@ fn print_lockfile_update( && candidate.minor == current.minor && candidate.patch == current.patch)) } - let possibilities = if let Some(query) = [diff.added.iter(), diff.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())?; + let possibilities = if let Some(query) = diff.alternatives_query() { loop { match registry.query_vec(&query, QueryKind::Exact) { std::task::Poll::Ready(res) => { @@ -320,6 +313,7 @@ fn fill_with_deps<'a>( } } +/// All resolved versions of a package name within a [`SourceId`] #[derive(Default, Clone, Debug)] pub struct PackageDiff { removed: Vec, @@ -405,4 +399,25 @@ impl PackageDiff { changes.into_iter().map(|(_, v)| v).collect() } + + /// For querying [`PackageRegistry`] for alternative versions to report to the user + pub fn alternatives_query(&self) -> Option { + let package_id = [ + self.added.iter(), + self.unchanged.iter(), + self.removed.iter(), + ] + .into_iter() + .flatten() + .next() + // Limit to registry as that is the only source with meaningful alternative versions + .filter(|s| s.source_id().is_registry())?; + let query = crate::core::dependency::Dependency::parse( + package_id.name(), + None, + package_id.source_id(), + ) + .expect("already a valid dependency"); + Some(query) + } } From 619238e86bd59a48c3176961b134dc82d1143749 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 7 Mar 2024 16:50:38 -0600 Subject: [PATCH 5/5] refactor(lockfile): Pull out dep change detection --- src/cargo/ops/cargo_generate_lockfile.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index ff1c85e75c9..394dde6c90f 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -202,10 +202,7 @@ fn print_lockfile_update( vec![] }; - if diff.removed.len() == 1 && diff.added.len() == 1 { - let added = diff.added.into_iter().next().unwrap(); - let removed = diff.removed.into_iter().next().unwrap(); - + if let Some((removed, added)) = diff.change() { let latest = if !possibilities.is_empty() { possibilities .iter() @@ -400,6 +397,19 @@ impl PackageDiff { changes.into_iter().map(|(_, v)| v).collect() } + /// Guess if a package upgraded/downgraded + /// + /// All `PackageDiff` knows is that entries were added/removed within [`Resolve`]. + /// A package could be added or removed because of dependencies from other packages + /// which makes it hard to definitively say "X was upgrade to N". + pub fn change(&self) -> Option<(&PackageId, &PackageId)> { + if self.removed.len() == 1 && self.added.len() == 1 { + Some((&self.removed[0], &self.added[0])) + } else { + None + } + } + /// For querying [`PackageRegistry`] for alternative versions to report to the user pub fn alternatives_query(&self) -> Option { let package_id = [