Implement RFC 3289: source replacement ambiguity #10907

arlosi · 2022-07-27T19:39:26Z

Implements RFC 3289

When the crates-io source is replaced, the user needs to specify --registry <NAME> when running an API operation to disambiguate which registry to use. Otherwise, cargo will issue a new error.
In source replacement, the replace-with key can reference the name of an alt registry in the [registries] table.
Publishing to source-replaced crates.io is no longer permitted using the crates.io token (registry.token). We have had a deprecation warning in place since Several updates to token/index handling. #7973 (1.45.0).

Testing

Tests that interacting with crates.io use the new replace_crates_io function, which internally sets an environment variable to change the URL of crates.io.

Changes are insta-stable.

cc #10894
r? @Eh2406

Eh2406

The code changes themselves are surprisingly small and clear. They look good. Thank you.

However I would like to see tests that explicitly use all the new functionality. Off the top of my head replace-with pointing to a registry. Making sure all the suggested arguments actually work. Making sure that the correct API token goes to the correct server in places where we were doing it wrong before. (And confirming it in the places we were doing it right before.) To be fair maybe these are being tested by existing tests, and I didn't notice in all of the mechanical changes that were required. If so I'm sorry.

On a meta-note: as are reviewing bandwidth continues to be very limited, I want to push to have more of the "obviously correct things" tested. There are very few people who can keep all of the implications of all of cargoes functionality in their head, and at the moment there's only one of them on the team. Inevitably there are gonna be some PR's that are merged without fully realizing all of the implications, the closer we can get to "all the existing tests still pass, so it's safe to ship" the better.

tests/testsuite/publish.rs

crates/cargo-test-support/src/lib.rs

src/cargo/ops/registry.rs

Eh2406 · 2022-08-25T16:04:27Z

@arlosi do we have good testing for "Making sure that the correct API token goes to the correct server in places where we were doing it wrong before. (And confirming it in the places we were doing it right before.)"

aside from that, this looks good to me except for the questions about how to test --registry crates-io which I want to leave up to Eric. So

@bors r=@ehuss

Implement RFC 3289: source replacement ambiguity ### Implements [RFC 3289](rust-lang/rfcs#3289) * When the crates-io source is replaced, the user needs to specify `--registry <NAME>` when running an API operation to disambiguate which registry to use. Otherwise, cargo will issue a new error. * In source replacement, the `replace-with` key can reference the name of an alt registry in the `[registries]` table. * Publishing to source-replaced crates.io is no longer permitted using the crates.io token (`registry.token`). We have had a deprecation warning in place since #7973 (1.45.0). ### Testing * Tests are updated to add the `--registry dummy-registry` parameter to specify the test registry (otherwise they would get the new error message) * A few tests that need to verify crates-io-specific configuration use an internal `allow_silent_crates_io_replacement` function to allow the previous behavior of silently replacing crates.io within the testing framework. Changes are insta-stable. cc #10894 r? `@Eh2406`

Eh2406 · 2022-08-25T16:05:15Z

@bors r-

Eh2406 · 2022-09-01T21:35:08Z

The changes look good, and well tested. I would just approve, but this is insta-stable. So I'm going to ask for some checkmarks.

By the way, rereading the RFC I'm not seeing code for "Cargo.toml manifest key publish = <NAME> is not set (only applies for publishing)." But I'm happy to leave that for follow-up.

@rfcbot merge

rfcbot · 2022-09-01T21:35:09Z

Team member @Eh2406 has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

ehuss

Looks great, thank you very much!

Can you update the config documentation for replace-with to also mention it can use a name from the [registries] table? here

Just to double-check: What is the behavior of the verification step when crates.io is replaced? Does it resolve using the replaced source, or does it use crates.io? I can't find a test for that scenario, and I vaguely recall discussing this situation, but I don't remember the details.

src/doc/src/reference/source-replacement.md

src/cargo/ops/registry.rs

src/cargo/core/source/source_id.rs

weihanglo · 2022-10-07T23:51:16Z

💔 Test failed - checks-actions

This new test needs to disambiguate as well.

cargo/tests/testsuite/login.rs

Lines 95 to 120 in 882c5dd

    
           fn empty_login_token() { 
        
               let _registry = RegistryBuilder::new().build(); 
        
               setup_new_credentials(); 
        
               cargo_process("login") 
        
                   .with_stdout("please paste the API Token found on [..]/me below") 
        
                   .with_stdin("\t\n") 
        
                   .with_stderr( 
        
                       "\ 
        
           [UPDATING] `dummy-registry` index 
        
           [ERROR] please provide a non-empty token 
        
           ", 
        
                   ) 
        
                   .with_status(101) 
        
                   .run(); 
        
               cargo_process("login") 
        
                   .arg("") 
        
                   .with_stderr( 
        
                       "\ 
        
           [ERROR] please provide a non-empty token 
        
           ", 
        
                   ) 
        
                   .with_status(101) 
        
                   .run(); 
        
           }

