Skip to content

Allow patch to point to the same source #11854

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

Closed
wants to merge 1 commit into from
Closed

Allow patch to point to the same source #11854

wants to merge 1 commit into from

Conversation

fralalonde
Copy link

Cargo currently disallows patching a git dependency to use a different branch of the same source URL. This behaviour is unnecessarily restrictive: a same-URL patch simply has no effect if the branch/tag/commit are the same.

In a project depending on another git-hosted (unpublished) crate, patch should allow to specify another branch of the crate. This scenario occurs naturally when team-developing private projects across multiple crates. A current workaround is to fork the the crate's repository (including alternate branch) just to give it a different URL.

Recognizing that the same-URL situation could also be the result of unintended configuration, this PR preserves the check but relaxes the error into a warning.

@rustbot
Copy link
Collaborator

rustbot commented Mar 15, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon.

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-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2023
@fralalonde
Copy link
Author

#10756

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.

This is not the “right fix”. This allows all patches that previously conflict with each other now are accepted unanimously. Here is one of the failing tests.

SourceId::canonical_url() is used everwhere requiring a comparison between SourceIds. It is a URL but dropping hash (#) and querystring. I feel like it isn't really feasible to provide such a feature. We could try anyway.

@fralalonde
Copy link
Author

Thank you for taking the time to consider my entry.

Yes, I suspected this would be too blunt a change. I also understand this is hard to do "correctly" given the current design of the patch mechanism. Could a solution be a new configuration parameter or environment variable that would switch the behaviour to a "relaxed" mode? Kind of an "unsafe" mode for Cargo patches.

For reference, this is what I'm talking about (should have put this example in the initial commit):

Multiple crates from the same workspace all declaring the same dependency on a private (git-only) crate like this:

[dependencies]
private-lib = { git = "https://gitlab.com/myorg/private-lib", tag = "0.2.0" }

...should be overrideable (IMHO) with a single workspace-level patch entry like:

[patch."https://gitlab.com/myorg/private-lib"]
private-lib = { git = "https://gitlab.com/myorg/private-lib", branch = "0.2.1_alpha1" }

...which at the moment isn't possible because the patch's repo may not have the same URL as the original's crate.

Or maybe I've missed something in the control of dependencies and [patch] isn't what I'm looking for at all? All comments regarding the situation or my limited intellect welcome.

@weihanglo
Copy link
Member

Thanks for the response!

The Cargo team may not have the bandwidth driving the feature to complete. People might need to bring their own design to discuss and work on it from the ground up. I am happy to answer questions regarding Cargo internals anyway, though myself are currently in holidays so expected to be less responsive.

Since this PR is not really a solution ready for review. Let's continue the discussion in the issue #10756 first. I'll close it now. Feel free to repost your findings there in #10756 :)

@weihanglo weihanglo closed this Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-registries Area: registries 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.

3 participants