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

Silence the warning about forgetting the vendoring #13886

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

jneem
Copy link
Contributor

@jneem jneem commented May 8, 2024

When sparse crates.io is used, dependencies from crates.io always come from ReplacedSources, as the non-sparse crates.io gets replaced by the sparse one. As far as diagnostics go, this case shouldn't "count" as a replaced source.

I had trouble adding a test for this change, because the test harness replaces everything with a dummy registry.

Fixes #12802

@rustbot
Copy link
Collaborator

rustbot commented May 8, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-overrides Area: general issues with overriding dependencies (patch, replace, paths) A-source-replacement Area: [source] replacement S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2024
@ehuss
Copy link
Contributor

ehuss commented May 9, 2024

Thanks!

I believe you can use the replace_crates_io() function to replace crates.io without using the typical source replacement. See the test builtin_source_replacement as an example. Can you give that a try and see if it works?

Also, can you update the docstring on Registry::is_replaced to mention that it is false for built-in replacements?

Perhaps @arlosi could give this a second set of eyes to determine if this is the best strategy to fix the issue. I think it should be fine. This method is only used for this one error message. I just have a slight concern that it could cause confusion in the future (returning false for something that is replaced internally).

@jneem
Copy link
Contributor Author

jneem commented May 9, 2024

Thanks for the hint! I managed to get a test working (which fails before this PR, and passes after).

I just have a slight concern that it could cause confusion in the future (returning false for something that is replaced internally).

I was a little worried about this too. I think the usages of ReplacedSource::is_builtin_replacement suggests that it's often necessary to distinguish between builtin and non-builtin replacements. So if it ever becomes necessary to do so on the Source trait, we could replace is_replaced by a method returning a three-value enum (NotReplaced, BuiltinReplaced, OtherReplaced)

@arlosi
Copy link
Contributor

arlosi commented Jun 4, 2024

I think this is OK:

  • The fact that Cargo is using source replacement to handle sparse registry access to crates.io is an implementation detail.
  • Consumers of the Source trait should not see it as a source replacement since the user did not configure source replacement here.

Thanks for this fix!

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 4, 2024

📌 Commit 2f97a3b has been approved by arlosi

It is now in the queue for this repository.

@bors 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 4, 2024
@bors
Copy link
Collaborator

bors commented Jun 4, 2024

⌛ Testing commit 2f97a3b with merge 34a6a87...

@bors
Copy link
Collaborator

bors commented Jun 4, 2024

☀️ Test successful - checks-actions
Approved by: arlosi
Pushing 34a6a87 to master...

@bors bors merged commit 34a6a87 into rust-lang:master Jun 4, 2024
21 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
Update cargo

9 commits in 7a6fad0984d28c8330974636972aa296b67c4513..34a6a87d8a2330d8c9d578f927489689328a652d
2024-05-31 22:26:03 +0000 to 2024-06-04 15:31:01 +0000
- Silence the warning about forgetting the vendoring (rust-lang/cargo#13886)
- fix(vendor): Ensure sort happens for vendor (rust-lang/cargo#14004)
- fix(add): Avoid escaping double-quotes by using string literals (rust-lang/cargo#14006)
- refactor(source): Split `RecursivePathSource` out of `PathSource` (rust-lang/cargo#13993)
- doc: Add README for resolver-tests (rust-lang/cargo#13977)
- Allows the default git/gitoxide configuration to be obtained from the ENV and config (rust-lang/cargo#13687)
- refactor: Transition direct assertions from cargo-test-support to snapbox (rust-lang/cargo#13980)
- Fix: Skip deserialization of unrelated fields with overlapping name (rust-lang/cargo#14000)
- chore(deps): update alpine docker tag to v3.20 (rust-lang/cargo#13996)

r? ghost
@rustbot rustbot added this to the 1.80.0 milestone Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-overrides Area: general issues with overriding dependencies (patch, replace, paths) A-source-replacement Area: [source] replacement S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

re-vendored suggestion is incorrect with sparse crates.io
5 participants