Muscraft · 2022-10-08T00:27:10Z

Having the fix for the issue from #11099 in this PR instead of in a separate commit or PR isn't ideal. Since there #11193 was opened to fix the issue from #11099 it might be better to have it merged before this PR.

Use correct version of cargo in test Fix `cargo_remove::offline` test using wrong version of cargo in test, the local version and calling instance of cargo instead of the one being tested. Issue discovered in #10907 after merge of #11099.

Eh2406 · 2022-10-08T03:50:34Z

@bors r=ehuss

bors · 2022-10-08T03:50:35Z

📌 Commit dd5134c has been approved by ehuss

It is now in the queue for this repository.

bors · 2022-10-08T03:50:43Z

⌛ Testing commit dd5134c with merge d9b05a1...

bors · 2022-10-08T04:51:04Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing d9b05a1 to master...

Ran into confusion on this when updating rust-lang#11062 to be on top of rust-lang#10907. This now threads both types of sources through which comments explaining each so callers will always have to explicitly acknowledge which they are dealing with.

9 commits in 3cdf1ab25dc4fe56f890e8c7330d53a23ad905d3..b8f30cb23c4e5f20854a4f683325782b7cff9837 2022-10-07 17:34:03 +0000 to 2022-10-10 19:16:06 +0000 - Add more doc comments for three modules (rust-lang/cargo#11207) - docs: fix (rust-lang/cargo#11208) - Add completions for `cargo remove` (rust-lang/cargo#11204) - Config file loaded via CLI takes priority over env vars (rust-lang/cargo#11077) - Use `#[default]` when possible (rust-lang/cargo#11197) - Implement RFC 3289: source replacement ambiguity (rust-lang/cargo#10907) - Use correct version of cargo in test (rust-lang/cargo#11193) - Check empty input for login (rust-lang/cargo#11145) - Add retry support to sparse registries (rust-lang/cargo#11069)

Update cargo 9 commits in 3cdf1ab25dc4fe56f890e8c7330d53a23ad905d3..b8f30cb23c4e5f20854a4f683325782b7cff9837 2022-10-07 17:34:03 +0000 to 2022-10-10 19:16:06 +0000 - Add more doc comments for three modules (rust-lang/cargo#11207) - docs: fix (rust-lang/cargo#11208) - Add completions for `cargo remove` (rust-lang/cargo#11204) - Config file loaded via CLI takes priority over env vars (rust-lang/cargo#11077) - Use `#[default]` when possible (rust-lang/cargo#11197) - Implement RFC 3289: source replacement ambiguity (rust-lang/cargo#10907) - Use correct version of cargo in test (rust-lang/cargo#11193) - Check empty input for login (rust-lang/cargo#11145) - Add retry support to sparse registries (rust-lang/cargo#11069)

Ran into confusion on this when updating rust-lang#11062 to be on top of rust-lang#10907. This now threads both types of sources through which comments explaining each so callers will always have to explicitly acknowledge which they are dealing with.

refactor(publish): Clarify which SourceId is being used Ran into confusion on this when updating #11062 to be on top of #10907. This now threads both types of sources through which comments explaining each so callers will always have to explicitly acknowledge which they are dealing with.

Fix old_cargos tests Some of these tests have bitrotted a bit since they aren't enabled by default. The issues are: * Change to `config` to `config.toml` shouldn't have been made to this test since old cargos can't read config.toml. * Cargo's own `Carog.toml` now using dotted keys breaks parsing on some old versions, ignore them. * `cargo pkgid` is now emitting a different format. * #13085 changed how packages are published in the testsuite to correctly include the `[features]` table in the generated `Cargo.toml`. This exposed an oversight in the `old_cargos::new_features` test wasn't actually testing things correctly. It now correctly illustrates the errors received in older versions. I have a vague memory of this, but I don't remember why it was done this way. * #10907 changed the default config to use `[registries]` instead of `[source]` to implement source replacement of crates.io. Older versions can't handle that. Some of these tests probably should just be deleted, since I don't think they are really bringing much value. Or at least they should have some floor that they won't test under. However, I'm not quite ready to do that.

rust-highfive assigned Eh2406 Jul 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2022

arlosi force-pushed the sr2 branch 2 times, most recently from 1134053 to 028aa47 Compare July 27, 2022 19:54

This was referenced Jul 27, 2022

Implement RFC 3139: alternative registry authentication support #10592

Merged

Tracking Issue for source replacement ambiguity #10894

Closed

Eh2406 reviewed Aug 18, 2022

View reviewed changes

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

crates/cargo-test-support/src/lib.rs Outdated Show resolved Hide resolved

src/cargo/ops/registry.rs Outdated Show resolved Hide resolved

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

arlosi force-pushed the sr2 branch from 028aa47 to d0d07db Compare August 23, 2022 21:31

This comment was marked as outdated.

Sign in to view

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 Aug 25, 2022

This comment was marked as outdated.

Sign in to view

bors added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 25, 2022

This comment was marked as outdated.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Aug 25, 2022

arlosi force-pushed the sr2 branch 3 times, most recently from 52d330c to a620d05 Compare August 31, 2022 16:44

Eh2406 added the T-cargo Team: Cargo label Sep 1, 2022

rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Sep 1, 2022

ehuss reviewed Sep 2, 2022

View reviewed changes

src/doc/src/reference/source-replacement.md Outdated Show resolved Hide resolved

src/cargo/ops/registry.rs Outdated Show resolved Hide resolved

src/cargo/core/source/source_id.rs Outdated Show resolved Hide resolved

arlosi force-pushed the sr2 branch from a620d05 to 720c2fa Compare September 2, 2022 16:20

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 7, 2022

cassaundra mentioned this pull request Oct 8, 2022

Use correct version of cargo in test #11193

Merged

Implement RFC 3289: source replacement ambiguity

dd5134c

arlosi force-pushed the sr2 branch from 64bcd95 to dd5134c Compare October 8, 2022 03:32

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 Oct 8, 2022

bors merged commit d9b05a1 into rust-lang:master Oct 8, 2022

bors mentioned this pull request Oct 8, 2022

Support for overriding dl and api in config.json of registry index #11179

Closed

epage mentioned this pull request Oct 11, 2022

refactor(publish): Clarify which SourceId is being used #11216

Merged

weihanglo mentioned this pull request Oct 11, 2022

Update cargo rust-lang/rust#102934

Merged

Muscraft mentioned this pull request Oct 24, 2022

Output which errors are automatically correctable, to improve discoverability of cargo fix and cargo clippy --fix #10976

Closed

This was referenced Dec 7, 2022

Publishing with a crates.io mirror has a confusing error message #6722

Closed

Unable to publish to crates.io with source replacement #10640

Closed

ehuss added this to the 1.66.0 milestone Nov 6, 2023

ehuss mentioned this pull request Feb 12, 2024

Fix old_cargos tests #13435

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RFC 3289: source replacement ambiguity #10907

Implement RFC 3289: source replacement ambiguity #10907

arlosi commented Jul 27, 2022 •

edited

Loading

Eh2406 left a comment

Eh2406 commented Aug 25, 2022

This comment was marked as outdated.

This comment was marked as outdated.

Eh2406 commented Aug 25, 2022

This comment was marked as outdated.

Eh2406 commented Sep 1, 2022

rfcbot commented Sep 1, 2022 •

edited by weihanglo

Loading

ehuss left a comment

weihanglo commented Oct 7, 2022

Muscraft commented Oct 8, 2022

Eh2406 commented Oct 8, 2022

bors commented Oct 8, 2022

bors commented Oct 8, 2022

bors commented Oct 8, 2022

Implement RFC 3289: source replacement ambiguity #10907

Implement RFC 3289: source replacement ambiguity #10907

Conversation

arlosi commented Jul 27, 2022 • edited Loading

Implements RFC 3289

Testing

Eh2406 left a comment

Choose a reason for hiding this comment

Eh2406 commented Aug 25, 2022

This comment was marked as outdated.

This comment was marked as outdated.

Eh2406 commented Aug 25, 2022

This comment was marked as outdated.

Eh2406 commented Sep 1, 2022

rfcbot commented Sep 1, 2022 • edited by weihanglo Loading

ehuss left a comment

Choose a reason for hiding this comment

weihanglo commented Oct 7, 2022

Muscraft commented Oct 8, 2022

Eh2406 commented Oct 8, 2022

bors commented Oct 8, 2022

bors commented Oct 8, 2022

bors commented Oct 8, 2022

arlosi commented Jul 27, 2022 •

edited

Loading

rfcbot commented Sep 1, 2022 •

edited by weihanglo

Loading