Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: resolve crates.io source replacement ambiguity #3289

Merged
merged 5 commits into from
Jul 23, 2022
Merged

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Jul 5, 2022

When Cargo is performing an API operation (yank/login/publish/etc.) to a source-replaced crates-io, require the user to pass --registry <NAME> to specify exactly which registry to use. Additionally, ensure that the token for crates-io is never sent to a replacement registry.

Rendered

@arlosi arlosi changed the title RFC: source_replacement_ambiguity RFC: resolve crates.io source replacement ambiguity Jul 5, 2022
@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Jul 6, 2022
@jmyersmsft
Copy link

jmyersmsft commented Jul 7, 2022

I'd expand the language in Change 3 to clarify that credentials for one registry must not be sent to any other registry, regardless of whether either is crates.io. Any exception to that should have to be an explicit configuration by the user (and probably not in a place that would come from source control), or something controlled by the registry for which you have credentials (e.g. maybe crates.io could somehow say official-second-site-of-crates-io.net can receive crates.io credentials)


Should the `--registry <NAME>` command line argument be allowed to reference the name of a `source` from the `[source]` table as well? This makes it more flexible, but adds potentially unnecessary complexity.

Cargo's tests rely on the ability to replace the crates.io source and have the crates.io credentials go to the replaced source. We need a way for these tests to continue working.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too worried about this, as I believe all of the relevant tests could be adjusted to include --registry if necessary. In the past, the tests would pass --index, though that was removed in rust-lang/cargo#7973.

It is unfortunate that it is difficult to test cargo's behavior without using source replacement. There are some tests that are impossible to write, but I don't think it should be too much of a problem.

@ehuss
Copy link
Contributor

ehuss commented Jul 12, 2022

Thanks @arlosi for putting this together! This seems reasonable to me, so I'm going to go ahead and start FCP. We've been discussing this off and on for quite a while now, and this I believe matches what we all agree on.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 12, 2022

Team member @ehuss 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.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Jul 12, 2022
@ehuss
Copy link
Contributor

ehuss commented Jul 13, 2022

Oh, I did just think of one thing. If you are using a mirror of crates-io, and you run cargo publish --registry crates-io, I believe this RFC is indicating that the verification step will use the index directly from crates-io ("Cargo ignores source replacement when building and publishing the crate to crates.io."). However, I believe there are scenarios where that won't work (or won't work well). For example, I believe many users in China are using the mirror from USTC, since otherwise there are network problems.

I'm wondering if it would make sense to leave the verification step to use source replacement? I can't think of a strong reason to change that. The mirror might lag the original source, but in that case the user can either wait if they are publishing multiple inter-dependent packages, or they can use --no-verify.

Can you say more why that particular statement is there?

@arlosi
Copy link
Contributor Author

arlosi commented Jul 13, 2022

Yes, when using --registry crates-io all steps would be performed with crates.io directly.

If the verification step followed source replacement, then Cargo would need to fetch the index for the replacement and crates.io (the api URL is stored in config.json in the index, so we need to fetch that from the real crates.io). It seemed better to avoid fetching two indexes, and easier to explain.

It's currently not possible to publish to crates.io with source replacement configured, so this is not a loss of functionality. Users in China currently have to disable the USTC replacement in the config in order to publish.

I suppose if we were using -Z sparse-registry, then we could fetch only config.json from crates.io and the rest of the index from the mirror? Or maybe it's not a big deal to fetch two indexes?

Edit: In the implementation of this RFC, the verification build does use the source replacement.

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Jul 13, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Jul 13, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for writing this! Just some nitpicks.

text/0000-source_replacement_ambiguity.md Outdated Show resolved Hide resolved
text/0000-source_replacement_ambiguity.md Outdated Show resolved Hide resolved
text/0000-source_replacement_ambiguity.md Outdated Show resolved Hide resolved
text/0000-source_replacement_ambiguity.md Outdated Show resolved Hide resolved
text/0000-source_replacement_ambiguity.md Outdated Show resolved Hide resolved
arlosi and others added 2 commits July 22, 2022 08:49
Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Jul 23, 2022
@rfcbot
Copy link
Collaborator

rfcbot commented Jul 23, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@ehuss
Copy link
Contributor

ehuss commented Jul 23, 2022

Huzzah! The @rust-lang/cargo team has decided to accept this RFC.

To track further discussion, subscribe to the tracking issue here:
rust-lang/cargo#10894

bors added a commit to rust-lang/cargo that referenced this pull request Aug 25, 2022
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`
bors added a commit to rust-lang/cargo that referenced this pull request Oct 7, 2022
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 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`
bors added a commit to rust-lang/cargo that referenced this pull request Oct 7, 2022
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 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`
bors added a commit to rust-lang/cargo that referenced this pull request Oct 8, 2022
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 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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants