Use `includes` instead of `system_includes` for `includes` attr

[keith]

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 https://github.com/bazelbuild/bazel/issues/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

[keith]

this requires a toolchain change in order to add a test, I can do that if folks are happy with this direction

[keith]

see the alternative here https://github.com/bazelbuild/bazel/pull/25751

[trybka]

Selfishly, we have on the order of a dozen+ cc_toolchains that will need to add the "system_include_paths" feature for backwards compatibility until we can spend the time to test the rollout.

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.

[keith]

totally fair, I can add this behind an incompatible flag, before I do that on this one, should we consider https://github.com/bazelbuild/bazel/pull/25751/ or should I close that and move forward with this approach?

[keith]

it looks like there is another implementation option, we can use feature_configuration.is_requested instead of is_enabled so that toolchains don't have to contain the feature, then I imagine it could be added in the REPO.bazel to enable globally, without any toolchain updates. What do you think about that option? The downside is still that you'd have to handle --features=external_include_paths --host_features=external_include_paths separately

[keith]

I've updated to the is_requested approach because the tests have the same issue

[keith]

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 --spawn_strategy=standalone (which the bootstrap build of bazel does).

the change here is that previously those were ignored because --strict_system_includes=false (the default) disables those checks for any headers from -isystem, but they no longer are.

The fix here is to pass --features=external_include_paths --host_features=external_include_paths to continue using -isystem for external headers, and to start external_includes the same as system_includes for --strict_system_includes.

I've added a release notes blurb as well

[fmeum]

@pzembrod Could you take a look? This would be a great improvement to Bazel's usability.

[froody]

+1 I would love to see this in an 8.x release

[pzembrod]

I don't quite understand the implications of this change yet, but a preliminary import and presubmit shows that this causes many breakages. I found two failure modes so far: One is in that #include <memory> in an extern "C++" { ... } block (e.g. in https://github.com/google/boringssl/blob/main/include/openssl/base.h#L419) causes a "'memory' file not found" fatal error. Another is that when compiling a C file (e.g. https://github.com/google/brotli/blob/master/c/dec/bit_reader.h) the #include <string.h> line gets forwarded to the STL's string.h which causes problems, too. So it seems that this change gets C++ and C sources mixed up a bit. I understand the mechanics behind includes and system_includes too little yet to venture a guess how this might be fixed.

[pzembrod]

I don't quite understand the implications of this change yet, but a preliminary import and presubmit shows that this causes many breakages. I found two failure modes so far: One is in that #include <memory> in an extern "C++" { ... } block (e.g. in https://github.com/google/boringssl/blob/main/include/openssl/base.h#L419) causes a "'memory' file not found" fatal error. Another is that when compiling a C file (e.g. https://github.com/google/brotli/blob/master/c/dec/bit_reader.h) the #include <string.h> line gets forwarded to the STL's string.h which causes problems, too. So it seems that this change gets C++ and C sources mixed up a bit. I understand the mechanics behind includes and system_includes too little yet to venture a guess how this might be fixed.

@trybka just pointed out to me that I was missing setting the system_include_paths feature. He's working on this PR.

[keith]

rebased, no direct conflicts so im hoping we're still good here

[keith]

thanks!