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

torhovland · 2024-06-25T09:09:08Z

Implements the second half of #12425.

With the unstable-options feature enabled, cargo update --precise will allow making upgrades/downgrades that require changes to be made to the manifest files, similar to cargo update --breaking.

Blocked on #14259. See https://github.com/rust-lang/cargo/pull/14140/files#r1682381732.

rustbot · 2024-06-25T09:09:13Z

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

weihanglo · 2024-06-26T03:25:46Z

Should we wait for #14049, or this can be reviewed independently?

torhovland · 2024-06-26T06:46:27Z

The 4 new commits here can be reviewed. I think #14049 is close to being merged anyway.

epage · 2024-06-27T10:37:36Z

The 4 new commits here can be reviewed. I think #14049 is close to being merged anyway.

In the future, please make a blocking thing like that explicit either by making this a draft or opening an issue on an arbitrary part of the code.

tests/testsuite/update.rs

epage · 2024-06-27T10:44:41Z

In #12425 (comment), this PR was marked as addressing

Consider an error message if the command completed without doing any upgrades.

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

(btw probably best not to mark things as done when they aren't merged as things can change especially when the "solution" is not decided yet)

src/cargo/core/features.rs

epage · 2024-06-27T10:50:25Z

tests/testsuite/update_duplicated_with_precise_breaking_feature.rs

+//! Duplicating tests for `cargo update --precise` with the
+//! update-precise-breaking feature enabled. When the feature is stabilized,
+//! this file can be deleted.
+
+#![allow(deprecated)]
+


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.

tests/testsuite/update.rs

epage · 2024-06-27T11:02:28Z

src/bin/cargo/commands/update.rs

@@ -1,8 +1,11 @@
+use std::collections::HashMap;


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

tests/testsuite/update.rs

torhovland · 2024-06-27T11:04:25Z

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.

(btw probably best not to mark things as done when they aren't merged as things can change especially when the "solution" is not decided yet)

Got it.

epage · 2024-06-27T11:08:50Z

Description updated.

I'm still not seeing the description talk to

why an error is not the way forward

btw it would be good to focus on user impact and not implementation. The paragraph that alludes to this starts off by discussing the implementation which someone is likely to gloss over when looking for user impact.

bors · 2024-06-27T11:17:14Z

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

torhovland · 2024-07-01T10:00:43Z

Description updated.

I'm still not seeing the description talk to

why an error is not the way forward

I have updated the task list. Hope that's OK.

torhovland · 2024-07-01T12:03:36Z

This is ready for another look now.

epage · 2024-07-12T19:56:36Z

src/cargo/util/semver_ext.rs

+            // Only in the case of "ordinary" version requirements with pre-release
+            // versions do we need to help the version matching. In the case of Any,
+            // Locked, or Precise, the `matches()` function is already doing the
+            // correct handling.
+            if let OptVersionReq::Req(_) = self {
+                let mut version = version.clone();
+                version.pre = semver::Prerelease::EMPTY;
+                return self.matches(&version);
+            }


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) };

epage · 2024-07-12T20:20:58Z

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.

epage · 2024-07-12T20:26:52Z

