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.

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

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)

epage · 2024-06-27T10:47:14Z

src/cargo/core/features.rs

@@ -786,6 +786,7 @@ unstable_cli_options!(
    target_applies_to_host: bool = ("Enable the `target-applies-to-host` key in the .cargo/config.toml file"),
    trim_paths: bool = ("Enable the `trim-paths` option in profiles"),
    unstable_options: bool = ("Allow the usage of unstable options"),
+    update_precise_breaking: bool = ("Allow `update --precise` to do breaking upgrades"),


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.

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.

tests/testsuite/update.rs

epage · 2024-06-27T10:53:22Z

tests/testsuite/update.rs

+        // The lockfile update is failing for the same reason a non-breaking
+        // update would. There is still a [email protected] here that didn't get
+        // upgraded (the transitive dependency within bar), and we are now
+        // asking the lockfile update to update it to 0.2.0, which it cannot do.
        .with_stderr_data(str![[r#"


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.

epage · 2024-06-27T10:57:49Z

tests/testsuite/update.rs

        .with_stderr_data(str![[r#"
-[ERROR] unknown `-Z` flag specified: update-precise-breaking
-
-For available unstable features, see https://doc.rust-lang.org/nightly/cargo/reference/unstable.html
-If you intended to use an unstable rustc feature, try setting `RUSTFLAGS="-Zupdate-precise-breaking"`
+[UPDATING] `dummy-registry` index
+[ERROR] failed to select a version for the requirement `pinned = "=0.1"`
+candidate versions found which didn't match: 0.2.0


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.

epage · 2024-06-27T10:59:59Z

tests/testsuite/update.rs

+#[cargo_test]
+fn update_precise_breaking_incompatible() {


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 [email protected] --precise 0.3.0") .with_stderr_data(str![[r#" [UPGRADING] incompatible ^0.1 -> ^0.3 [UPDATING] incompatible v0.1.0 -> v0.3.0

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.

epage · 2024-06-27T11:03:34Z

tests/testsuite/update.rs

+        .masquerade_as_nightly_cargo(&[])
+        .with_status(101)
+        .with_stderr_data(str![[r#"
+[ERROR] expected a version like "1.32"


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.

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-06-28T06:30:26Z

src/bin/cargo/commands/update.rs

-            .fail_if_stable_opt("--breaking", 12425)?;
-
-        let upgrades = ops::upgrade_manifests(&mut ws, &update_opts.to_update)?;
-        ops::resolve_ws(&ws, update_opts.dry_run)?;


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.

torhovland · 2024-06-28T12:07:01Z

tests/testsuite/update.rs

-if you are looking for the prerelease package it needs to be specified explicitly
-    pre = { version = "0.2.0-beta" }
-perhaps a crate was updated and forgotten to be re-vendored?
+[ERROR] New requirement ^0.2 is invalid, because it doesn't match 0.2.0-beta


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

torhovland · 2024-06-28T14:17:58Z

src/cargo/util/semver_ext.rs

-            return self.matches(&version);
+            let mut version_cleaned = version.clone();
+            version_cleaned.pre = semver::Prerelease::EMPTY;
+            return self.matches(&version) || self.matches(&version_cleaned);


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.

weihanglo · 2024-06-28T13:55:47Z

tests/testsuite/update.rs

-if you are looking for the prerelease package it needs to be specified explicitly
-    pre = { version = "0.2.0-beta" }
-perhaps a crate was updated and forgotten to be re-vendored?
+[ERROR] New requirement ^0.2 is invalid, because it doesn't match 0.2.0-beta


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…)

weihanglo · 2024-06-28T13:58:44Z

tests/testsuite/update.rs

-if you are looking for the prerelease package it needs to be specified explicitly
-    pre = { version = "0.2.0-beta" }
-perhaps a crate was updated and forgotten to be re-vendored?
+[ERROR] New requirement ^0.2 is invalid, because it doesn't match 0.2.0-beta


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

weihanglo · 2024-06-28T14:05:39Z

src/cargo/ops/cargo_update.rs

@@ -14,6 +14,7 @@ use crate::util::toml_mut::manifest::LocalManifest;
 use crate::util::toml_mut::upgrade::upgrade_requirement;
 use crate::util::{style, OptVersionReq};
 use crate::util::{CargoResult, VersionExt};
+use anyhow::Context;


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

Suggested change

use anyhow::Context;

use anyhow::Context as _;

weihanglo · 2024-06-28T14:16:16Z

tests/testsuite/update.rs


    // Will upgrade the direct dependency
    p.cargo("update -Zunstable-options --breaking [email protected]")
        .masquerade_as_nightly_cargo(&["update-breaking"])
        .with_stderr_data(str![[r#"
 [UPDATING] `[..]` index
-[UPGRADING] dep ^1.0 -> ^2.0
+[UPGRADING] dep ^1.0 -> ^3.0


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.

weihanglo · 2024-06-28T14:17:56Z

tests/testsuite/update.rs

            "#,
        )
        .file("bar/src/lib.rs", "")
        .build();

    p.cargo("generate-lockfile").run();

-    Package::new("dep", "1.1.1").publish();
-    Package::new("dep", "2.0.0").publish();
+    Package::new("dep", "2.0.1").publish();


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 [email protected]") .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.

weihanglo · 2024-06-28T14:24:48Z

tests/testsuite/update.rs

+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("update -p incompatible --precise 2.0.0")


-p is not needed anymore :)

weihanglo · 2024-06-28T14:26:01Z

tests/testsuite/update.rs

+[ERROR] failed to select a version for the requirement `renamed-from = "^1.0"`
+candidate versions found which didn't match: 2.0.0
+location searched: `dummy-registry` index (which is replacing registry `crates-io`)
+required by package `foo v0.0.1 ([..]/foo)`


[..] glob is not necessary.

weihanglo · 2024-06-28T14:28:55Z

tests/testsuite/update.rs

+candidate versions found which didn't match: 2.0.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?


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

weihanglo · 2024-06-28T14:39:29Z

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)]
+


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 😬)

weihanglo · 2024-06-28T14:42:17Z

src/cargo/ops/cargo_update.rs

+pub fn update_lockfile(
+    ws: &Workspace<'_>,
+    opts: &UpdateOptions<'_>,
+    upgrades: &UpgradeMap,


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

cargo/src/cargo/core/registry.rs

Lines 136 to 154 in 4403332

/// A map of all "locked packages" which is filled in when parsing a lock file

/// and is used to guide dependency resolution by altering summaries as they're

/// queried from this source.

///

/// This map can be thought of as a glorified `Vec<MySummary>` where `MySummary`

/// has a `PackageId` for which package it represents as well as a list of

/// `PackageId` for the resolved dependencies. The hash map is otherwise

/// structured though for easy access throughout this registry.

type LockedMap = HashMap<

// The first level of key-ing done in this hash map is the source that

// dependencies come from, identified by a `SourceId`.

// The next level is keyed by the name of the package...

(SourceId, InternedString),

// ... and the value here is a list of tuples. The first element of each

// tuple is a package which has the source/name used to get to this

// point. The second element of each tuple is the list of locked

// dependencies that the first element has.

Vec<(PackageId, Vec<PackageId>)>,

>;

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.

…his will test backward compatibility when the capability of allowing breaking changes has been implemented.

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

This is ready for another look now.

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

epage reviewed Jun 27, 2024

View reviewed changes

tests/testsuite/update.rs Outdated Show resolved Hide resolved

epage reviewed Jun 27, 2024

View reviewed changes

tests/testsuite/update.rs Outdated Show resolved Hide resolved

epage reviewed Jun 27, 2024

View reviewed changes

torhovland force-pushed the update-precise branch from 5e5b13d to b8a45a2 Compare June 27, 2024 11:39

torhovland force-pushed the update-precise branch 2 times, most recently from 0097bf8 to 93c2ede Compare June 27, 2024 13:16

torhovland commented Jun 28, 2024

View reviewed changes

torhovland force-pushed the update-precise branch from 93c2ede to d21785d Compare June 28, 2024 07:01

torhovland changed the title ~~Add update-precise-breaking feature.~~ Add capability of making breaking changes in update --precise Jun 28, 2024

torhovland force-pushed the update-precise branch from d21785d to d7ca2a9 Compare June 28, 2024 11:49

rustbot added the A-semver Area: semver specifications, version matching, etc. label Jun 28, 2024

torhovland force-pushed the update-precise branch 2 times, most recently from 95dddc4 to 48dc39e Compare June 28, 2024 12:04

torhovland commented Jun 28, 2024

View reviewed changes

weihanglo reviewed Jun 28, 2024

View reviewed changes

torhovland force-pushed the update-precise branch from 48dc39e to eeff572 Compare July 1, 2024 11:36

torhovland added 6 commits July 1, 2024 13:36

test: Demonstrate a problem with matching precise prerelease versions.

13abe15

fix: Matching of precise prerelease versions.

de1a4a2

test: Adjust update-breaking tests.

f01fb3b

test: Duplicate update precise tests with unstable-options enabled. T…

b9e3207

…his will test backward compatibility when the capability of allowing breaking changes has been implemented.

test: Tests for allowing breaking updates in cargo update --precise.

42bdec1

feat: Make cargo update --precise capable of doing breaking upgrades.

f57599c

torhovland force-pushed the update-precise branch from eeff572 to f57599c Compare July 1, 2024 11:37

	/// A map of all "locked packages" which is filled in when parsing a lock file
	/// and is used to guide dependency resolution by altering summaries as they're
	/// queried from this source.
	///
	/// This map can be thought of as a glorified `Vec<MySummary>` where `MySummary`
	/// has a `PackageId` for which package it represents as well as a list of
	/// `PackageId` for the resolved dependencies. The hash map is otherwise
	/// structured though for easy access throughout this registry.
	type LockedMap = HashMap<
	// The first level of key-ing done in this hash map is the source that
	// dependencies come from, identified by a `SourceId`.
	// The next level is keyed by the name of the package...
	(SourceId, InternedString),
	// ... and the value here is a list of tuples. The first element of each
	// tuple is a package which has the source/name used to get to this
	// point. The second element of each tuple is the list of locked
	// dependencies that the first element has.
	Vec<(PackageId, Vec<PackageId>)>,
	>;

		#[cargo_test]
		fn update_precise_breaking_incompatible() {

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

Are you sure you want to change the base?

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

Conversation

torhovland commented Jun 25, 2024 • edited Loading

rustbot commented Jun 25, 2024

weihanglo commented Jun 26, 2024

torhovland commented Jun 26, 2024

epage commented Jun 27, 2024

epage commented Jun 27, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torhovland commented Jun 27, 2024

epage commented Jun 27, 2024

bors commented Jun 27, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torhovland commented Jul 1, 2024

torhovland commented Jul 1, 2024

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

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

torhovland commented Jun 25, 2024 •

edited

Loading