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

Draft: allow patching Git source dependencies with patch files #9001

Closed
wants to merge 3 commits into from

Conversation

da-x
Copy link
Member

@da-x da-x commented Dec 19, 2020

First attempt at addressing #4648. This is a functioning demo (though sure there are some bugs).

This is currently lacking testing/documentation to get early feedback. It's also narrowed to Git sources only (i.e. git = <url>), patch-files spec on other sources are currently ignored.

I'm posting this to understand whether I've came close to a good approach before investing more time to extend it, and to get some answers to questions that came up during the implementation -

  1. Should SourceId know about the patches? Does the lockfile need to know?
  2. Patches are essentially per source and not per package, so would we have to respecify all of them for packages that share a source, or should we move this out of TomlDependency?
  3. Should this really be only under target/, and what should we name this directory?
  4. How to handle other kind of Sources? Should we limit this to the standard Repository and Git sources dependencies?
  5. Is it okay to rely over git command in order to apply the patches, or should we rely over patch util, or on some Rust crate that knows how to apply diffs independently?

da-x added 3 commits December 19, 2020 17:21
This is needed so that we have access to the target directory. In the
special case where we have patch files, the checked out Git repo will
be there instead of under $HOME/.cargo.
Patches need to be of `git format-patch` export. Filenames are relative
to workspace manifest. For example:

    [patch.crates-io]
    time = { git = "https://github.com/time-rs/time", tag = "0.1.41", patch-files = ["patches/0001-Hack-around-a-getting-precise-time-in-Windows.patch"]}
@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 19, 2020
@Eh2406
Copy link
Contributor

Eh2406 commented Dec 21, 2020

Thank you for the work to make Cargo better! We unfortunately have a lot of things floating around in our issues, not all of witch are good ideas. We have not had time to document which we think are/not worth implementing. As mentioned (but not clearly) in the Contributor Guide here and here, we want to discuss ideas before work is put in on implementation and intend to tag Feature accepted when the conversation has concluded that it is good to go.

I don't know if this functionality is something the Team wants in Cargo. I look forward to discussing the pros and cons. This message is just ment as a heads up that this PR may not get merge, with no intended criticism of you or your work.

@alexcrichton
Copy link
Member

I would agree with @Eh2406 that this seems like a weighty enough feature that I don't think starting with merging or discussion on a PR is best. I think instead there should be a discussion about how this would work in Cargo and such, ideally through an RFC but not necessarily required.

@da-x
Copy link
Member Author

da-x commented Jan 12, 2021

Ok, so maybe this wasn't discussed enough in the original issue #4648 - we can do it there. I'll write an RFC based on the discussion.

@bors
Copy link
Contributor

bors commented Feb 10, 2021

☔ The latest upstream changes (presumably #9133) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss
Copy link
Contributor

ehuss commented Aug 6, 2021

Closing per the comments above about designing and implementing new features. Thanks!

@ehuss ehuss closed this Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants