fix(add): Improve error when adding registry packages while vendored #13281

LuuuXXX · 2024-01-11T11:18:30Z

What does this PR try to resolve?

When a vendored directory is established, cargo add no longer adds new packages. Instead, it tries to translate a package name into a package that already exists in the vendored directory.
More details

Since @epage has done most of the work, here I do the rest of the finishing work.

Improves the error from #10729

How should we test and review this PR?

The implementation procedure is as follows:
#10729 (comment)

Test steps：

Try to get an arbitrary crate and execute cargo vendor command.
Configure the vendor directory in .cargo/config.toml.
Add alter-registry to the config.toml file.

[registries]
alter-registry= { index = "XXX" }

run the same cargo add command.

cargo add another-crate --registry alter-registry

rustbot · 2024-01-11T11:18:35Z

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

epage · 2024-01-12T22:10:33Z

I recommend re-arranging the commits so the first commit shows the current behavior via a test and then the commit with the fix updates the test. This helps make clear to the reviewers what the behavior change was and ensures we "test the test" (assuming you ran the tests).

I tried to do that with your branch and when i cherry-picked the test onto master, it passed.

LuuuXXX · 2024-01-14T08:25:06Z

I recommend re-arranging the commits so the first commit shows the current behavior via a test
Okay, revised the commit order

epage · 2024-01-15T16:17:12Z

Its not just about the commit order but that every commit should pass cargo test. In doing so, the commit that fixes the bug will need to be updating the test to reflect the new behavior.

LuuuXXX · 2024-01-17T01:06:35Z

Its not just about the commit order but that every commit should pass cargo test.

No problem, I've resubmitted the commits.

epage · 2024-01-17T16:30:01Z

The test is still the last commit.

The test should be the first commit and it should pass as-is. The follow up changes should change the tests as the behavior changes which will show that the behavior did change in the intended way and that the test tests the right thing. Last I looked, it appeared that the current tests do not test the right thing.

LuuuXXX · 2024-02-01T01:53:00Z

The test should be the first commit and it should pass as-is.

I reordered commits.

Last I looked, it appeared that the current tests do not test the right thing.

I've added two new tests,
One is to ensure that no package can be obtained from the vendor directory when a non-vendor package is added. (Throwing error in my opinion is the right thing to do.)
The other is to ensure that non-vendor packages can be obtained through alter registry.

epage · 2024-02-01T20:17:56Z

I reordered commits.

They were re-ordered but not updated so that every commit passes. I've pushed an update to your branch that does this.

tests/testsuite/cargo_add/add_no_vendored_package_with_alter_registry/mod.rs

LuuuXXX · 2024-02-20T03:37:54Z

Thanks, it's been squashed

epage · 2024-02-20T19:47:49Z

Thanks, it's been squashed

I was asking for the last commit to be squashed, not all of them.

LuuuXXX · 2024-02-21T07:09:12Z

my bad, I recreated the commit history with latest modifies

epage · 2024-02-21T18:29:53Z

As a reminder, each commit should pass tests.

LuuuXXX · 2024-02-22T06:23:19Z

yes yes, recreated again

epage · 2024-02-22T17:27:09Z

Thanks for all of the work you put into this and patience through the back and forth!

@bors r+

bors · 2024-02-22T17:27:12Z

📌 Commit 640c077 has been approved by epage

It is now in the queue for this repository.

bors · 2024-02-22T17:28:20Z

⌛ Testing commit 640c077 with merge e08a813...

bors · 2024-02-22T17:54:40Z

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

