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

Added rust_prost_transform rule for modifying granular proto_library #3083

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Dec 11, 2024

This new rule is to support granular modifications to existing proto_library targets without adding duplicate actions to the build graph depending on what rust_prost_library target was consumed.

closes #2045

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems really handy!

The placement on the proto_library is interesting compared to other languages... It feels like in other languages, each proto_library would have a lang_proto_library target wrapping it, and that that would be an obvious place to put this data. Whereas we don't expect one prost_proto_library per proto_library but instead glom together a bunch of proto_library targets into one prost_proto_library, which means any attributes on that are a bit disconnected from the .proto files they're applying to...

I'd be slightly tempted to say we should add this as a dict[ProtoLabel, TransformLabel] to prost_proto_library rather than adding it to the proto_library itself (using data for this feels... Convenient, but wrong?)

But also, at the end of the day this seems useful and fine, so... Feel free to merge, but would be interested in your thoughts...

@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Dec 13, 2024

I'd be slightly tempted to say we should add this as a dict[ProtoLabel, TransformLabel] to prost_proto_library rather than adding it to the proto_library itself (using data for this feels... Convenient, but wrong?)

We discussed this offline but for posterity, the way the rules work is by using an aspect to perform the protogen and rustc compile on the proro_library targets themselves to try and avoid issues around duplicate symbols. I know of no way to have the rule inject data to the aspect action. The only way I could think of was to add such a mapping to the toolchain but that seemed untenable. Thus I've come up with this idea which is primarily intended for devs to add additional prost info to their proto_library targets (external will be harder to modify).

I think this is something that would be easy to phase out if a better solution comes along but I haven't seen one yet. Pull-requests welcome!

@UebelAndre UebelAndre added this pull request to the merge queue Dec 13, 2024
Merged via the queue into bazelbuild:main with commit f42004c Dec 13, 2024
4 checks passed
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.

Add support for injecting rust code into generated prost libraries
2 participants