-
Notifications
You must be signed in to change notification settings - Fork 351
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
fix: export headers with top-level Bazel targets #12762
fix: export headers with top-level Bazel targets #12762
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12762 +/- ##
=======================================
Coverage 93.59% 93.59%
=======================================
Files 2078 2078
Lines 182481 182481
=======================================
Hits 170786 170786
Misses 11695 11695
☔ View full report in Codecov by Sentry. |
@@ -159,13 +189,15 @@ cc_library( | |||
cc_library( | |||
name = "mocks", | |||
testonly = True, | |||
hdrs = ["//google/cloud:mocks"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just went with the most commonly used names. I figured we can change them later if we wanted to.
@@ -67,6 +67,12 @@ config_setting( | |||
}, | |||
) | |||
|
|||
filegroup( | |||
name = "common_hdrs", | |||
srcs = [h for h in google_cloud_cpp_common_hdrs if not h.startswith("internal/")], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observation: we are doing this internal
filtering on handwritten libraries. Our fully generated libraries also have internal
headers which are picked up by globs.
Do we care to filter both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That is one of the reasons why this is "part of" and not "fixes" for that bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included the fixes in this PR. PTAL
@@ -31,11 +31,13 @@ filegroup( | |||
filegroup( | |||
name = "hdrs", | |||
srcs = glob([d + "*.h" for d in src_dirs]), | |||
visibility = ["//:__pkg__"], | |||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g.
filegroup(
name = "public_hdrs",
srcs = glob([d + "*.h" for d in service_dirs]),
visibility = ["//:__pkg__"],
)
(Note the service_dirs
instead of src_dirs
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, that may be handy in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included in this PR now.
Part of the work for #12760
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)