Skip to content

Commit 08936ae

Browse files
erenoncopybara-github
erenon
authored and
copybara-github
committed
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 #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. Closes #13107. PiperOrigin-RevId: 366220384
1 parent 47edc57 commit 08936ae

File tree

8 files changed

+190
-10
lines changed

8 files changed

+190
-10
lines changed

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,16 @@ public ImmutableList<PathFragment> getFrameworkIncludeDirs() {
312312
return commandLineCcCompilationContext.frameworkIncludeDirs;
313313
}
314314

315+
/**
316+
* Returns the immutable list of external include directories (possibly empty but never null).
317+
* This includes the include dirs from the transitive deps closure of the target. This list does
318+
* not contain duplicates. All fragments are either absolute or relative to the exec root (see
319+
* {@link com.google.devtools.build.lib.analysis.BlazeDirectories#getExecRoot(String)}).
320+
*/
321+
public ImmutableList<PathFragment> getExternalIncludeDirs() {
322+
return commandLineCcCompilationContext.externalIncludeDirs;
323+
}
324+
315325
/**
316326
* Returns the immutable set of declared include directories, relative to a "-I" or "-iquote"
317327
* directory" (possibly empty but never null).
@@ -648,6 +658,7 @@ static class CommandLineCcCompilationContext {
648658
private final ImmutableList<PathFragment> quoteIncludeDirs;
649659
private final ImmutableList<PathFragment> systemIncludeDirs;
650660
private final ImmutableList<PathFragment> frameworkIncludeDirs;
661+
private final ImmutableList<PathFragment> externalIncludeDirs;
651662
private final ImmutableList<String> defines;
652663
private final ImmutableList<String> localDefines;
653664

@@ -656,12 +667,14 @@ static class CommandLineCcCompilationContext {
656667
ImmutableList<PathFragment> quoteIncludeDirs,
657668
ImmutableList<PathFragment> systemIncludeDirs,
658669
ImmutableList<PathFragment> frameworkIncludeDirs,
670+
ImmutableList<PathFragment> externalIncludeDirs,
659671
ImmutableList<String> defines,
660672
ImmutableList<String> localDefines) {
661673
this.includeDirs = includeDirs;
662674
this.quoteIncludeDirs = quoteIncludeDirs;
663675
this.systemIncludeDirs = systemIncludeDirs;
664676
this.frameworkIncludeDirs = frameworkIncludeDirs;
677+
this.externalIncludeDirs = externalIncludeDirs;
665678
this.defines = defines;
666679
this.localDefines = localDefines;
667680
}
@@ -685,6 +698,8 @@ public static class Builder {
685698
private final TransitiveSetHelper<PathFragment> systemIncludeDirs = new TransitiveSetHelper<>();
686699
private final TransitiveSetHelper<PathFragment> frameworkIncludeDirs =
687700
new TransitiveSetHelper<>();
701+
private final TransitiveSetHelper<PathFragment> externalIncludeDirs =
702+
new TransitiveSetHelper<>();
688703
private final NestedSetBuilder<PathFragment> looseHdrsDirs = NestedSetBuilder.stableOrder();
689704
private final NestedSetBuilder<Artifact> declaredIncludeSrcs = NestedSetBuilder.stableOrder();
690705
private final NestedSetBuilder<Artifact> nonCodeInputs = NestedSetBuilder.stableOrder();
@@ -749,6 +764,7 @@ public Builder mergeDependentCcCompilationContext(
749764
quoteIncludeDirs.addTransitive(otherCcCompilationContext.getQuoteIncludeDirs());
750765
systemIncludeDirs.addTransitive(otherCcCompilationContext.getSystemIncludeDirs());
751766
frameworkIncludeDirs.addTransitive(otherCcCompilationContext.getFrameworkIncludeDirs());
767+
externalIncludeDirs.addTransitive(otherCcCompilationContext.getExternalIncludeDirs());
752768
looseHdrsDirs.addTransitive(otherCcCompilationContext.getLooseHdrsDirs());
753769
declaredIncludeSrcs.addTransitive(otherCcCompilationContext.getDeclaredIncludeSrcs());
754770
headerInfoBuilder.addDep(otherCcCompilationContext.headerInfo);
@@ -879,6 +895,26 @@ public Builder addFrameworkIncludeDirs(Iterable<PathFragment> frameworkIncludeDi
879895
return this;
880896
}
881897

898+
/**
899+
* Mark specified include directory as external, coming from an external workspace. It can be
900+
* added with "-isystem" (GCC) or --system-header-prefix (Clang) to suppress warnings coming
901+
* from external files.
902+
*/
903+
public Builder addExternalIncludeDir(PathFragment externalIncludeDir) {
904+
this.externalIncludeDirs.add(externalIncludeDir);
905+
return this;
906+
}
907+
908+
/**
909+
* Mark specified include directories as external, coming from an external workspace. These can
910+
* be added with "-isystem" (GCC) or --system-header-prefix (Clang) to suppress warnings coming
911+
* from external files.
912+
*/
913+
public Builder addExternalIncludeDirs(Iterable<PathFragment> externalIncludeDirs) {
914+
this.externalIncludeDirs.addAll(externalIncludeDirs);
915+
return this;
916+
}
917+
882918
/** Add a single declared include dir, relative to a "-I" or "-iquote" directory". */
883919
public Builder addLooseHdrsDir(PathFragment dir) {
884920
looseHdrsDirs.add(dir);
@@ -1023,6 +1059,7 @@ public CcCompilationContext build(ActionOwner owner, MiddlemanFactory middlemanF
10231059
quoteIncludeDirs.getMergedResult(),
10241060
systemIncludeDirs.getMergedResult(),
10251061
frameworkIncludeDirs.getMergedResult(),
1062+
externalIncludeDirs.getMergedResult(),
10261063
defines.getMergedResult(),
10271064
ImmutableList.copyOf(localDefines)),
10281065
constructedPrereq,

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

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1013,27 +1013,45 @@ private CcCompilationContext initializeCcCompilationContext(RuleContext ruleCont
10131013
boolean siblingRepositoryLayout = configuration.isSiblingRepositoryLayout();
10141014
RepositoryName repositoryName = label.getRepository();
10151015
PathFragment repositoryPath = repositoryName.getExecPath(siblingRepositoryLayout);
1016-
ccCompilationContextBuilder.addQuoteIncludeDir(repositoryPath);
1017-
ccCompilationContextBuilder.addQuoteIncludeDir(
1016+
PathFragment genIncludeDir =
10181017
siblingRepositoryLayout
10191018
? ruleContext.getGenfilesFragment()
1020-
: ruleContext.getGenfilesFragment().getRelative(repositoryPath));
1021-
ccCompilationContextBuilder.addQuoteIncludeDir(
1019+
: ruleContext.getGenfilesFragment().getRelative(repositoryPath);
1020+
PathFragment binIncludeDir =
10221021
siblingRepositoryLayout
10231022
? ruleContext.getBinFragment()
1024-
: ruleContext.getBinFragment().getRelative(repositoryPath));
1023+
: ruleContext.getBinFragment().getRelative(repositoryPath);
10251024

1026-
ccCompilationContextBuilder.addSystemIncludeDirs(systemIncludeDirs);
1027-
ccCompilationContextBuilder.addFrameworkIncludeDirs(frameworkIncludeDirs);
1025+
ccCompilationContextBuilder.addQuoteIncludeDir(repositoryPath);
1026+
ccCompilationContextBuilder.addQuoteIncludeDir(genIncludeDir);
1027+
ccCompilationContextBuilder.addQuoteIncludeDir(binIncludeDir);
10281028
ccCompilationContextBuilder.addQuoteIncludeDirs(quoteIncludeDirs);
1029+
ccCompilationContextBuilder.addFrameworkIncludeDirs(frameworkIncludeDirs);
10291030

1030-
for (PathFragment includeDir : includeDirs) {
1031-
ccCompilationContextBuilder.addIncludeDir(includeDir);
1031+
boolean external =
1032+
!repositoryName.isDefault()
1033+
&& !repositoryName.isMain()
1034+
&& featureConfiguration.isEnabled(CppRuleClasses.EXTERNAL_INCLUDE_PATHS);
1035+
1036+
if (external) {
1037+
ccCompilationContextBuilder.addExternalIncludeDir(repositoryPath);
1038+
ccCompilationContextBuilder.addExternalIncludeDir(genIncludeDir);
1039+
ccCompilationContextBuilder.addExternalIncludeDir(binIncludeDir);
1040+
ccCompilationContextBuilder.addExternalIncludeDirs(quoteIncludeDirs);
1041+
ccCompilationContextBuilder.addExternalIncludeDirs(systemIncludeDirs);
1042+
ccCompilationContextBuilder.addExternalIncludeDirs(includeDirs);
1043+
} else {
1044+
ccCompilationContextBuilder.addSystemIncludeDirs(systemIncludeDirs);
1045+
ccCompilationContextBuilder.addIncludeDirs(includeDirs);
10321046
}
10331047

10341048
PublicHeaders publicHeaders = computePublicHeaders(this.publicHeaders);
10351049
if (publicHeaders.getVirtualIncludePath() != null) {
1036-
ccCompilationContextBuilder.addIncludeDir(publicHeaders.getVirtualIncludePath());
1050+
if (external) {
1051+
ccCompilationContextBuilder.addExternalIncludeDir(publicHeaders.getVirtualIncludePath());
1052+
} else {
1053+
ccCompilationContextBuilder.addIncludeDir(publicHeaders.getVirtualIncludePath());
1054+
}
10371055
}
10381056

10391057
if (configuration.isCodeCoverageEnabled()) {
@@ -1696,6 +1714,7 @@ private CcToolchainVariables setupCompileBuildVariables(
16961714
getCopts(builder.getSourceFile(), sourceLabel),
16971715
dotdFileExecPath,
16981716
usePic,
1717+
ccCompilationContext.getExternalIncludeDirs(),
16991718
additionalBuildVariables);
17001719
return buildVariables.build();
17011720
}

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,13 @@ public enum CompileBuildVariables {
6464
* @see CcCompilationContext#getSystemIncludeDirs().
6565
*/
6666
SYSTEM_INCLUDE_PATHS("system_include_paths"),
67+
/**
68+
* Variable for the collection of external include paths.
69+
*
70+
* @see CcCompilationContext#getExternalIncludeDirs().
71+
*/
72+
EXTERNAL_INCLUDE_PATHS("external_include_paths"),
73+
6774
/**
6875
* Variable for the collection of framework include paths.
6976
*
@@ -313,6 +320,7 @@ private static CcToolchainVariables setupVariables(
313320
userCompileFlags,
314321
dotdFileExecPath,
315322
usePic,
323+
ImmutableList.of(),
316324
ImmutableMap.of());
317325
return buildVariables.build();
318326
}
@@ -331,6 +339,7 @@ public static void setupSpecificVariables(
331339
Iterable<String> userCompileFlags,
332340
String dotdFileExecPath,
333341
boolean usePic,
342+
ImmutableList<PathFragment> externalIncludeDirs,
334343
Map<String, String> additionalBuildVariables) {
335344
buildVariables.addStringSequenceVariable(
336345
USER_COMPILE_FLAGS.getVariableName(), userCompileFlags);
@@ -380,6 +389,12 @@ public static void setupSpecificVariables(
380389
buildVariables.addStringVariable(PIC.getVariableName(), "");
381390
}
382391

392+
if (!externalIncludeDirs.isEmpty()) {
393+
buildVariables.addStringSequenceVariable(
394+
EXTERNAL_INCLUDE_PATHS.getVariableName(),
395+
Iterables.transform(externalIncludeDirs, PathFragment::getSafePathString));
396+
}
397+
383398
buildVariables.addAllStringVariables(additionalBuildVariables);
384399
}
385400

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,9 @@ public static Label ccToolchainTypeAttribute(RuleDefinitionEnvironment env) {
245245
/** A string constant for the include_paths feature. */
246246
public static final String INCLUDE_PATHS = "include_paths";
247247

248+
/** A string constant for the external_include_paths feature. */
249+
public static final String EXTERNAL_INCLUDE_PATHS = "external_include_paths";
250+
248251
/** A string constant for the feature signalling static linking mode. */
249252
public static final String STATIC_LINKING_MODE = "static_linking_mode";
250253

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ _FEATURE_NAMES = struct(
9292
fission_flags_for_lto_backend = "fission_flags_for_lto_backend",
9393
min_os_version_flag = "min_os_version_flag",
9494
include_directories = "include_directories",
95+
external_include_paths = "external_include_paths",
9596
absolute_path_directories = "absolute_path_directories",
9697
from_package = "from_package",
9798
change_tool = "change_tool",
@@ -988,6 +989,22 @@ _include_directories_feature = feature(
988989
],
989990
)
990991

992+
_external_include_paths_feature = feature(
993+
name = _FEATURE_NAMES.external_include_paths,
994+
flag_sets = [
995+
flag_set(
996+
actions = [ACTION_NAMES.cpp_compile],
997+
flag_groups = [
998+
flag_group(
999+
flags = [
1000+
"-isystem",
1001+
],
1002+
),
1003+
],
1004+
),
1005+
],
1006+
)
1007+
9911008
_from_package_feature = feature(
9921009
name = _FEATURE_NAMES.from_package,
9931010
flag_sets = [
@@ -1260,6 +1277,7 @@ _feature_name_to_feature = {
12601277
_FEATURE_NAMES.fission_flags_for_lto_backend: _fission_flags_for_lto_backend_feature,
12611278
_FEATURE_NAMES.min_os_version_flag: _min_os_version_flag_feature,
12621279
_FEATURE_NAMES.include_directories: _include_directories_feature,
1280+
_FEATURE_NAMES.external_include_paths: _external_include_paths_feature,
12631281
_FEATURE_NAMES.from_package: _from_package_feature,
12641282
_FEATURE_NAMES.absolute_path_directories: _absolute_path_directories_feature,
12651283
_FEATURE_NAMES.change_tool: _change_tool_feature,

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,8 @@ java_test(
363363
deps = [
364364
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
365365
"//src/main/java/com/google/devtools/build/lib/rules/cpp",
366+
"//src/main/java/com/google/devtools/build/lib/vfs",
367+
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
366368
"//src/test/java/com/google/devtools/build/lib/analysis/util",
367369
"//src/test/java/com/google/devtools/build/lib/packages:testutil",
368370
"//third_party:guava",

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

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@
2323
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
2424
import com.google.devtools.build.lib.packages.util.Crosstool.CcToolchainConfig;
2525
import com.google.devtools.build.lib.packages.util.MockPlatformSupport;
26+
import com.google.devtools.build.lib.vfs.ModifiedFileSet;
27+
import com.google.devtools.build.lib.vfs.PathFragment;
28+
import com.google.devtools.build.lib.vfs.Root;
2629
import org.junit.Test;
2730
import org.junit.runner.RunWith;
2831
import org.junit.runners.JUnit4;
@@ -285,4 +288,60 @@ public void testPresenceOfMinOsVersionBuildVariable() throws Exception {
285288
assertThat(variables.getStringVariable(CcCommon.MINIMUM_OS_VERSION_VARIABLE_NAME))
286289
.isEqualTo("6");
287290
}
291+
292+
@Test
293+
public void testExternalIncludePathsVariable() throws Exception {
294+
AnalysisMock.get()
295+
.ccSupport()
296+
.setupCcToolchainConfig(
297+
mockToolsConfig,
298+
CcToolchainConfig.builder().withFeatures(CppRuleClasses.EXTERNAL_INCLUDE_PATHS));
299+
useConfiguration("--features=external_include_paths");
300+
scratch.appendFile("WORKSPACE", "local_repository(", " name = 'pkg',", " path = '/foo')");
301+
getSkyframeExecutor()
302+
.invalidateFilesUnderPathForTesting(
303+
reporter,
304+
new ModifiedFileSet.Builder().modify(PathFragment.create("WORKSPACE")).build(),
305+
Root.fromPath(rootDirectory));
306+
307+
scratch.file("/foo/WORKSPACE", "workspace(name = 'pkg')");
308+
scratch.file(
309+
"/foo/BUILD",
310+
"cc_library(name = 'foo',",
311+
" hdrs = ['foo.hpp'])",
312+
"cc_library(name = 'foo2',",
313+
" hdrs = ['foo.hpp'],",
314+
" include_prefix = 'prf')");
315+
scratch.file(
316+
"x/BUILD",
317+
"cc_library(name = 'bar',",
318+
" hdrs = ['bar.hpp'])",
319+
"cc_binary(name = 'bin',",
320+
" srcs = ['bin.cc'],",
321+
" deps = ['bar', '@pkg//:foo', '@pkg//:foo2'])");
322+
323+
CcToolchainVariables variables = getCompileBuildVariables("//x:bin", "bin");
324+
325+
ImmutableList.Builder<String> entries =
326+
ImmutableList.<String>builder()
327+
.add(
328+
"/k8-fastbuild/bin/external/pkg/_virtual_includes/foo2",
329+
"external/pkg",
330+
"/k8-fastbuild/bin/external/pkg");
331+
if (analysisMock.isThisBazel()) {
332+
entries.add("external/bazel_tools", "/k8-fastbuild/bin/external/bazel_tools");
333+
}
334+
335+
assertThat(
336+
CcToolchainVariables.toStringList(
337+
variables, CompileBuildVariables.EXTERNAL_INCLUDE_PATHS.getVariableName())
338+
.stream()
339+
.map(x -> removeOutDirectory(x))
340+
.collect(ImmutableList.toImmutableList()))
341+
.containsExactlyElementsIn(entries.build());
342+
}
343+
344+
private String removeOutDirectory(String s) {
345+
return s.replace("blaze-out", "").replace("bazel-out", "");
346+
}
288347
}

‎tools/cpp/unix_cc_toolchain_config.bzl

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -669,6 +669,32 @@ def _impl(ctx):
669669
],
670670
)
671671

672+
external_include_paths_feature = feature(
673+
name = "external_include_paths",
674+
flag_sets = [
675+
flag_set(
676+
actions = [
677+
ACTION_NAMES.preprocess_assemble,
678+
ACTION_NAMES.linkstamp_compile,
679+
ACTION_NAMES.c_compile,
680+
ACTION_NAMES.cpp_compile,
681+
ACTION_NAMES.cpp_header_parsing,
682+
ACTION_NAMES.cpp_module_compile,
683+
ACTION_NAMES.clif_match,
684+
ACTION_NAMES.objc_compile,
685+
ACTION_NAMES.objcpp_compile,
686+
],
687+
flag_groups = [
688+
flag_group(
689+
flags = ["-isystem", "%{external_include_paths}"],
690+
iterate_over = "external_include_paths",
691+
expand_if_available = "external_include_paths",
692+
),
693+
],
694+
),
695+
],
696+
)
697+
672698
symbol_counts_feature = feature(
673699
name = "symbol_counts",
674700
flag_sets = [
@@ -1167,6 +1193,7 @@ def _impl(ctx):
11671193
preprocessor_defines_feature,
11681194
includes_feature,
11691195
include_paths_feature,
1196+
external_include_paths_feature,
11701197
fdo_instrument_feature,
11711198
cs_fdo_instrument_feature,
11721199
cs_fdo_optimize_feature,

0 commit comments

Comments
 (0)