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

Support path dependencies for Cargo.toml patches #3071

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

P1n3appl3
Copy link
Contributor

@P1n3appl3 P1n3appl3 commented Dec 9, 2024

This makes cargo_bazel respect the [patch] section in the manifest when using crates_vendor. This change was originally proposed in #1902, and this PR is a cleaned up version of that.

The added example and docs section show the resulting BUILD files and directory structure, but here's a high level description:

  • The call to cargo vendor will no longer download crates which are patched out
  • BUILD files are generated by reading the cargo manifest from the patched crate. They're placed in the vendor dir alongside the unpatched crates.
  • These generated BUILD files reference the sources of the patched crate through a filegroup, which is the only "manual" work needed to expose the patched crate to the targets in the vendored dir.

That last one was the main point of discussion when this change was initially proposed (see this comment). There are several options here, such as having cargo_bazel generate BUILD files outside the vendor dir, or having the user manually write BUILD files for each patch. We still believe that the compromise of manually writing BUILD files that expose filegroups and letting crates_vendor still generate BUILD files in the normal vendor dir is ideal for a couple of reasons:

  • If you wanted to manually write BUILD files to patch out vendored crates with your own, that use case is already supported via the override-targets annotation.
  • Even if it'd require the least manual effort, letting cargo_bazel generate files outside of the vendor dir is highly surprising for users, and it'd establish a brittle 2-way dependency between the generated BUILD files inside and outside the vendor dir. If either were ever moved it'd require some nontrivial fixup.
  • The BUILD file that exposes filegroups can be the same for all patched crates. See this change we made on fuchsia where we symlinked that BUILD file to all of our patches.

Fuchsia's adoption of bazel for rust is still blocked on this feature. We're happy to make changes or add tests that the maintainers deem necessary so that we can get this landed and avoid having to maintain these patches on top of upstream. We're successfully using this change now, and besides the integration test added in the example, we'll make sure that this feature continues working as we update rules_rust regularly.

@illicitonion
Copy link
Collaborator

Hi @P1n3appl3! Sorry for not picking up your work here earlier. I recently added quite a similar change in #3025 which is intended to make path dependencies and [patch]es work for crate_universe rules - I haven't actually tried it out with vendor, but would be interested in your experience trying it out - it currently uses a slightly different approach for the BUILD files (it makes a remote repository which copies in the source files, and uses the existing cargo-bazel BUILD file generation to generate a BUILD file in that remote repository. (This is likely also responsible for the merge conflicts you're running into already with the new PR - sorry!)

Let me know what you think and how you'd like to proceed - I'm not particularly attached to what I merged (other than the tests, the tests are useful! Including #3063 which adds tests that live edits to the source code behind the path dependency work).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants