cargo
cargo copied to clipboard
rust-lang
Implement RFC 3289: source replacement ambiguity
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-withkey 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_iofunction, which internally sets an environment variable to change the URL of crates.io.
Changes are insta-stable.
cc #10894 r? @Eh2406
@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
:pushpin: Commit d0d07db4605535b7b0ac1b2bcd50bce4339ff3bf has been approved by ehuss
It is now in the queue for this repository.
:hourglass: Testing commit d0d07db4605535b7b0ac1b2bcd50bce4339ff3bf with merge 0853c1380e20fdee6fcc51253ac41c307c06de04...
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
Team member @Eh2406 has proposed to merge this. The next step is review by the rest of the tagged team members:
- [x] @Eh2406
- [x] @ehuss
- [x] @epage
- [x] @joshtriplett
- [x] @weihanglo
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.
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.
This appears to cause a regression when combined with setting registry.default. See this zulip thread for details.
Looking in to how to fix it before merging this.
I fixed the registry.default issue. The registry.default feature appears to have no tests. They should probably be added - but I'd rather not further block this PR.
Edit I decided to add a test :)
There's a question above that didn't seem to get answered:
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.
From my quick testing, it looks like the verification step uses the replaced source. That seems contrary to the statements made in https://github.com/rust-lang/rfcs/pull/3289#issuecomment-1182732575 on how that should work. Can you expand on that? Am I misunderstanding it? Can we get a test for that on whatever behavior it should have?
From my quick testing, it looks like the verification step uses the replaced source.
Yes, that's correct. The verification step does use the replaced source. I changed my mind and decided it felt more consistent to do the verification using the replaced source. It was also much simpler to implement. The main argument against it was that we'd have to potentially fetch two git indexes, but the sparse registry support is getting close to stabilization, which mitigates that issue.
I've added a test to verify the expected behavior.
Sounds good, thanks!
I suspect it may still be a little difficult to publish with source replacement and without --no-verify. If one tries to publish multiple dependent packages, they will be at the mercy of the replaced source syncing relatively fast. I suspect that usually isn't the case, which means that to publish multiple packages, users will still need to disable source replacement or use --no-verify, or just wait a long time for the mirror to sync. If there is much demand to try to address that, I think we can just change it in the future (or think of alternatives).
@bors r+
:pushpin: Commit 2ecef9497c62a2ec1586bcb99160f6a7e32cd40d has been approved by ehuss
It is now in the queue for this repository.
:hourglass: Testing commit 2ecef9497c62a2ec1586bcb99160f6a7e32cd40d with merge 55362771d7114baac535709c296785cba461526b...
2022-10-07T02:36:45.1055785Z ---- cargo_remove::offline::case stdout ----
2022-10-07T02:36:45.1056373Z thread 'cargo_remove::offline::case' panicked at 'Expected success, was 101
2022-10-07T02:36:45.1056758Z stderr:
2022-10-07T02:36:45.1057009Z ```
2022-10-07T02:36:45.1057492Z warning: please specify `--format-version` flag explicitly to avoid compatibility problems
2022-10-07T02:36:45.1058245Z error: failed to get `clippy` as a dependency of package `cargo-remove-test-fixture v0.1.0 (/home/runner/work/cargo/cargo/target/tmp/cit/t673/case)`
2022-10-07T02:36:45.1058616Z
2022-10-07T02:36:45.1058763Z Caused by:
2022-10-07T02:36:45.1059265Z failed to load source for dependency `clippy`
2022-10-07T02:36:45.1059523Z
2022-10-07T02:36:45.1059671Z Caused by:
2022-10-07T02:36:45.1060040Z Unable to update registry `crates-io`
2022-10-07T02:36:45.1060274Z
2022-10-07T02:36:45.1060424Z Caused by:
2022-10-07T02:36:45.1061135Z could not find a configured source with the name `dummy-registry` when attempting to lookup `crates-io` (configuration in `/home/runner/work/cargo/cargo/target/tmp/cit/t673/home/.cargo/config`)
2022-10-07T02:36:45.1061678Z
2022-10-07T02:36:45.1061820Z ```
2022-10-07T02:36:45.1062212Z ', tests/testsuite/cargo_remove/offline/mod.rs:23:10
2022-10-07T02:36:45.1062621Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
2022-10-07T02:36:45.1062906Z
2022-10-07T02:36:45.1062974Z
2022-10-07T02:36:45.1063344Z failures:
2022-10-07T02:36:45.1063636Z cargo_remove::offline::case
See https://github.com/rust-lang/cargo/pull/11099#pullrequestreview-1133862985
:umbrella: The latest upstream changes (presumably #11177) made this pull request unmergeable. Please resolve the merge conflicts.
:pushpin: Commit 64bcd95c574ac4e99fa4a1d3a7f7bf937608e2eb has been approved by ehuss
It is now in the queue for this repository.
:hourglass: Testing commit 64bcd95c574ac4e99fa4a1d3a7f7bf937608e2eb with merge 51113fb7636000ef3a7942042b0e07f9465cdb98...
💔 Test failed - checks-actions
This new test needs to disambiguate as well.
https://github.com/rust-lang/cargo/blob/882c5dd830aa3c7d1f1d548b56bc04a6fe2856ee/tests/testsuite/login.rs#L95-L120
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.
:pushpin: Commit dd5134c7a59e3a3b8587f1ef04a930185d2ca503 has been approved by ehuss
It is now in the queue for this repository.
:hourglass: Testing commit dd5134c7a59e3a3b8587f1ef04a930185d2ca503 with merge d9b05a149cb7b9882b5170546163cf7dc512fb4e...
:sunny: Test successful - checks-actions Approved by: ehuss Pushing d9b05a149cb7b9882b5170546163cf7dc512fb4e to master...