Improved consistency between breaking and non-breaking updates #14259

torhovland · 2024-07-16T09:38:19Z

Related to #12425.

This is ensuring more consistent output messages between breaking and non-breaking updates, as well as better error messages, as we are now running the same ops::update_lockfile in either case.

See how the the tests changed for examples. This becomes even more important after adding support for breaking precise updates in #14140, and this PR is necessary to unblock that PR. Here's why:

Some of the duplicated existing precise tests with the unstable feature enabled are not passing without this PR. In other words, we would be changing the behaviour of the non-breaking precise update.
Some invocations are silently finishing rather than reporting an error such as this:

[ERROR] package ID specification `[email protected]` did not match any packages
Did you mean one of these?

  [email protected]

The output is generally not consistent with the non-breaking update. For example, without this PR the output changes like this:

-[UPDATING] incompatible v0.1.0 -> v0.2.0
-[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest
+[LOCKING] 1 package to latest compatible version
+[UPDATING] incompatible v0.1.0 -> v0.2.0 (latest: v0.2.1)

Requested as a separate PR here and here.

This PR does not introduce an error message if there is nothing to update. Instead, it adds more output, as demonstrated by the test update_breaking_specific_packages_that_wont_update:

[LOCKING] 0 packages to latest compatible versions
[NOTE] pass `--verbose` to see 5 unchanged dependencies behind latest

Whether or not to throw an error is still an item on the TODO list here.

rustbot · 2024-07-16T09:38:24Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

torhovland · 2024-07-16T09:52:00Z

src/cargo/ops/cargo_update.rs

+                // In case of `--breaking` (or precise upgrades, coming in
+                // #14140), we want to keep all packages unchanged that didn't
+                // get upgraded.
+                    || (opts.breaking && !upgrades.contains_key(&(p.name(), p.source_id())))


This is an important change that has been discussed here: #14140 (comment)

To put another way, there is concern with using keep like this and resolving this is a blocker for moving forward.

I am curious what the actual concern is, though. We are only modifying keep in the case of breaking or precise updates, and we have quite extensive test coverage of those.

The reason this is not put in to_avoid is that those require PackageIds, i.e. they require a version. When doing upgrades, we don't have any versions, only version requirements.

In general converting from a requirement to a version is not possible and converting to a set of versions requires scanning all the versions on the index. But in this case keep is only called on versions that are in the lock file. I think we already scan all the versions in the lock file to construct to_avoid, so I'd rather see this check done there.

I have changed this PR so it no longer changes keep, but instead builds up what it needs in to_avoid. Please review!

epage · 2024-07-16T13:32:39Z

src/cargo/ops/cargo_update.rs

@@ -49,7 +62,11 @@ pub fn generate_lockfile(ws: &Workspace<'_>) -> CargoResult<()> {
    Ok(())
 }

-pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoResult<()> {
+pub fn update_lockfile(


This is ensuring more consistent output messages between breaking and non-breaking updates, as well as better error messages, as we are now running the same ops::update_lockfile in either case.

Reusing ops::update_lockfile is one way of improving these. Are there alternative designs we could use?

We could duplicate update_lockfile and modify it to fit our needs, but the approach in this PR seems better to me. A significant chunk of the function is now conditional on breaking updates:

if opts.breaking {} else {}

epage · 2024-07-16T13:34:14Z

Requested as a separate PR #14140 (comment) and #14140 (comment).

Quoting from the first link:

Except this PR description does not include an explanation as to why this PR addresses that and why an error is not the way forward
Description updated.

The PR says:

This PR is making a change that also affects cargo update --breaking. We'll reuse ops::update_lockfile() for both breaking and non-breaking updates. The benefit is more consistent output and behaviour between the two. In particular, it addresses a task about an error output if there is nothing to upgrade. See #12425 (comment).

This is focused on the implementation, glosses over the reason, and doesn't say what is being changed.

I think its best to split this out from this PR, either making ti independent or making one depend on the other. Trying to slip it in like this is making it harder to review and discuss both.

If this PR is related to this comment, its not clear. It doesn't talk about about the error case mentioned in that comment or give a justification for why this is the approach to go.

torhovland · 2024-07-18T10:17:09Z

If this PR is related to this comment, its not clear. It doesn't talk about about the error case mentioned in that comment or give a justification for why this is the approach to go.

I have updated the PR description with more context. Not sure what else to add. If you think it needs more, let me know.

epage · 2024-07-18T13:52:29Z

See how the the tests changed for examples.

That is what I am wanting you to talk about. This is supposed to change behavior related to one of the tasks in a direction that I have serious doubs about. I'd like the intended behavior changes enumerated and why those behaviors were selected, not an explanation for what this unblocks.

epage · 2024-07-18T13:56:51Z

src/cargo/ops/cargo_update.rs

+                let spec = PackageIdSpec::new(name.to_string())
+                    .with_url(source_id.url().clone())
+                    .with_version(matching_dep.version().clone().into());
+                let spec = format!("{spec}");


isn't that just spec.to_string()?

torhovland · 2024-07-21T07:34:57Z

If this PR is related to this comment, its not clear. It doesn't talk about about the error case mentioned in that comment or give a justification for why this is the approach to go.

I have added to the description something about whether or not to error if there is nothing to do.

torhovland · 2024-07-21T07:39:48Z

See how the the tests changed for examples.

That is what I am wanting you to talk about. This is supposed to change behavior related to one of the tasks in a direction that I have serious doubs about. I'd like the intended behavior changes enumerated and why those behaviors were selected, not an explanation for what this unblocks.

This PR isn't really related to the task list at all. The only purpose of this PR is to factor out any change to to_avoid or keep in fn update_lockfile.

torhovland · 2024-08-06T09:29:32Z

@epage Could you let me know if you have any further concerns about this PR? #14140 is blocked on this.

torhovland · 2024-08-21T06:10:40Z

@epage Could you take a look at #14140 (comment), please? I believe it will unblock this PR as well as #14140.

bors · 2024-09-03T16:31:18Z

☔ The latest upstream changes (presumably #14471) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned weihanglo Jul 16, 2024

rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-update S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 16, 2024

torhovland mentioned this pull request Jul 16, 2024

Add capability of making breaking changes in update --precise #14140

Draft

torhovland commented Jul 16, 2024

View reviewed changes

torhovland force-pushed the switch-to-update-lockfile branch from 5cb1f15 to 98ccf0f Compare July 16, 2024 09:53

epage reviewed Jul 16, 2024

View reviewed changes

test: Adjust update-breaking tests.

8c8ad48

torhovland force-pushed the switch-to-update-lockfile branch 2 times, most recently from e2162b7 to 66bc2fe Compare July 18, 2024 10:03

epage reviewed Jul 18, 2024

View reviewed changes

fix: Use ops::update_lockfile for consistency with non-breaking update.

324ad02

torhovland force-pushed the switch-to-update-lockfile branch from 66bc2fe to 324ad02 Compare July 21, 2024 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved consistency between breaking and non-breaking updates #14259

Improved consistency between breaking and non-breaking updates #14259

torhovland commented Jul 16, 2024 •

edited

Loading

rustbot commented Jul 16, 2024

torhovland Jul 16, 2024

epage Jul 16, 2024

torhovland Jul 16, 2024

Eh2406 Jul 17, 2024

torhovland Jul 18, 2024

epage Jul 16, 2024

torhovland Jul 18, 2024

epage commented Jul 16, 2024

torhovland commented Jul 18, 2024

epage commented Jul 18, 2024

epage Jul 18, 2024

torhovland Jul 21, 2024

torhovland commented Jul 21, 2024

torhovland commented Jul 21, 2024

torhovland commented Aug 6, 2024

torhovland commented Aug 21, 2024

bors commented Sep 3, 2024

Improved consistency between breaking and non-breaking updates #14259

Are you sure you want to change the base?

Improved consistency between breaking and non-breaking updates #14259

Conversation

torhovland commented Jul 16, 2024 • edited Loading

rustbot commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage commented Jul 16, 2024

torhovland commented Jul 18, 2024

epage commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torhovland commented Jul 21, 2024

torhovland commented Jul 21, 2024

torhovland commented Aug 6, 2024

torhovland commented Aug 21, 2024

bors commented Sep 3, 2024

torhovland commented Jul 16, 2024 •

edited

Loading