Add external_include_paths C++ feature

Projects with strict warnings level experience issues when they
depend on third party libraries with less-strict warnings:
The compiler will emit warnings for external sources.
See bazelbuild#12009 for more.

This change optionally silences those warnings, by changing -I flags to
-isystem flags and by adding -isystem flags for each -iquote flag, in
case of external dependencies, i.e: those targets that come from a
non-main workspace.

The new behavior can be enabled by --features=external_include_paths,
otherwise there's no functional change.

The default flag_group in unix_cc_toolchain_config will silence warnings
coming from -I and -iquote dirs in case of GCC, and -I for Clang. Clang
will still produce warnings for -iquote dirs, therefore the Clang
specific --system-header-prefix is recommended instead.

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

@@ -312,6 +312,17 @@ public ImmutableList<PathFragment> getFrameworkIncludeDirs() {
     return commandLineCcCompilationContext.frameworkIncludeDirs;
   }
 
+  /**
+   * Returns the immutable list of external include directories (possibly empty but never null).
+   * This includes the include dirs from the transitive deps closure of the target.
+   * This list does not contain duplicates. All fragments are either absolute or relative to the
+   * exec root (see {@link
+   * com.google.devtools.build.lib.analysis.BlazeDirectories#getExecRoot(String)}).
+   */
+  public ImmutableList<PathFragment> getExternalIncludeDirs() {
+    return commandLineCcCompilationContext.externalIncludeDirs;
+  }
+
   /**
    * Returns the immutable set of declared include directories, relative to a "-I" or "-iquote"
    * directory" (possibly empty but never null).
@@ -648,6 +659,7 @@ static class CommandLineCcCompilationContext {
     private final ImmutableList<PathFragment> quoteIncludeDirs;
     private final ImmutableList<PathFragment> systemIncludeDirs;
     private final ImmutableList<PathFragment> frameworkIncludeDirs;
+    private final ImmutableList<PathFragment> externalIncludeDirs;
     private final ImmutableList<String> defines;
     private final ImmutableList<String> localDefines;
 
@@ -656,12 +668,14 @@ static class CommandLineCcCompilationContext {
         ImmutableList<PathFragment> quoteIncludeDirs,
         ImmutableList<PathFragment> systemIncludeDirs,
         ImmutableList<PathFragment> frameworkIncludeDirs,
+        ImmutableList<PathFragment> externalIncludeDirs,
         ImmutableList<String> defines,
         ImmutableList<String> localDefines) {
       this.includeDirs = includeDirs;
       this.quoteIncludeDirs = quoteIncludeDirs;
       this.systemIncludeDirs = systemIncludeDirs;
       this.frameworkIncludeDirs = frameworkIncludeDirs;
+      this.externalIncludeDirs = externalIncludeDirs;
       this.defines = defines;
       this.localDefines = localDefines;
     }
@@ -685,6 +699,8 @@ public static class Builder {
     private final TransitiveSetHelper<PathFragment> systemIncludeDirs = new TransitiveSetHelper<>();
     private final TransitiveSetHelper<PathFragment> frameworkIncludeDirs =
         new TransitiveSetHelper<>();
+    private final TransitiveSetHelper<PathFragment> externalIncludeDirs =
+        new TransitiveSetHelper<>();
     private final NestedSetBuilder<PathFragment> looseHdrsDirs = NestedSetBuilder.stableOrder();
     private final NestedSetBuilder<Artifact> declaredIncludeSrcs = NestedSetBuilder.stableOrder();
     private final NestedSetBuilder<Artifact> nonCodeInputs = NestedSetBuilder.stableOrder();
@@ -749,6 +765,7 @@ public Builder mergeDependentCcCompilationContext(
       quoteIncludeDirs.addTransitive(otherCcCompilationContext.getQuoteIncludeDirs());
       systemIncludeDirs.addTransitive(otherCcCompilationContext.getSystemIncludeDirs());
       frameworkIncludeDirs.addTransitive(otherCcCompilationContext.getFrameworkIncludeDirs());
+      externalIncludeDirs.addTransitive(otherCcCompilationContext.getExternalIncludeDirs());
       looseHdrsDirs.addTransitive(otherCcCompilationContext.getLooseHdrsDirs());
       declaredIncludeSrcs.addTransitive(otherCcCompilationContext.getDeclaredIncludeSrcs());
       headerInfoBuilder.addDep(otherCcCompilationContext.headerInfo);
@@ -873,12 +890,33 @@ public Builder addSystemIncludeDirs(Iterable<PathFragment> systemIncludeDirs) {
       return this;
     }
 
+
     /** Add framewrok include directories to be added with "-F". */
     public Builder addFrameworkIncludeDirs(Iterable<PathFragment> frameworkIncludeDirs) {
       this.frameworkIncludeDirs.addAll(frameworkIncludeDirs);
       return this;
     }
 