tests/testsuite/precise_pre_release.rs

    p.cargo("update my-dependency --precise 0.1.2-pre.0 -Zunstable-options")
        .masquerade_as_nightly_cargo(&["precise-pre-release"])
        .with_stderr_data(str![[r#"
+[UPGRADING] my-dependency ^0.1.1 -> ^0.1.2-pre.0
 [UPDATING] `dummy-registry` index
-[UPDATING] my-dependency v0.1.1 -> v0.1.2-pre.0

 "#]])


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.

epage · 2024-07-12T20:27:59Z

tests/testsuite/update.rs

    p.cargo("update -Zunstable-options incompatible --precise 0.2.0")
        .masquerade_as_nightly_cargo(&["update-precise-breaking"])
-        .with_status(101)
        .with_stderr_data(str![[r#"
+[UPGRADING] incompatible ^0.1 -> ^0.2
 [UPDATING] `dummy-registry` index
-[ERROR] failed to select a version for the requirement `incompatible = "^0.1"`
-candidate versions found which didn't match: 0.2.0
-location searched: `dummy-registry` index (which is replacing registry `crates-io`)
-required by package `foo v0.0.1 ([ROOT]/foo)`
-perhaps a crate was updated and forgotten to be re-vendored?
+[UPDATING] incompatible v0.1.0 -> v0.2.0
+[NOTE] pass `--verbose` to see 1 unchanged dependencies behind latest


update_precise_breaking_consistent_output

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

epage · 2024-07-12T20:36:12Z

tests/testsuite/update.rs

    // No spec
    p.cargo("update -Zunstable-options incompatible --precise 0.3.1")


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

epage · 2024-07-12T20:41:28Z

tests/testsuite/update.rs

@@ -2693,14 +2648,10 @@ fn update_precise_breaking_incompatible_build_metadata() {
    p.cargo("generate-lockfile").run();
    p.cargo("update -Zunstable-options incompatible --precise 2.0.0+b")


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

epage · 2024-07-12T20:51:09Z

src/cargo/ops/cargo_update.rs

+    let breaking_precise_upgrades = opts.precise.is_some() && !upgrades.is_empty();
+    let breaking_mode = opts.breaking || breaking_precise_upgrades;
+
+    let keep = |p: &PackageId| {
+        (!to_avoid_sources.contains(&p.source_id()) && !to_avoid.contains(p))
+            // In case of `--breaking` or precise upgrades, we want to keep all
+            // packages unchanged that didn't get upgraded.
+                || (breaking_mode && !upgrades.contains_key(&(p.name().to_string(), p.source_id())))
+    };


@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 Improved consistency between breaking and non-breaking updates #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 `[email protected]` did not match any packages Did you mean one of these? [email protected]

The output is generally no longer consistent with the non-breaking update. For example, without Improved consistency between breaking and non-breaking updates #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).

epage · 2024-07-12T20:52:32Z

src/cargo/ops/cargo_update.rs

+        Version::parse(precise)
+            .with_context(|| format!("invalid version format for precise version `{precise}`"))?


I'm not seeing this error covered in the tests

epage · 2024-07-12T20:54:06Z

src/cargo/ops/cargo_update.rs

+    let old_version = semver::Version::new(
+        comparator.major,
+        comparator.minor.unwrap_or_default(),
+        comparator.patch.unwrap_or_default(),
+    );
+    let is_downgrade = new_version < old_version;
+    let status = if is_downgrade {
+        "Downgrading"
+    } else {
+        "Upgrading"
+    };
+
    if upgrade_messages.insert(upgrade_message.clone()) {
        gctx.shell()
-            .status_with_color("Upgrading", &upgrade_message, &style::GOOD)?;
+            .status_with_color(status, &upgrade_message, &style::WARN)?;
    }


Why did the style for Upgrading change?

epage · 2024-07-12T20:55:38Z

src/cargo/util/toml_mut/upgrade.rs

        // Validate contract
-        #[cfg(debug_assertions)]
-        {
-            assert!(


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

torhovland · 2024-08-16T10:57:39Z

tests/testsuite/update.rs

+}
+
+#[cargo_test]
+fn update_precise_breaking_direct_plus_transitive() {


@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.

fix: Limiting pre-release match semantics to use only on `OptVersionReq::Req` ### What does this PR try to resolve? The prerelease matches semantics should be only used on `OptVersionReq::Req`, but it dosn't. The other `OptVersionReq` types have specify matches logic already, for example a `OptVersionReq::Precise` will failed on `matches_prerelease`, see #14140 (comment) ### How should we test and review this PR? the first commit added the test, the second commit updated the test and fixed the issue. ### Additional information

rustbot assigned weihanglo Jun 25, 2024

torhovland force-pushed the update-precise branch 3 times, most recently from 7b753cf to 5e5b13d Compare June 25, 2024 14:49

This was referenced Jun 27, 2024

More update --breaking tests #14049

Merged

A way for users to bulk upgrade across incompatible versions #12425

Open