Use includes instead of system_includes for includes attr
#4
Conversation
Previously even though the attribute was named `includes` it was passed through the `system_includes` field of the compilation context. This resulted in toolchains passing these include paths with `-isystem`, which is unexpected if you use this for first party code. Many non bazel-first projects have header directory structures that require custom include paths be propagated throughout the graph, the alternative to `includes` is to use `strip_include_prefix`. The downside of `strip_include_prefix` is that you add 1 include path per `cc_library`, even if the libraries are in the same package. With `includes` these are deduplicated. In the case of LLVM using `includes` reduced the number of search paths on the order of hundreds. If users want to use `-isystem` for third party code that uses `includes`, they can pass `--features=external_include_paths --host_features=external_include_paths` If there are first party libraries users want to use `-isystem` with, they can use `features = ["system_include_paths"]` Fixes bazelbuild#20267 RELNOTES[INC]: Use `-I` instead of `-isystem` for `cc_library` / `cc_binary` `includes` attr. To use `-isystem` for only external repositories, you can pass `--features=external_include_paths --host_features=external_include_paths`. To use `-isystem` for a single `cc_library` / `cc_binary` `includes`, you can set `features = ["system_include_paths"],` on the target
|
this requires a toolchain change in order to add a test, I can do that if folks are happy with this direction |
|
see the alternative here bazelbuild#25751 |
|
Selfishly, we have on the order of a dozen+ cc_toolchains that will need to add the I'm not opposed in principle, but can we add an incompatible flag for this? Or keep the current behavior if a feature is unset? Open to suggestions, but I'd like to have a way to disable this globally (for our monorepo) without having to modify many toolchains. |
|
totally fair, I can add this behind an incompatible flag, before I do that on this one, should we consider bazelbuild#25751 or should I close that and move forward with this approach? |
|
it looks like there is another implementation option, we can use |
|
I've updated to the |
|
ok the CI failures here lead to some interesting discoveries. for one the BUILD files of both grpc and c-ares have undeclared inclusion errors lurking that only hit if you use the change here is that previously those were ignored because The fix here is to pass I've added a release notes blurb as well |
Previously even though the attribute was named
includesit was passedthrough the
system_includesfield of the compilation context. Thisresulted in toolchains passing these include paths with
-isystem,which is unexpected if you use this for first party code.
Many non bazel-first projects have header directory structures that
require custom include paths be propagated throughout the graph, the
alternative to
includesis to usestrip_include_prefix. The downsideof
strip_include_prefixis that you add 1 include path percc_library, even if the libraries are in the same package. Withincludesthese are deduplicated. In the case of LLVM usingincludesreduced the number of search paths on the order of hundreds.
If users want to use
-isystemfor third party code that usesincludes, they can pass--features=external_include_paths --host_features=external_include_pathsIf there are first party libraries users want to use
-isystemwith,they can use
features = ["system_include_paths"]Fixes bazelbuild#20267
RELNOTES[INC]: Use
-Iinstead of-isystemforcc_library/cc_binaryincludesattr. To use-isystemfor only external repositories, you can pass--features=external_include_paths --host_features=external_include_paths. To use-isystemfor a singlecc_library/cc_binaryincludes, you can setfeatures = ["system_include_paths"],on the target