Add includes = [ "../../include" ] to libraries with includes level…#10445
Conversation
…s up. Cleanup of all the libraries that recently went further into subdirectories while the headers are still at the original place. Signed-off-by: Henner Zeller <h.zeller@acm.org>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a relative include path (../../include) to multiple Bazel cc_library targets within the src/odb/src directory. Feedback was provided regarding the use of relative paths in the includes attribute, which is considered a maintainability risk in Bazel due to potential include path pollution. The reviewer suggested centralizing header management using strip_include_prefix in a main library target instead of duplicating relative paths across multiple BUILD files.
| includes = [ | ||
| "../../include", | ||
| ], |
There was a problem hiding this comment.
Using includes with relative paths like ../../include is a maintainability risk and is generally discouraged for internal Bazel libraries. This attribute propagates transitively to all dependents, which can lead to "include path pollution" and potential header name collisions in dependent targets.
Recommendation:
Instead of adding includes to every sub-library, consider defining a central cc_library in the package where the headers reside (e.g., src/odb/BUILD) that exports these headers using strip_include_prefix = "include". Sub-libraries can then simply depend on this central target. This approach is more idiomatic, centralizes include path management, and avoids duplicating relative paths across multiple BUILD files.
This observation applies to all the BUILD files modified in this pull request.
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
…s up.
Cleanup of all the libraries that recently went further into subdirectories while the headers are still at the original place.