cargo

Add capability of making breaking changes in `update --precise`

#14140

Open

Why were these changed?

Because there is no update-breaking feature. There's only the -Zunstable-options.

We still want an identifying label on all uses of masquerade_as_nightly_cargo so we can easily find all tests related to an unstable feature even if there isn't an "unstable feature" for it.

Fixed.

Why a new unstable option rather than unstable_options?

Because since there isn't any difference between the breaking command cargo update foo --precise 2.0.1 and its non-breaking counterpart, I thought we needed a feature.

Should the breaking instead be cargo update foo --breaking --precise 2.0.1?

Or is it fine to just use unstable-options to allow the breaking update?

I view this as a new supported value within the option which could be used with -Zunstable-options. If there is doing more than turning error cases into success cases, then we might want to consider a more explicit opt-in.

Fixed.

Still trying to decide what the best approach is here but in the mean time, could you split this into two commits

• One that duplicates the tests
• One that enables the unstable feature

That way it shows what behavior change happened (or not) with the feature.

Instead of duplicating tests, one approach we could take is using a test only env var to activate the unstable behavior, for example __CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2.

(Yes I admit that this opens a door for people without -Zunstable-options, though we already have __CARGO_TEST_CHANNEL_OVERRIDE_DO_NOT_USE_THIS hence 😬)

__CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2 is used from tests to modify Cargo behaviour. You would still need duplicate tests if you wanted to run Cargo with both options.

I was thinking the duplicate test file is OK, as it is meant to be temporary and can be deleted later.

I have put it in a separate commit, and it didn't need any modification after implementing the feature.

This was split out into its own commit but I'd like to see it split into two commits.

Currently, the commit is

$ cp tests/testsuite/update.rs
$ edit tests/testsuite/update_duplicated_with_precise_breaking_feature.rs
$ git commit

I was asking to see

$ cp tests/testsuite/update.rs
$ git commit
$ edit tests/testsuite/update_duplicated_with_precise_breaking_feature.rs
$ git commit

Depending on how well it diffs, maybe it should be more

This makes it easier to audit in this review, and in the future, what the differences are between the two tests files.

Why are these unspecified?

I thought it was a list of features, but I see now it is a list of reasons. I'll update all the empty ones.

Fixed.

Like with --breaking, should we limit this to just the ones that can be upgraded?

Like other cases, this can be broken out into your task list

I'm curious what you think the correct behaviour should be. We are upgrading shared, so we are not trying to keep it unchanged. So the update tries to do its thing, but fails. Anyway, I'll add this to the task list.

This is

Can we limit updates more in case of breaking upgrades? See #14140 (comment) and the test update_precise_breaking_shared_non_ws.

See #12425 (comment)

I've renamed the task in the hope that it has enough context to stand on its own

Should cargo update --precise only attempt to update direct dependencies (which can succeed) or also transitive dependencies (which will typically fail) See #14140 (comment) and the test update_precise_breaking_shared_non_ws.

So --precise will error if the mentioned item can't upgrade but --breaking won't...

Unlike with --breaking, there is no command line difference between a breaking and a non-breaking precise update. So we just try to do any upgrades, and whether or not we did any, the same lockfile update is run. So a precise update may fall back to being non-breaking, and may fail in the same way as a precise update could break before.

If we changed the API to --breaking --precise we could stop if there aren't any upgrades.

I've renamed the existing task to

Should cargo update --breaking error if the command completed without doing any upgrades?

and added this as a parallel task

Should cargo update --precise error if the command completed without doing any upgrades?

Is there a rest that makes sure cargo update incompatible --precise 2.3.4 sets the version req correctly when 2.5.7 is present?