+    /**
+     * Mark specified include directory as external, coming from an external workspace.
+     * It can be added with "-isystem" (GCC) or --system-header-prefix (Clang) to suppress
+     * warnings coming from external files.
+     */
+    public Builder addExternalIncludeDir(PathFragment externalIncludeDir) {
+      this.externalIncludeDirs.add(externalIncludeDir);
+      return this;
+    }
+
+    /**
+     * Mark specified include directories as external, coming from an external workspace.
+     * These can be added with "-isystem" (GCC) or --system-header-prefix (Clang) to suppress
+     * warnings coming from external files.
+     */
+    public Builder addExternalIncludeDirs(Iterable<PathFragment> externalIncludeDirs) {
+      this.externalIncludeDirs.addAll(externalIncludeDirs);
+      return this;
+    }
+
     /** Add a single declared include dir, relative to a "-I" or "-iquote" directory". */
     public Builder addLooseHdrsDir(PathFragment dir) {
       looseHdrsDirs.add(dir);
@@ -1023,6 +1061,7 @@ public CcCompilationContext build(ActionOwner owner, MiddlemanFactory middlemanF
               quoteIncludeDirs.getMergedResult(),
               systemIncludeDirs.getMergedResult(),
               frameworkIncludeDirs.getMergedResult(),
+              externalIncludeDirs.getMergedResult(),
               defines.getMergedResult(),
               ImmutableList.copyOf(localDefines)),
           constructedPrereq,

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

@@ -1011,23 +1011,38 @@ private CcCompilationContext initializeCcCompilationContext(RuleContext ruleCont
     boolean siblingRepositoryLayout = configuration.isSiblingRepositoryLayout();
     RepositoryName repositoryName = label.getRepository();
     PathFragment repositoryPath = repositoryName.getExecPath(siblingRepositoryLayout);
+    PathFragment genIncludeDir = ruleContext.getGenfilesFragment().getRelative(repositoryPath);
+    PathFragment binIncludeDir = ruleContext.getBinFragment().getRelative(repositoryPath);
     ccCompilationContextBuilder.addQuoteIncludeDir(repositoryPath);
-    ccCompilationContextBuilder.addQuoteIncludeDir(
-        ruleContext.getGenfilesFragment().getRelative(repositoryPath));
-    ccCompilationContextBuilder.addQuoteIncludeDir(
-        ruleContext.getBinFragment().getRelative(repositoryPath));
-
-    ccCompilationContextBuilder.addSystemIncludeDirs(systemIncludeDirs);
-    ccCompilationContextBuilder.addFrameworkIncludeDirs(frameworkIncludeDirs);
+    ccCompilationContextBuilder.addQuoteIncludeDir(genIncludeDir);
+    ccCompilationContextBuilder.addQuoteIncludeDir(binIncludeDir);
     ccCompilationContextBuilder.addQuoteIncludeDirs(quoteIncludeDirs);
+    ccCompilationContextBuilder.addFrameworkIncludeDirs(frameworkIncludeDirs);
 
-    for (PathFragment includeDir : includeDirs) {
-      ccCompilationContextBuilder.addIncludeDir(includeDir);
+    boolean external =
+      !repositoryName.isDefault() &&
+      !repositoryName.isMain() &&
+      featureConfiguration.isEnabled(CppRuleClasses.EXTERNAL_INCLUDE_PATHS);
+
+    if (external) {
+      ccCompilationContextBuilder.addExternalIncludeDir(repositoryPath);
+      ccCompilationContextBuilder.addExternalIncludeDir(genIncludeDir);
+      ccCompilationContextBuilder.addExternalIncludeDir(binIncludeDir);
+      ccCompilationContextBuilder.addExternalIncludeDirs(quoteIncludeDirs);
+      ccCompilationContextBuilder.addExternalIncludeDirs(systemIncludeDirs);
+      ccCompilationContextBuilder.addExternalIncludeDirs(includeDirs);
+    } else {
+      ccCompilationContextBuilder.addSystemIncludeDirs(systemIncludeDirs);
+      ccCompilationContextBuilder.addIncludeDirs(includeDirs);
     }
 
     PublicHeaders publicHeaders = computePublicHeaders(this.publicHeaders);
     if (publicHeaders.getVirtualIncludePath() != null) {
-      ccCompilationContextBuilder.addIncludeDir(publicHeaders.getVirtualIncludePath());
+      if (external) {
+        ccCompilationContextBuilder.addExternalIncludeDir(publicHeaders.getVirtualIncludePath());
+      } else {
+        ccCompilationContextBuilder.addIncludeDir(publicHeaders.getVirtualIncludePath());
+      }
     }
 
     if (configuration.isCodeCoverageEnabled()) {
@@ -1690,6 +1705,7 @@ private CcToolchainVariables setupCompileBuildVariables(
         getCopts(builder.getSourceFile(), sourceLabel),
         dotdFileExecPath,
         usePic,
+        ccCompilationContext.getExternalIncludeDirs(),
         additionalBuildVariables);
     return buildVariables.build();
   }

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

@@ -64,6 +64,13 @@ public enum CompileBuildVariables {
    * @see CcCompilationContext#getSystemIncludeDirs().
    */
   SYSTEM_INCLUDE_PATHS("system_include_paths"),
+  /**
+   * Variable for the collection of external include paths.
+   *
+   * @see CcCompilationContext#getExternalIncludeDirs().
+   */
+  EXTERNAL_INCLUDE_PATHS("external_include_paths"),
+
   /**
    * Variable for the collection of framework include paths.
    *
@@ -313,6 +320,7 @@ private static CcToolchainVariables setupVariables(
         userCompileFlags,
         dotdFileExecPath,
         usePic,
+        ImmutableList.of(),
         ImmutableMap.of());
     return buildVariables.build();
   }
@@ -331,6 +339,7 @@ public static void setupSpecificVariables(
       Iterable<String> userCompileFlags,
       String dotdFileExecPath,
       boolean usePic,
+      ImmutableList<PathFragment> externalIncludeDirs,
       Map<String, String> additionalBuildVariables) {
     buildVariables.addStringSequenceVariable(
         USER_COMPILE_FLAGS.getVariableName(), userCompileFlags);
@@ -380,6 +389,10 @@ public static void setupSpecificVariables(
       buildVariables.addStringVariable(PIC.getVariableName(), "");
     }
 
+    buildVariables.addStringSequenceVariable(
+        EXTERNAL_INCLUDE_PATHS.getVariableName(),
+        Iterables.transform(externalIncludeDirs, PathFragment::getSafePathString));
+
     buildVariables.addAllStringVariables(additionalBuildVariables);
   }
 

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

@@ -245,6 +245,9 @@ public static Label ccToolchainTypeAttribute(RuleDefinitionEnvironment env) {
   /** A string constant for the include_paths feature. */
   public static final String INCLUDE_PATHS = "include_paths";
 
+  /** A string constant for the external_include_paths feature. */
+  public static final String EXTERNAL_INCLUDE_PATHS = "external_include_paths";
+
   /** A string constant for the feature signalling static linking mode. */
   public static final String STATIC_LINKING_MODE = "static_linking_mode";
 

src/test/java/com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl

@@ -92,6 +92,7 @@ _FEATURE_NAMES = struct(
     fission_flags_for_lto_backend = "fission_flags_for_lto_backend",
     min_os_version_flag = "min_os_version_flag",
     include_directories = "include_directories",
+    external_include_paths = "external_include_paths",
     absolute_path_directories = "absolute_path_directories",
     from_package = "from_package",
     change_tool = "change_tool",
@@ -988,6 +989,22 @@ _include_directories_feature = feature(
     ],
 )
 
+_external_include_paths_feature = feature(
+    name = _FEATURE_NAMES.external_include_paths,
+    flag_sets = [
+        flag_set(
+            actions = [ACTION_NAMES.cpp_compile],
+            flag_groups = [
+                flag_group(
+                    flags = [
+                        "-isystem",
+                    ],
+                ),
+            ],
+        ),
+    ],
+)
+
 _from_package_feature = feature(
     name = _FEATURE_NAMES.from_package,
     flag_sets = [
@@ -1260,6 +1277,7 @@ _feature_name_to_feature = {
     _FEATURE_NAMES.fission_flags_for_lto_backend: _fission_flags_for_lto_backend_feature,
     _FEATURE_NAMES.min_os_version_flag: _min_os_version_flag_feature,
     _FEATURE_NAMES.include_directories: _include_directories_feature,
+    _FEATURE_NAMES.external_include_paths: _external_include_paths_feature,
     _FEATURE_NAMES.from_package: _from_package_feature,
     _FEATURE_NAMES.absolute_path_directories: _absolute_path_directories_feature,
     _FEATURE_NAMES.change_tool: _change_tool_feature,

src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD

@@ -374,6 +374,8 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/rules/cpp",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
         "//src/test/java/com/google/devtools/build/lib/packages:testutil",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//third_party:guava",
         "//third_party:junit4",
         "//third_party:truth",

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

@@ -23,6 +23,9 @@
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.packages.util.Crosstool.CcToolchainConfig;
 import com.google.devtools.build.lib.packages.util.MockPlatformSupport;
+import com.google.devtools.build.lib.vfs.ModifiedFileSet;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Root;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -285,4 +288,49 @@ public void testPresenceOfMinOsVersionBuildVariable() throws Exception {
     assertThat(variables.getStringVariable(CcCommon.MINIMUM_OS_VERSION_VARIABLE_NAME))
         .isEqualTo("6");
   }
+
+  @Test
+  public void testExternalIncludePathsVariable() throws Exception {
+    AnalysisMock.get()
+        .ccSupport()
+        .setupCcToolchainConfig(
+            mockToolsConfig,
+            CcToolchainConfig.builder()
+                .withFeatures(CppRuleClasses.EXTERNAL_INCLUDE_PATHS));
+    useConfiguration("--features=external_include_paths");
+    scratch.appendFile("WORKSPACE",
+      "local_repository(",
+      "    name = 'pkg',",
+      "    path = '/foo')");
+    getSkyframeExecutor()
+        .invalidateFilesUnderPathForTesting(
+            reporter,
+            new ModifiedFileSet.Builder().modify(PathFragment.create("WORKSPACE")).build(),
+            Root.fromPath(rootDirectory));
+
+    scratch.file("/foo/WORKSPACE", "workspace(name = 'pkg')");
+    scratch.file(
+        "/foo/BUILD",
+        "cc_library(name = 'foo',",
+        "           hdrs = ['foo.hpp'])",
+        "cc_library(name = 'foo2',",
+        "           hdrs = ['foo.hpp'],",
+        "           include_prefix = 'prf')");
+    scratch.file(
+        "x/BUILD",
+        "cc_library(name = 'bar',",
+        "           hdrs = ['bar.hpp'])",
+        "cc_binary(name = 'bin',",
+        "          srcs = ['bin.cc'],",
+        "          deps = ['bar', '@pkg//:foo', '@pkg//:foo2'])");
+
+    CcToolchainVariables variables = getCompileBuildVariables("//x:bin", "bin");
+    assertThat(CcToolchainVariables.toStringList(variables, CompileBuildVariables.EXTERNAL_INCLUDE_PATHS.getVariableName()))
+        .containsExactly(
+             "bazel-out/k8-fastbuild/bin/external/pkg/_virtual_includes/foo2",
+             "external/pkg",
+             "bazel-out/k8-fastbuild/bin/external/pkg",
+             "external/bazel_tools",
+             "bazel-out/k8-fastbuild/bin/external/bazel_tools");
+  }
 }

tools/cpp/unix_cc_toolchain_config.bzl

@@ -669,6 +669,32 @@ def _impl(ctx):
         ],
     )
 
+    external_include_paths_feature = feature(
+        name = "external_include_paths",
+        flag_sets = [
+            flag_set(
+                actions = [
+                    ACTION_NAMES.preprocess_assemble,
+                    ACTION_NAMES.linkstamp_compile,
+                    ACTION_NAMES.c_compile,
+                    ACTION_NAMES.cpp_compile,
+                    ACTION_NAMES.cpp_header_parsing,
+                    ACTION_NAMES.cpp_module_compile,
+                    ACTION_NAMES.clif_match,
+                    ACTION_NAMES.objc_compile,
+                    ACTION_NAMES.objcpp_compile,
+                ],
+                flag_groups = [
+                    flag_group(
+                        flags = ["-isystem", "%{external_include_paths}"],
+                        iterate_over = "external_include_paths",
+                        expand_if_available = "external_include_paths",
+                    ),
+                ],
+            ),
+        ],
+    )
+
     symbol_counts_feature = feature(
         name = "symbol_counts",
         flag_sets = [
@@ -1167,6 +1193,7 @@ def _impl(ctx):
             preprocessor_defines_feature,
             includes_feature,
             include_paths_feature,
+            external_include_paths_feature,
             fdo_instrument_feature,
             cs_fdo_instrument_feature,
             cs_fdo_optimize_feature,