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

Set includes for glog headers in Bazel library #880

Closed
wants to merge 1 commit into from

Conversation

sfc-gh-ebrossard
Copy link

Resolves #837

This adds the includes property to the cc_library for glog so that includes are treated as system headers, suppressing issues related to lint, clang-tidy, etc. that may differ from the local project and glog.

@sfc-gh-ebrossard
Copy link
Author

I'm not sure it's feasible for this change to satisfy #837 (comment) or the failing tests, actually, given that includes has to specify a list of directories, and some of the headers specified in hdrs such as logging.h are in the "top-level" glog directory. The error is:

in includes attribute of cc_library rule //:glog: '.' resolves to the workspace root, which would allow this rule and all of its transitive dependents to include any file in your workspace. Please include only what you need.

@drigz
Copy link
Member

drigz commented Dec 8, 2022

I'm afraid I've never fully understood the subtleties and the right way to hold includes vs strip_include_prefix but I feel this is not the right way to do it. Especially since exposing the internal files can break downstream projects if they include the wrong file by accident (this happened with config.h). My guess is that to fix #837 we would need to rearrange the header sources (eg add a top-level include/) and adapt the CMake build correspondingly, but I haven't looked into the details. Sorry :(

@sfc-gh-ebrossard
Copy link
Author

Yes, I think you're right. It would end up being much more invasive than simply setting includes, and with greater risk for breaking downstream projects.

It's probably more straightforward just to resolve the compiler warnings/clang-tidy issues/etc. that necessitate setting includes for some projects. There's also bazel-contrib/rules_foreign_cc#418, though, which is another can of worms...

I'll go ahead and close this PR, given that it doesn't seem like the right approach in any case.

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.

bazel build does not mark includes as system headers
2 participants