Add system_includes to cc_library/cc_binary

Previously even though the attribute was named `includes`, the paths
added there were actually propagated through the `system_includes`
attribute of compilation context. This lead to crosstools passing these
flags with `-isystem` which was unexpected 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`

Fixes bazelbuild#20267

Author: keith

src/main/starlark/builtins_bzl/common/cc/attrs.bzl

@@ -132,6 +132,22 @@ Subject to <a href="${link make-variables}">"Make variable"</a> substitution.
 Each string is prepended with the package path and passed to the C++ toolchain for
 expansion via the "include_paths" CROSSTOOL feature. A toolchain running on a POSIX system
 with typical feature definitions will produce
+<code>-iquote path_to_package/include_entry</code>.
+Unlike <a href="#cc_binary.copts">COPTS</a>, these flags are added for this rule
+and every rule that depends on it. (Note: not the rules it depends upon!) Be
+very careful, since this may have far-reaching effects.  When in doubt, add
+"-I" flags to <a href="#cc_binary.copts">COPTS</a> instead.
+<p>
+The added <code>include</code> paths will include generated files as well as
+files in the source tree.
+</p>
+"""),
+    "system_includes": attr.string_list(doc = """
+List of include dirs to be added to the compile line.
+Subject to <a href="${link make-variables}">"Make variable"</a> substitution.
+Each string is prepended with the package path and passed to the C++ toolchain for
+expansion via the "include_paths" CROSSTOOL feature. A toolchain running on a POSIX system
+with typical feature definitions will produce
 <code>-isystem path_to_package/include_entry</code>.
 This should only be used for third-party libraries that
 do not conform to the Google style of writing #include statements.

src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl

@@ -510,7 +510,8 @@ def cc_binary_impl(ctx, additional_linkopts, force_linkstatic = False):
         cxx_flags = cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions, attr = "cxxopts"),
         defines = cc_helper.defines(ctx, additional_make_variable_substitutions),
         local_defines = cc_helper.local_defines(ctx, additional_make_variable_substitutions) + cc_helper.get_local_defines_for_runfiles_lookup(ctx, ctx.attr.deps),
-        system_includes = cc_helper.system_include_dirs(ctx, additional_make_variable_substitutions),
+        includes = cc_helper.include_dirs(ctx, additional_make_variable_substitutions, attr = "includes"),
+        system_includes = cc_helper.include_dirs(ctx, additional_make_variable_substitutions, attr = "system_includes"),
         private_hdrs = cc_helper.get_private_hdrs(ctx),
         public_hdrs = cc_helper.get_public_hdrs(ctx),
         copts_filter = cc_helper.copts_filter(ctx, additional_make_variable_substitutions),

src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl

@@ -1062,13 +1062,13 @@ def _package_source_root(repository, package, sibling_repository_layout):
         repository = repository[1:]
     return paths.get_relative(paths.get_relative("external", repository), package)
 
-def _system_include_dirs(ctx, additional_make_variable_substitutions):
+def _include_dirs(ctx, additional_make_variable_substitutions, *, attr):
     result = []
     sibling_repository_layout = ctx.configuration.is_sibling_repository_layout()
     package = ctx.label.package
     package_exec_path = _package_exec_path(ctx, package, sibling_repository_layout)
     package_source_root = _package_source_root(ctx.label.workspace_name, package, sibling_repository_layout)
-    for include in ctx.attr.includes:
+    for include in getattr(ctx.attr, attr, []):
         includes_attr = _expand(ctx, include, additional_make_variable_substitutions)
         if includes_attr.startswith("/"):
             continue
@@ -1264,7 +1264,7 @@ cc_helper = struct(
     get_private_hdrs = _get_private_hdrs,
     get_public_hdrs = _get_public_hdrs,
     report_invalid_options = _report_invalid_options,
-    system_include_dirs = _system_include_dirs,
+    include_dirs = _include_dirs,
     get_coverage_environment = _get_coverage_environment,
     create_cc_instrumented_files_info = _create_cc_instrumented_files_info,
     linkopts = _linkopts,

src/main/starlark/builtins_bzl/common/cc/cc_library.bzl

@@ -63,7 +63,8 @@ def _cc_library_impl(ctx):
         cxx_flags = cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions, attr = "cxxopts"),
         defines = cc_helper.defines(ctx, additional_make_variable_substitutions),
         local_defines = cc_helper.local_defines(ctx, additional_make_variable_substitutions) + cc_helper.get_local_defines_for_runfiles_lookup(ctx, ctx.attr.deps + ctx.attr.implementation_deps),
-        system_includes = cc_helper.system_include_dirs(ctx, additional_make_variable_substitutions),
+        includes = cc_helper.include_dirs(ctx, additional_make_variable_substitutions, attr = "includes"),
+        system_includes = cc_helper.include_dirs(ctx, additional_make_variable_substitutions, attr = "system_includes"),
         copts_filter = cc_helper.copts_filter(ctx, additional_make_variable_substitutions),
         purpose = "cc_library-compile",
         srcs = cc_helper.get_srcs(ctx),

src/main/starlark/builtins_bzl/common/objc/compilation_support.bzl

@@ -105,7 +105,7 @@ def _build_common_variables(
         has_module_map = has_module_map,
         attr_linkopts = attr_linkopts,
         direct_cc_compilation_contexts = direct_cc_compilation_contexts,
-        includes = cc_helper.system_include_dirs(ctx, {}) if hasattr(ctx.attr, "includes") else [],
+        includes = cc_helper.include_dirs(ctx, {}, attr = "includes"),
     )
 
     return struct(

src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java

@@ -644,6 +644,60 @@ public void testUseIsystemForIncludes() throws Exception {
         .containsExactlyElementsIn(expected);
   }
 
+  @Test
+  public void testSystemAndNormalIncludes() throws Exception {
+    scratch.file(
+        "BUILD",
+        """
+        cc_library(
+            name = "no_includes",
+            srcs = ["no_includes.cc"],
+        )
+
+        cc_library(
+            name = "lib",
+            srcs = ["lib.cc"],
+            includes = ["normal-includes"],
+            system_includes = ["system-includes"],
+        )
+        cc_library(
+            name = "bin",
+            srcs = ["bin.cc"],
+            includes = ["normal-includes"],
+            system_includes = ["system-includes"],
+        )
+        """);
+    ConfiguredTarget noIncludes = getConfiguredTarget("//:no_includes");
+    ConfiguredTarget lib = getConfiguredTarget("//:lib");
+    ConfiguredTarget bin = getConfiguredTarget("//:bin");
+
+    String systemRoot = "system-includes";
+    List<PathFragment> expectedSystem =
+        new ImmutableList.Builder<PathFragment>()
+            .addAll(
+                noIncludes.get(CcInfo.PROVIDER).getCcCompilationContext().getSystemIncludeDirs())
+            .add(PathFragment.create(systemRoot))
+            .add(targetConfig.getBinFragment(RepositoryName.MAIN).getRelative(systemRoot))
+            .build();
+    assertThat(lib.get(CcInfo.PROVIDER).getCcCompilationContext().getSystemIncludeDirs())
+        .containsExactlyElementsIn(expectedSystem);
+    assertThat(bin.get(CcInfo.PROVIDER).getCcCompilationContext().getSystemIncludeDirs())
+        .containsExactlyElementsIn(expectedSystem);
+
+    String normalRoot = "normal-includes";
+    List<PathFragment> expectedNormalIncludes =
+        new ImmutableList.Builder<PathFragment>()
+            .addAll(
+                noIncludes.get(CcInfo.PROVIDER).getCcCompilationContext().getSystemIncludeDirs())
+            .add(PathFragment.create(normalRoot))
+            .add(targetConfig.getBinFragment(RepositoryName.MAIN).getRelative(normalRoot))
+            .build();
+    assertThat(lib.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
+        .containsExactlyElementsIn(expectedNormalIncludes);
+    assertThat(bin.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
+        .containsExactlyElementsIn(expectedNormalIncludes);
+  }
+
   @Test
   public void testCcTestDisallowsAlwaysLink() throws Exception {
     scratch.file(

src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java

@@ -1381,6 +1381,7 @@ public void testIncludesDirsOfTransitiveCcDepsGetPassedToCompileAction() throws
             name = "cc_lib",
             srcs = ["a.cc"],
             includes = ["foo/bar"],
+            system_includes = ["baz/qux"],
         )
 
         objc_library(
@@ -1397,6 +1398,14 @@ public void testIncludesDirsOfTransitiveCcDepsGetPassedToCompileAction() throws
             Iterables.concat(
                 Iterables.transform(
                     rootedIncludePaths("package/foo/bar"),
+                    element -> ImmutableList.of("-I" + element)))));
+
+    assertContainsSublist(
+        removeConfigFragment(removeConfigFragment(compileAction.getArguments())),
+        ImmutableList.copyOf(
+            Iterables.concat(
+                Iterables.transform(
+                    rootedIncludePaths("package/baz/qux"),
                     element -> ImmutableList.of("-isystem", element)))));
   }