Update cargo 16 commits in 194a60b2952bd5d12ba15dd2577a97eed7d3c587..8964c8ccff6e420e2a38b8696d178d69fab84d9d 2024-02-21 01:53:45 +0000 to 2024-02-27 19:22:46 +0000 - feat: Add "-Zpublic-dependency" for public-dependency feature. (rust-lang/cargo#13340) - Stabilize global cache data tracking. (rust-lang/cargo#13492) - chore: bump baseline version requirement of sub crates (rust-lang/cargo#13494) - fix(doctest): search native libs in build script outputs (rust-lang/cargo#13490) - chore: fixed a typo(two->too) (rust-lang/cargo#13489) - test: relax help text assertion (rust-lang/cargo#13488) - refactor: clean up for `GlobalContext` rename (rust-lang/cargo#13486) - test(cli): Verify terminal styling (rust-lang/cargo#13461) - fix(cli): Respect CARGO_TERM_COLOR in '--list' and '-Zhelp' (rust-lang/cargo#13479) - Error messages when collecting workspace members now mention the workspace root location (rust-lang/cargo#13480) - fix(add): Improve error when adding registry packages while vendored (rust-lang/cargo#13281) - [docs]:Add missing jump links (rust-lang/cargo#13478) - Add global_cache_tracker stability tests. (rust-lang/cargo#13467) - fix(cli): Control clap colors through config (rust-lang/cargo#13463) - chore: remove the unused function (rust-lang/cargo#13472) - Fix missing brackets (rust-lang/cargo#13470)

rustbot assigned epage Jan 11, 2024

rustbot added A-dependency-resolution Area: dependency resolution and the resolver A-directory-source Area: directory sources (vendoring) A-registries Area: registries Command-add S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2024

LuuuXXX force-pushed the issue-10729 branch from 90a8c67 to ae6b820 Compare January 13, 2024 06:33

LuuuXXX force-pushed the issue-10729 branch from ae6b820 to 6d265a6 Compare January 16, 2024 06:22

rustbot added the A-manifest Area: Cargo.toml issues label Jan 16, 2024

LuuuXXX force-pushed the issue-10729 branch 2 times, most recently from cdb15d6 to b049570 Compare January 16, 2024 06:41

LuuuXXX changed the title ~~Fix: `cargo add refuses to add new non-vendored packages due to source replacement~~ [wip] Fix: `cargo add refuses to add new non-vendored packages due to source replacement Jan 22, 2024

LuuuXXX force-pushed the issue-10729 branch 6 times, most recently from 787a975 to 7b9d3f5 Compare January 31, 2024 09:10

LuuuXXX changed the title ~~[wip] Fix: `cargo add refuses to add new non-vendored packages due to source replacement~~ Fix: `cargo add refuses to add new non-vendored packages due to source replacement Feb 1, 2024

epage force-pushed the issue-10729 branch from 62baae4 to 5cc09ea Compare February 1, 2024 20:15

epage reviewed Feb 1, 2024

View reviewed changes

tests/testsuite/cargo_add/add_no_vendored_package_with_alter_registry/mod.rs Outdated Show resolved Hide resolved

epage changed the title ~~Fix: `cargo add refuses to add new non-vendored packages due to source replacement~~ fix(add): Improve error when adding registry packages while vendored Feb 1, 2024

LuuuXXX force-pushed the issue-10729 branch 2 times, most recently from 03705aa to e550de4 Compare February 20, 2024 03:32

LuuuXXX force-pushed the issue-10729 branch 5 times, most recently from 6f5cbe2 to 4e22e35 Compare February 21, 2024 06:09

Add two tests to make sure fixup

7dc7b1b

LuuuXXX force-pushed the issue-10729 branch from ae8bb98 to 7dc7b1b Compare February 22, 2024 04:34

Added QueryKind::Normalized, and used it in cargo-add

a31184b

LuuuXXX force-pushed the issue-10729 branch from 3b36aa8 to a31184b Compare February 22, 2024 05:37

Rename Fuzzy to Alternatives

640c077

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 Feb 22, 2024

bors merged commit e08a813 into rust-lang:master Feb 22, 2024
23 checks passed

weihanglo mentioned this pull request Feb 27, 2024

Update cargo rust-lang/rust#121699

Merged

weihanglo mentioned this pull request Mar 2, 2024

cargo add refuses to add new non-vendored packages due to source replacement #10729

Closed

ehuss added this to the 1.78.0 milestone Mar 2, 2024

epage mentioned this pull request Jul 26, 2024

cargo add wrongly autocorrects crates when vendored #14309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(add): Improve error when adding registry packages while vendored #13281

fix(add): Improve error when adding registry packages while vendored #13281

LuuuXXX commented Jan 11, 2024 •

edited by epage

Loading

rustbot commented Jan 11, 2024

epage commented Jan 12, 2024

LuuuXXX commented Jan 14, 2024

epage commented Jan 15, 2024

LuuuXXX commented Jan 17, 2024 •

edited

Loading

epage commented Jan 17, 2024

LuuuXXX commented Feb 1, 2024 •

edited

Loading

epage commented Feb 1, 2024

LuuuXXX commented Feb 20, 2024

epage commented Feb 20, 2024

LuuuXXX commented Feb 21, 2024 •

edited

Loading

epage commented Feb 21, 2024

LuuuXXX commented Feb 22, 2024

epage commented Feb 22, 2024

bors commented Feb 22, 2024

bors commented Feb 22, 2024

bors commented Feb 22, 2024

fix(add): Improve error when adding registry packages while vendored #13281

fix(add): Improve error when adding registry packages while vendored #13281

Conversation

LuuuXXX commented Jan 11, 2024 • edited by epage Loading

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Jan 11, 2024

epage commented Jan 12, 2024

LuuuXXX commented Jan 14, 2024

epage commented Jan 15, 2024

LuuuXXX commented Jan 17, 2024 • edited Loading

epage commented Jan 17, 2024

LuuuXXX commented Feb 1, 2024 • edited Loading

epage commented Feb 1, 2024

LuuuXXX commented Feb 20, 2024

epage commented Feb 20, 2024

LuuuXXX commented Feb 21, 2024 • edited Loading

epage commented Feb 21, 2024

LuuuXXX commented Feb 22, 2024

epage commented Feb 22, 2024

bors commented Feb 22, 2024

bors commented Feb 22, 2024

bors commented Feb 22, 2024

LuuuXXX commented Jan 11, 2024 •

edited by epage

Loading

LuuuXXX commented Jan 17, 2024 •

edited

Loading

LuuuXXX commented Feb 1, 2024 •

edited

Loading

LuuuXXX commented Feb 21, 2024 •

edited

Loading