Use includes instead of system_includes for includes attr

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

Author: keith

compile.sh

@@ -61,7 +61,7 @@ if [[ $PLATFORM == "darwin" ]] && \
 fi
 
 if [[ $PLATFORM == "windows" ]]; then
-  EXTRA_BAZEL_ARGS="${EXTRA_BAZEL_ARGS-} --cxxopt=/std:c++17 --host_cxxopt=/std:c++17"
+  EXTRA_BAZEL_ARGS="${EXTRA_BAZEL_ARGS-} --cxxopt=/std:c++17 --host_cxxopt=/std:c++17 --copt=/external:W0 --host_copt=/external:W0"
 fi
 
 source scripts/bootstrap/bootstrap.sh

scripts/bootstrap/bootstrap.sh

@@ -44,6 +44,7 @@ _BAZEL_ARGS="--spawn_strategy=standalone \
       --enable_bzlmod \
       --check_direct_dependencies=error \
       --lockfile_mode=update \
+      --features=external_include_paths --host_features=external_include_paths \
       --override_repository=$(cat derived/maven/MAVEN_CANONICAL_REPO_NAME)=derived/maven \
       --java_runtime_version=${JAVA_VERSION} \
       --java_language_version=${JAVA_VERSION} \

src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java

@@ -1133,7 +1133,10 @@ public void validateInclusions(
 
   private Iterable<PathFragment> getValidationIgnoredDirs() {
     List<PathFragment> cxxSystemIncludeDirs = getBuiltInIncludeDirectories();
-    return Iterables.concat(cxxSystemIncludeDirs, ccCompilationContext.getSystemIncludeDirs());
+    return Iterables.concat(
+        cxxSystemIncludeDirs,
+        ccCompilationContext.getSystemIncludeDirs(),
+        ccCompilationContext.getExternalIncludeDirs());
   }
 
   @VisibleForTesting

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

@@ -510,7 +510,7 @@ 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),
         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,7 +1062,7 @@ 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):
     result = []
     sibling_repository_layout = ctx.configuration.is_sibling_repository_layout()
     package = ctx.label.package
@@ -1264,7 +1264,8 @@ 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,
+    system_include_dirs = _include_dirs,  # TODO: Remove uses of old name
     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,7 @@ 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),
         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/cc/compile/cc_compilation_helper.bzl

@@ -227,7 +227,10 @@ def _init_cc_compilation_context(
     external_include_dirs = []
     declared_include_srcs = []
 
-    if not external:
+    if not external and feature_configuration.is_requested("system_include_paths"):
+        system_include_dirs_for_context = system_include_dirs + include_dirs
+        include_dirs_for_context = []
+    elif not external:
         system_include_dirs_for_context = list(system_include_dirs)
         include_dirs_for_context = list(include_dirs)
     else:

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, {}) if hasattr(ctx.attr, "includes") else [],
     )
 
     return struct(

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

@@ -573,7 +573,7 @@ public void testIsolatedIncludes() throws Exception {
     ConfiguredTarget foo = getConfiguredTarget("//bang:bang");
 
     String includesRoot = "bang/bang_includes";
-    assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getSystemIncludeDirs())
+    assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
         .containsAtLeast(
             PathFragment.create(includesRoot),
             targetConfig.getGenfilesFragment(RepositoryName.MAIN).getRelative(includesRoot));
@@ -596,12 +596,12 @@ public void testDisabledGenfilesDontShowUpInSystemIncludePaths() throws Exceptio
     ConfiguredTarget foo = getConfiguredTarget("//bang:bang");
     PathFragment genfilesDir =
         targetConfig.getGenfilesFragment(RepositoryName.MAIN).getRelative(includesRoot);
-    assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getSystemIncludeDirs())
+    assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
         .contains(genfilesDir);
 
     useConfiguration("--incompatible_merge_genfiles_directory");
     foo = getConfiguredTarget("//bang:bang");
-    assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getSystemIncludeDirs())
+    assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
         .doesNotContain(genfilesDir);
   }
 
@@ -626,6 +626,7 @@ public void testUseIsystemForIncludes() throws Exception {
             name = "bang",
             srcs = ["bang.cc"],
             includes = ["bang_includes"],
+            features = ["system_include_paths"],
         )
         """);
 
@@ -644,6 +645,45 @@ public void testUseIsystemForIncludes() throws Exception {
         .containsExactlyElementsIn(expected);
   }
 
+  @Test
+  public void testUseIForIncludes() throws Exception {
+    // Tests the effect of --use_isystem_for_includes.
+    useConfiguration("--incompatible_merge_genfiles_directory=false");
+    scratch.file(
+        "no_includes/BUILD",
+        """
+        cc_library(
+            name = "no_includes",
+            srcs = ["no_includes.cc"],
+        )
+        """);
+    ConfiguredTarget noIncludes = getConfiguredTarget("//no_includes:no_includes");
+
+    scratch.file(
+        "bang/BUILD",
+        """
+        cc_library(
+            name = "bang",
+            srcs = ["bang.cc"],
+            includes = ["bang_includes"],
+        )
+        """);
+
+    ConfiguredTarget foo = getConfiguredTarget("//bang:bang");
+
+    String includesRoot = "bang/bang_includes";
+    List<PathFragment> expected =
+        new ImmutableList.Builder<PathFragment>()
+            .addAll(
+                noIncludes.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
+            .add(PathFragment.create(includesRoot))
+            .add(targetConfig.getGenfilesFragment(RepositoryName.MAIN).getRelative(includesRoot))
+            .add(targetConfig.getBinFragment(RepositoryName.MAIN).getRelative(includesRoot))
+            .build();
+    assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
+        .containsExactlyElementsIn(expected);
+  }
+
   @Test
   public void testCcTestDisallowsAlwaysLink() throws Exception {
     scratch.file(

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

@@ -1196,18 +1196,12 @@ public void testIncludePathOrder() throws Exception {
     assertContainsSublist(
         action.getCompilerOptions(),
         ImmutableList.of(
-            "-isystem",
-            "foo/foo",
-            "-isystem",
-            genfilesDir + "/foo/foo",
-            "-isystem",
-            binDir + "/foo/foo",
-            "-isystem",
-            "foo/bar",
-            "-isystem",
-            genfilesDir + "/foo/bar",
-            "-isystem",
-            binDir + "/foo/bar"));
+            "-Ifoo/foo",
+            "-I" + genfilesDir + "/foo/foo",
+            "-I" + binDir + "/foo/foo",
+            "-Ifoo/bar",
+            "-I" + genfilesDir + "/foo/bar",
+            "-I" + binDir + "/foo/bar"));
   }
 
   @Test
@@ -1911,22 +1905,24 @@ public void testImplementationDepsCompilationContextIsNotPropagated() throws Exc
     assertThat(artifactsToStrings(libCompilationContext.getDeclaredIncludeSrcs()))
         .doesNotContain("src foo/implementation_dep.h");
 
-    assertThat(pathfragmentsToStrings(libCompilationContext.getSystemIncludeDirs()))
+    assertThat(pathfragmentsToStrings(libCompilationContext.getIncludeDirs()))
         .contains("foo/public_dep");
-    assertThat(pathfragmentsToStrings(libCompilationContext.getSystemIncludeDirs()))
+    assertThat(pathfragmentsToStrings(libCompilationContext.getIncludeDirs()))
         .contains("foo/interface_dep");
+    assertThat(pathfragmentsToStrings(libCompilationContext.getIncludeDirs()))
+        .doesNotContain("foo/implementation_dep");
     assertThat(pathfragmentsToStrings(libCompilationContext.getSystemIncludeDirs()))
         .doesNotContain("foo/implementation_dep");
 
     CcCompilationContext publicDepCompilationContext =
         getCppCompileAction("//foo:public_dep").getCcCompilationContext();
     assertThat(artifactsToStrings(publicDepCompilationContext.getDeclaredIncludeSrcs()))
         .contains("src foo/interface_dep.h");
-    assertThat(pathfragmentsToStrings(publicDepCompilationContext.getSystemIncludeDirs()))
+    assertThat(pathfragmentsToStrings(publicDepCompilationContext.getIncludeDirs()))
         .contains("foo/interface_dep");
     assertThat(artifactsToStrings(publicDepCompilationContext.getDeclaredIncludeSrcs()))
         .contains("src foo/implementation_dep.h");
-    assertThat(pathfragmentsToStrings(publicDepCompilationContext.getSystemIncludeDirs()))
+    assertThat(pathfragmentsToStrings(publicDepCompilationContext.getIncludeDirs()))
         .contains("foo/implementation_dep");
   }
 

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

@@ -1398,11 +1398,11 @@ def _impl(ctx):
 
     List<String> mergedSystemIncludes =
         ((Depset) myInfo.getValue("merged_system_includes")).getSet(String.class).toList();
-    assertThat(mergedSystemIncludes).containsAtLeast("foo/bar", "a/dep1/baz", "a/dep2/qux");
+    assertThat(mergedSystemIncludes).contains("foo/bar");
 
     List<String> mergedIncludes =
         ((Depset) myInfo.getValue("merged_includes")).getSet(String.class).toList();
-    assertThat(mergedIncludes).contains("baz/qux");
+    assertThat(mergedIncludes).containsAtLeast("baz/qux", "a/dep1/baz", "a/dep2/qux");
 
     List<String> mergedQuoteIncludes =
         ((Depset) myInfo.getValue("merged_quote_includes")).getSet(String.class).toList();

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

@@ -1397,7 +1397,7 @@ public void testIncludesDirsOfTransitiveCcDepsGetPassedToCompileAction() throws
             Iterables.concat(
                 Iterables.transform(
                     rootedIncludePaths("package/foo/bar"),
-                    element -> ImmutableList.of("-isystem", element)))));
+                    element -> ImmutableList.of("-I" + element)))));
   }
 
   @Test