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

Add cc_proto_aspect_hint to allow copts on cc_proto_library #19706

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

keith
Copy link
Contributor

@keith keith commented Dec 18, 2024

This new cc_proto_aspect_hint allows users to specify copts that are
then used when compiling the generated C++ code.

This is a potential solution to bazelbuild/bazel#4446

In that discussion the primary concern was that if you have multiple
cc_proto_library targets that depend on the same proto_library
targets, if the copts was part of cc_proto_library you'd have to be
sure to duplicate that to all cc_proto_library targets in the
dependency tree. Using the relatively new aspect_hints functionality
in bazel we can instead attach that info to the proto_library target
itself, without adding C++ specific attributes to the proto_library
rule.

This new `cc_proto_aspect_hint` allows users to specify `copts` that are
then used when compiling the generated C++ code.

This is a potential solution to bazelbuild/bazel#4446

In that discussion the primary concern was that if you have multiple
`cc_proto_library` targets that depend on the same `proto_library`
targets, if the `copts` was part of `cc_proto_library` you'd have to be
sure to duplicate that to all `cc_proto_library` targets in the
dependency tree. Using the relatively new `aspect_hints` functionality
in bazel we can instead attach that info to the `proto_library` target
itself, without adding C++ specific attributes to the `proto_library`
rule.
cc_proto_aspect_hint = rule(
implementation = _aspect_hint_impl,
attrs = {
"copts": attr.string_list(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can add any other attributes we want here, i imagine linkopts would be the next most sensible one, happy to add other useful ones in this PR, or just as use cases come up

@zhangskz
Copy link
Member

@comius Could you help take a look?

@zhangskz zhangskz added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 18, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 18, 2024
@zhangskz zhangskz added the bazel label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants