More `update --breaking` tests by torhovland · Pull Request #14049 · rust-lang/cargo

torhovland · 2024-06-12T07:37:57Z

rustbot · 2024-06-12T07:38:01Z

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-06-12T07:38:34Z

r? epage

tests/testsuite/update.rs

torhovland · 2024-06-12T09:44:21Z

tests/testsuite/update.rs

+    p.cargo("update -Zunstable-options --breaking incompatible@1.0.0")
+        .masquerade_as_nightly_cargo(&["update-breaking"])
+        .with_stderr(
+            // FIXME: This is wrong.


I found another problem. We're not matching incompatible with incompatible@1.0.0.

But maybe that's not a bug. Strictly speaking, 1.0.0 does not match the version requirement 1.0.

Anyway, I pushed a fix.

Think of this feature as a specially named --force flag. They identified the entry they want to update so we should.

What we also need a test for is if they specify a compatible version that isn't in the lockfile.

I'm actually not using the lock file. This should work whether or not there is a lock file. I'm just checking if the version matches the version req.

Thats an implementation detail while I'm talking of user-facing semantics. We should match spec behavior with cargo update without --breaking.

Imagine a non-existing compatible@1.0.10. When run without a lock file, cargo update compatible@1.0.10 does not complain. Then, if the same command is run again (now with a lock file), it complains about compatible@1.0.10 not matching any packages. So it is not idempotent.

Is it OK if cargo update --breaking compatible@1.0.10 also doesn't complain, whether or not there is a lock file?

If not OK, we'll have to query a previous_resolve just for this error message. And it might be a good idea to go back to reusing the ops::update_lockfile that the non-breaking update is running (but then having to make changes to keep). In any case, can we put that in a separate PR?

@epage This may or may not be a task to put on the task list, but it depends on your answer to the question about cargo update --breaking compatible@1.0.10 not complaining.

Let's put it in the task list either way and not block this conversation further.

Done.

FYI, this is changed in #14140. If there isn't a lock file, update -Zunstable-options --breaking incompatible@9.0.0 will do this:

[UPDATING] `dummy-registry` index [LOCKING] 3 packages to latest compatible versions [ADDING] incompatible v1.0.1 (latest: v2.0.0)

If there is a lock file, it will do this:

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

src/cargo/ops/cargo_update.rs

tests/testsuite/update.rs

torhovland · 2024-06-19T13:49:24Z

@epage I found another bug, I think. See the latest commit: 901f78e

epage · 2024-06-19T21:15:46Z

If I'm understanding, thats a question of whether non-workspace path dependencies should be affected by this (and whether we should error if nothing matched).

I think this makes a good case for erroring. Our mental model is this is a specialized force, so it makes sense the lockfile can update when others can't if we error.

As for local deps like this, we should make this a task in the tracking issue to verify the general approach of cargo wrt mutations (cargo add, cargo fix, etc).

torhovland · 2024-06-20T07:19:49Z

Looks like this is actually consistent. We can not run cargo add on non-ws crates. When running cargo fix only ws members are being fixed. And cargo-edit also does not make upgrades in non-ws crates.

torhovland · 2024-06-20T07:42:16Z

@epage I consider this PR ready on my part now. Awaiting further review comments.

weihanglo · 2024-06-21T12:17:25Z

While #14115 is not about bumping SemVer-breaking versions, this behavior described in #14115 (comment) could be a thing to consider supporting or adding a test: cargo update <pkg> --breaking shouldn't touch other unrelated packages from the same source.

tests/testsuite/update.rs

epage · 2024-06-21T13:42:35Z

tests/testsuite/update.rs

+/// keyed on dependency name and source, we also need to know which manifest
+/// files to update and which to keep unchanged, because they may be pinned or


this feels weird for the test to be prescriptive on the implementation in a comment

This isn't right. Its not just files but individual dependencies. You could have renamed in one dependency block but not another

I removed that part of the comment, moved the rest (updated) to the implementation, and added a test case for mixed pinning within the same manifest.

The comment removal was done in a later commit. Should it be done in the initial commit?

src/cargo/ops/cargo_update.rs

tests/testsuite/update.rs

epage · 2024-06-27T02:53:57Z

tests/testsuite/update.rs

+    let root_manifest = p.read_file("Cargo.toml");
+    let foo_manifest = p.read_file("foo/Cargo.toml");
+    let bar_manifest = p.read_file("bar/Cargo.toml");
+    let baz_manifest = p.read_file("baz/Cargo.toml");
+
+    assert_e2e().eq(


nit: Personally, I'd read next to use to keep knowledge as local as possible

epage · 2024-06-27T02:55:36Z

tests/testsuite/update.rs

+    let foo_manifest = p.read_file("foo/Cargo.toml");
+    let bar_manifest = p.read_file("bar/Cargo.toml");
+    let baz_manifest = p.read_file("baz/Cargo.toml");


Would it work to give these workspace members names tied to their role in the test? For example, they can be "pinned", "unpinned", "mixed-pinned". That'd make it easier to follow what the test is doing

torhovland · 2024-06-27T07:06:33Z

@epage Ready for review again.

torhovland · 2024-06-27T07:17:51Z

@epage Sorry, my rebasing on master probably made the force push diff unreadable for you.

But all I changed was deleting that long comment in the test commit, and applying your suggestions here.

epage · 2024-06-27T10:34:29Z

Thanks!

@bors r+

bors · 2024-06-27T10:34:31Z

📌 Commit f41bdc1 has been approved by epage

It is now in the queue for this repository.

bors · 2024-06-27T10:35:39Z

⌛ Testing commit f41bdc1 with merge 2607661...

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

tests/testsuite/update.rs

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


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)

bors · 2024-06-27T11:02:56Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 2607661 to master...

Update cargo 23 commits in 4ed7bee47f7dd4416b36fada1909e9a62c546246..a515d463427b3912ec0365d106791f88c1c14e1b 2024-06-25 16:28:22 +0000 to 2024-07-02 20:53:36 +0000 - test: migrate rust_version and rustc* to snapbox (rust-lang/cargo#14177) - test: mirgate fix* and future_incompat_report to snapbox (rust-lang/cargo#14173) - test：migrate `edition/error` to snapbox (rust-lang/cargo#14175) - chore(deps): update compatible (rust-lang/cargo#14174) - refactor(source): Clean up after PathSource/RecursivePathSource split (rust-lang/cargo#14169) - test: Migrate some files to snapbox (rust-lang/cargo#14132) - test: fix several assertions (rust-lang/cargo#14167) - test: replace glob with explicit unordered calls (rust-lang/cargo#14166) - Make it clear that `CARGO_CFG_TARGET_FAMILY` is multi-valued (rust-lang/cargo#14165) - Document `CARGO_CFG_TARGET_ABI` (rust-lang/cargo#14164) - test: Migrate git to snapbox (rust-lang/cargo#14159) - test: migrate some files to snapbox (rust-lang/cargo#14158) - test: migrate registry and registry_auth to snapbox (rust-lang/cargo#14149) - gix: remove `revision` feature from cargo (rust-lang/cargo#14160) - test: migrate package* and publish* to snapbox (rust-lang/cargo#14130) - More `update --breaking` tests (rust-lang/cargo#14049) - test: migrate clean to snapbox (rust-lang/cargo#14096) - Allow `unexpected_builtin_cfgs` lint in `user_specific_cfgs` test (rust-lang/cargo#14153) - test: migrate search, source_replacement and standard_lib to snapbox (rust-lang/cargo#14151) - Docs: Update config summary to include missing keys. (rust-lang/cargo#14145) - test: migrate `dep_info/diagnostics/direct_minimal_versions` to snapbox (rust-lang/cargo#14143) - Docs: Remove duplicate `strip` section. (rust-lang/cargo#14146) - Docs: Fix curly quotes in config docs. (rust-lang/cargo#14144)

…te-breaking, r=weihanglo Improved error message when `update --breaking` invalid spec. Improves an error message when trying to do `cargo update --breaking clap@foo`: ``` - [ERROR] expected a version like "1.32" + [ERROR] invalid package ID specification: `clap@foo` + Caused by: + expected a version like "1.32" ``` Related to #12425. Fixes an item [here](#12425 (comment)), as noted [here](#14049 (comment)).

rustbot assigned weihanglo Jun 12, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2024

rustbot assigned epage and unassigned weihanglo Jun 12, 2024

torhovland force-pushed the issue-12425-more-tests branch 2 times, most recently from 372ff8f to 7bdf956 Compare June 12, 2024 08:09

torhovland commented Jun 12, 2024

View reviewed changes

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

torhovland force-pushed the issue-12425-more-tests branch from 7bdf956 to 3a45d6e Compare June 12, 2024 09:42

torhovland commented Jun 12, 2024

View reviewed changes

rustbot added the Command-update label Jun 12, 2024

epage reviewed Jun 12, 2024

View reviewed changes

src/cargo/ops/cargo_update.rs Show resolved Hide resolved

tests/testsuite/update.rs Show resolved Hide resolved

torhovland mentioned this pull request Jun 19, 2024

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

Open

12 tasks

torhovland force-pushed the issue-12425-more-tests branch from 2de4c24 to b3a1a50 Compare June 19, 2024 10:24

torhovland force-pushed the issue-12425-more-tests branch from 901f78e to 0d59dfc Compare June 20, 2024 07:38

torhovland force-pushed the issue-12425-more-tests branch from 0d59dfc to 28f57be Compare June 20, 2024 10:33

rustbot added the A-manifest Area: Cargo.toml issues label Jun 20, 2024

torhovland force-pushed the issue-12425-more-tests branch from 28f57be to 70c01f9 Compare June 20, 2024 10:46

epage reviewed Jun 21, 2024

View reviewed changes

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

epage reviewed Jun 21, 2024

View reviewed changes

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

epage reviewed Jun 21, 2024

View reviewed changes

src/cargo/ops/cargo_update.rs Show resolved Hide resolved

epage reviewed Jun 21, 2024

View reviewed changes

src/cargo/ops/cargo_update.rs Show resolved Hide resolved

torhovland force-pushed the issue-12425-more-tests branch from 70c01f9 to 78e10b7 Compare June 24, 2024 10:12

torhovland force-pushed the issue-12425-more-tests branch from 2dcc815 to 690f6a0 Compare June 24, 2024 11:59

torhovland mentioned this pull request Jun 25, 2024

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

Draft

epage reviewed Jun 27, 2024

View reviewed changes

tests/testsuite/update.rs Show resolved Hide resolved

epage reviewed Jun 27, 2024

View reviewed changes

tests/testsuite/update.rs Show resolved Hide resolved

epage reviewed Jun 27, 2024

View reviewed changes

tests/testsuite/update.rs Show resolved Hide resolved

epage reviewed Jun 27, 2024

View reviewed changes

torhovland force-pushed the issue-12425-more-tests branch 2 times, most recently from e9fd85a to 5434977 Compare June 27, 2024 06:45

torhovland added 4 commits June 27, 2024 09:12

refactor: Clean up test output and tracing, and introduce variable.

abc86e2

test: More update --breaking tests.

8882d05

fix: update --breaking now understands package@version.

f096a94

fix: update --breaking now handles mixed renaming and pinning.

f41bdc1

torhovland force-pushed the issue-12425-more-tests branch from 5434977 to f41bdc1 Compare June 27, 2024 07:12

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2024

epage reviewed Jun 27, 2024

View reviewed changes

bors merged commit 2607661 into rust-lang:master Jun 27, 2024

weihanglo mentioned this pull request Jul 3, 2024

Update cargo rust-lang/rust#127258

Merged

rustbot added this to the 1.81.0 milestone Jul 3, 2024

torhovland mentioned this pull request Jul 21, 2024

Improved error message when update --breaking invalid spec. #14279

Merged

		/// keyed on dependency name and source, we also need to know which manifest
		/// files to update and which to keep unchanged, because they may be pinned or

Conversation

torhovland commented Jun 12, 2024

Uh oh!

rustbot commented Jun 12, 2024

Uh oh!

torhovland commented Jun 12, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torhovland Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

torhovland commented Jun 19, 2024

Uh oh!

epage commented Jun 19, 2024

Uh oh!

torhovland commented Jun 20, 2024

Uh oh!

torhovland commented Jun 20, 2024

Uh oh!

weihanglo commented Jun 21, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torhovland commented Jun 27, 2024

Uh oh!

torhovland commented Jun 27, 2024

Uh oh!

epage commented Jun 27, 2024

Uh oh!

bors commented Jun 27, 2024

Uh oh!

bors commented Jun 27, 2024

Uh oh!

epage Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

torhovland Jun 27, 2024 •

edited

Loading

epage Jun 27, 2024 •

edited

Loading