Does adding 0.4.5 here cover it?

    Package::new("incompatible", "0.3.0").publish();
    Package::new("incompatible", "0.3.1").publish();
    Package::new("incompatible", "0.4.5").publish();

    p.cargo("update -Zunstable-options -p incompatible@0.1.0 --precise 0.3.0")
        .with_stderr_data(str![[r#"
[UPGRADING] incompatible ^0.1 -> ^0.3
[UPDATING] incompatible v0.1.0 -> v0.3.0

That covers

• Compatible upgrade available
• Incompatible upgrade available

Seems to cover every case.

Only did a quick glance at the tests; haven't gotten to the implementation yet.

In case my comment gets lost track of on a merged pr

Hadn't notice this before. This error message is bad. We should at least be calling out that we are parsing a package id spec

Existing behavior:

$ cargo update clap@foo
error: invalid package ID specification: `clap@foo`

Caused by:
  expected a version like "1.32"

(again, task is list is fine)

#14049 (comment)

Added to list.

With this change, it is now possible to revert the introduction of the dry_run argument in resolve_ws. Let me know what you think.

@epage I'm curious what you think of this.

Would you mind changing the first letter to lower case? There is a convention we do that. (I know old message didnt' follow the convention…)

We have another unstable --precise <pre-release>, which we may want to allow here?

Lower casing fixed. Awaiting feedback on the error case.

Why is this an error?

• manifest points to 0.1
• 0.2.0-beta is published
• user does cargo update --precise 0.2.0-beta

We gave it a valid version that it a breaking change from the current one, so it seems like we should just use that as our version requirement.

The fact that the error doesn't make sense from the users perspective ("I never said the version req was ^0.2 so why is it trying that?") I think helps highlight that.

Looks like its only an error because the user's req was 0.1 instead of 0.1.0? In that case, I think its reasonable for us to act as if it was 0.1.0.

This needs more looking into. A test is failing. I'll also move this and related changes to a separate commit.

Two commits inserted at the beginning of the PR.

nit: Context is a name so general that may conflict with other structs.

Fixed.

There is discrepancy between the normal update and breaking update.

• With and without v prefix
• Show SemVer requirement operator or not.

Not a blocker for this PR, but we might want track this somewhere.

This was discussed here: #13979 (comment)

I'll add it to the task list.

What's the motivation behind 45bc28c?

In this test (update_breaking_spec_version_transitive) I wanted to specifically target foo.dep but not bar.dep. I can do that if foo.dep=1.0 and bar.dep=2.0:

    p.cargo("update -Zunstable-options --breaking dep@1.0")
        .with_stderr_data(str![[r#"
[UPGRADING] dep ^1.0 -> ^3.0
[UPDATING] dep v1.0.0 -> v3.0.0

But not if bar.dep=1.1, because that makes both foo.dep and bar.dep resolve to 1.1.

-p is not needed anymore :)

Fixed.

[..] glob is not necessary.

Fixed.

Oh no this error message is bad 😢

Keep in mind this is without the feature implemented. In the next commit this becomes:

[UPGRADING] pre ^1.0.0-alpha -> ^2.0.0
[UPDATING] `dummy-registry` index
[UPDATING] pre v1.0.0-alpha -> v2.0.0

Could we document this type alias? It's not immediately clear what should be included in this HashMap. This LockedMap is a good reference:

Fixed.

I don't think this is an appropriate place to be putting this fix.

Note the comment in the description

we're not sure if this part of the functionality should be implemented in semver or cargo.

At this time, this is an abstraction that we shouldn't be breaking with cargo-specific logic

btw these two commtis seem unrelated with the rest and seem like they should be their own PR

Hmm, but all I'm doing here is relaxing the cargo-specific logic. Without this fix, I get this bug:

   Upgrading pre ^0.1.0 -> ^0.2.0-beta
    Updating `dummy-registry` index
error: failed to select a version for the requirement `pre = "^0.2.0-beta"`
candidate versions found which didn't match: 0.2.0-beta

What do you think I should do?

If we take 0.2.0-beta, remove -beta, we get 0.2.0. That seems like 0.2.0-beta should be able to match that.

Any insight into why this isn't working?

This is a bug for this PR.

matches_prerelease will alaways a OptVersionReq::Req in my previous PR, but this PR will add OptVersionReq::Precise to match , see

    let req = if precise.is_some() {
        OptVersionReq::Precise(new_version, new_version_req)
    } else {
        OptVersionReq::Req(new_version_req)
    };

This seems to incorrectly be interacting with the precise-prerelease feature. The intention of that feature is that we should occasionally allow matching pre-releases with a regular version requirement.

update_precise_breaking_consistent_output

Consistent with what? How will this ensure we keep remaining consistent?

update_precise_breaking_spec_version

I'm confused as to what this test is trying to capture compared to other tests

Oh, its not that there is no spec but that we aren't specifying a version in the spec, only the name

Does cargo update --precise <no-meta> work on packages with metadata? Whichever way, this should do the same if its a breaking change

@torhovland I was very intentionally trying to avoid changing this in past PRs. To go back and change it requires a good explanation. Even better if you can also split this into its own commit or PR

@Eh2406 I'm very cautious of messing with keep and would appreciate your input on tweaking it for this case

keep also makes my head spin. I feel like it's something about the triple negative structure, we update the ones that are not keep, we keep the ones that are not avoid, we avoid the things that asked to be updated. But I've never found it clearer naming structure, so maybe that's not it. It's also trying to do a linear prediction of resolution which feels like an approximation.

In specific, why does this need to be a separate lookup as opposed to adding the matching packages to to_avoid?

Also, this callback is run a lot on no-op builds, so very small performance problems end up being a high percentage of runtime. I suspect the to_string is going to end up being expensive. Can the upgrades map be InternedStrings or &str?

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.

I'm pulling this out into a separate PR: #14259

I assume with that pulled out this PR will be updated so we can get it merged without #14259 so this doesn't get blocked on that decision?

OK, I suppose we can do that. It will help illustrate why #14259 is useful.

I don't like the idea of not building on #14259. I suggest we rather let this PR depend on #14259 and leave it as draft for now. Here's why:

• Some of the duplicated existing precise tests with the unstable feature enabled are no longer passing. In other words, without #14259 we will be changing the behaviour of the non-breaking precise update.

• Some invocations are now silently finishing rather than reporting an error. For example:

[ERROR] package ID specification `incompatible@2.0.0` did not match any packages
Did you mean one of these?

  incompatible@0.3.1

• The output is generally no longer consistent with the non-breaking update. For example, without #14259 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)

I don't like the idea of not building on #14259. I suggest we rather let this PR depend on #14259 and leave it as draft for now.

That PR has behavior changes that have not been agreed to, internal and user-facing. We should not block progress forward on something on changes like that. If there is some intermediate change that side steps the controversial parts that unblocks --precise, then let's go for that.

Is #14259 still controversial? It no longer changes keep. I hesitate moving forward with this PR without merging #14259 first, particularly because of the first bullet point above (breaking compatibility with the existing precise update).

I'm not seeing this error covered in the tests

Why did the style for Upgrading change?

With my other comment in mind, is there a reason this should be a bail, rather than an assert?

@epage Can I ask you to take a quick look at this test, please? I believe it answers the concern you raised in office hours. The test demonstrates that we do support upgrading a direct dependency while leaving the transitive dependency at its original version.

If this is reassuring to you, it means #14259 is a valid step forward and we should get it merged. Then I'll clean up this PR.