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

fix(resolve): Dont show locking workspace members #14445

Merged
merged 1 commit into from
Aug 26, 2024
Merged

fix(resolve): Dont show locking workspace members #14445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
48 changes: 36 additions & 12 deletions src/cargo/ops/cargo_update.rs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

The truth is that Cargo actually also locks versions of workspaces members, though I believe users often just don't care.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. When I first did this, the intent was to inform users of relevant information. It can also be confusing to see these mesaages. I just made a change to some of my workspaces, removing version and cargo check reported that it downgraded those packages.

Original file line number Diff line number Diff line change
Expand Up @@ -492,15 +492,21 @@ fn print_lockfile_generation(
resolve: &Resolve,
registry: &mut PackageRegistry<'_>,
) -> CargoResult<()> {
let changes = PackageChange::new(resolve);
let num_pkgs: usize = changes.iter().filter(|change| change.kind.is_new()).count();
if num_pkgs <= 1 {
// just ourself, nothing worth reporting
let changes = PackageChange::new(ws, resolve);
let num_pkgs: usize = changes
.iter()
.filter(|change| change.kind.is_new() && !change.is_member.unwrap_or(false))
.count();
if num_pkgs == 0 {
// nothing worth reporting
return Ok(());
}
status_locking(ws, num_pkgs)?;

for change in changes {
if change.is_member.unwrap_or(false) {
continue;
};
match change.kind {
PackageChangeKind::Added => {
let possibilities = if let Some(query) = change.alternatives_query() {
Expand Down Expand Up @@ -547,14 +553,21 @@ fn print_lockfile_sync(
resolve: &Resolve,
registry: &mut PackageRegistry<'_>,
) -> CargoResult<()> {
let changes = PackageChange::diff(previous_resolve, resolve);
let num_pkgs: usize = changes.iter().filter(|change| change.kind.is_new()).count();
let changes = PackageChange::diff(ws, previous_resolve, resolve);
let num_pkgs: usize = changes
.iter()
.filter(|change| change.kind.is_new() && !change.is_member.unwrap_or(false))
.count();
if num_pkgs == 0 {
// nothing worth reporting
return Ok(());
}
status_locking(ws, num_pkgs)?;

for change in changes {
if change.is_member.unwrap_or(false) {
continue;
};
match change.kind {
PackageChangeKind::Added
| PackageChangeKind::Upgraded
Expand Down Expand Up @@ -597,7 +610,7 @@ fn print_lockfile_updates(
precise: bool,
registry: &mut PackageRegistry<'_>,
) -> CargoResult<()> {
let changes = PackageChange::diff(previous_resolve, resolve);
let changes = PackageChange::diff(ws, previous_resolve, resolve);
let num_pkgs: usize = changes.iter().filter(|change| change.kind.is_new()).count();
if !precise {
status_locking(ws, num_pkgs)?;
Expand Down Expand Up @@ -787,20 +800,23 @@ struct PackageChange {
package_id: PackageId,
previous_id: Option<PackageId>,
kind: PackageChangeKind,
is_member: Option<bool>,
Copy link
Member

Choose a reason for hiding this comment

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

I can't see the necessity of wrapping with an Option here except for representing “not relevant because it is a removed package”. Is that the reason of using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I wanted to make sure that when I accessed it, this state was clear. At first, I thought I was also going to have to act on it but that didn't end up happening.

}

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

pub fn diff(previous_resolve: &Resolve, resolve: &Resolve) -> Vec<Self> {
pub fn diff(ws: &Workspace<'_>, previous_resolve: &Resolve, resolve: &Resolve) -> Vec<Self> {
let diff = PackageDiff::diff(previous_resolve, resolve);
Self::with_diff(diff)
Self::with_diff(diff, ws)
}

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

Choose a reason for hiding this comment

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

Use Workspace::is_member instead?

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 annoying, it requires a Package just to do a lookup with PackageId


let mut changes = IndexMap::new();
for diff in diff {
if let Some((previous_id, package_id)) = diff.change() {
Expand All @@ -815,38 +831,46 @@ impl PackageChange {
} else {
PackageChangeKind::Upgraded
};
let is_member = Some(member_ids.contains(&package_id));
let change = Self {
package_id,
previous_id: Some(previous_id),
kind,
is_member,
};
changes.insert(change.package_id, change);
} else {
for package_id in diff.removed {
let kind = PackageChangeKind::Removed;
let is_member = None;
let change = Self {
package_id,
previous_id: None,
kind,
is_member,
};
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 change = Self {
package_id,
previous_id: None,
kind,
is_member,
};
changes.insert(change.package_id, change);
}
}
for package_id in diff.unchanged {
let kind = PackageChangeKind::Unchanged;
let is_member = Some(member_ids.contains(&package_id));
let change = Self {
package_id,
previous_id: None,
kind,
is_member,
};
changes.insert(change.package_id, change);
}
Expand Down
18 changes: 9 additions & 9 deletions tests/testsuite/alt_registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ fn depend_on_alt_registry() {
p.cargo("check")
.with_stderr_data(str![[r#"
[UPDATING] `alternative` index
[LOCKING] 2 packages to latest compatible versions
[LOCKING] 1 package to latest compatible version
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.0.1 (registry `alternative`)
[CHECKING] bar v0.0.1 (registry `alternative`)
Expand Down Expand Up @@ -88,7 +88,7 @@ fn depend_on_alt_registry_depends_on_same_registry_no_index() {
p.cargo("check")
.with_stderr_data(str![[r#"
[UPDATING] `alternative` index
[LOCKING] 3 packages to latest compatible versions
[LOCKING] 2 packages to latest compatible versions
[DOWNLOADING] crates ...
[DOWNLOADED] baz v0.0.1 (registry `alternative`)
[DOWNLOADED] bar v0.0.1 (registry `alternative`)
Expand Down Expand Up @@ -131,7 +131,7 @@ fn depend_on_alt_registry_depends_on_same_registry() {
p.cargo("check")
.with_stderr_data(str![[r#"
[UPDATING] `alternative` index
[LOCKING] 3 packages to latest compatible versions
[LOCKING] 2 packages to latest compatible versions
[DOWNLOADING] crates ...
[DOWNLOADED] baz v0.0.1 (registry `alternative`)
[DOWNLOADED] bar v0.0.1 (registry `alternative`)
Expand Down Expand Up @@ -176,7 +176,7 @@ fn depend_on_alt_registry_depends_on_crates_io() {
str![[r#"
[UPDATING] `alternative` index
[UPDATING] `dummy-registry` index
[LOCKING] 3 packages to latest compatible versions
[LOCKING] 2 packages to latest compatible versions
[DOWNLOADING] crates ...
[DOWNLOADED] baz v0.0.1 (registry `dummy-registry`)
[DOWNLOADED] bar v0.0.1 (registry `alternative`)
Expand Down Expand Up @@ -217,7 +217,7 @@ fn registry_and_path_dep_works() {

p.cargo("check")
.with_stderr_data(str![[r#"
[LOCKING] 2 packages to latest compatible versions
[LOCKING] 1 package to latest compatible version
[CHECKING] bar v0.0.1 ([ROOT]/foo/bar)
[CHECKING] foo v0.0.1 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
Expand Down Expand Up @@ -435,7 +435,7 @@ fn alt_registry_and_crates_io_deps() {
str![[r#"
[UPDATING] `alternative` index
[UPDATING] `dummy-registry` index
[LOCKING] 3 packages to latest compatible versions
[LOCKING] 2 packages to latest compatible versions
[DOWNLOADING] crates ...
[DOWNLOADED] crates_io_dep v0.0.1 (registry `dummy-registry`)
[DOWNLOADED] alt_reg_dep v0.1.0 (registry `alternative`)
Expand Down Expand Up @@ -710,7 +710,7 @@ fn patch_alt_reg() {
p.cargo("check")
.with_stderr_data(str![[r#"
[UPDATING] `alternative` index
[LOCKING] 2 packages to latest compatible versions
[LOCKING] 1 package to latest compatible version
[CHECKING] bar v0.1.0 ([ROOT]/foo/bar)
[CHECKING] foo v0.0.1 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
Expand Down Expand Up @@ -804,7 +804,7 @@ fn no_api() {
p.cargo("check")
.with_stderr_data(str![[r#"
[UPDATING] `alternative` index
[LOCKING] 2 packages to latest compatible versions
[LOCKING] 1 package to latest compatible version
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.0.1 (registry `alternative`)
[CHECKING] bar v0.0.1 (registry `alternative`)
Expand Down Expand Up @@ -1657,7 +1657,7 @@ fn registries_index_relative_url() {
p.cargo("check")
.with_stderr_data(str![[r#"
[UPDATING] `relative` index
[LOCKING] 2 packages to latest compatible versions
[LOCKING] 1 package to latest compatible version
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.0.1 (registry `relative`)
[CHECKING] bar v0.0.1 (registry `relative`)
Expand Down
Loading