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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jneem
Copy link

@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
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)

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-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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