From 08936aecb96f2937c61bdedfebcf1c5a41a0786d Mon Sep 17 00:00:00 2001 From: Benedek Thaler Date: Thu, 1 Apr 2021 03:33:34 -0700 Subject: [PATCH] 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 --- .../lib/rules/cpp/CcCompilationContext.java | 37 ++++++++++++ .../lib/rules/cpp/CcCompilationHelper.java | 39 ++++++++---- .../lib/rules/cpp/CompileBuildVariables.java | 15 +++++ .../build/lib/rules/cpp/CppRuleClasses.java | 3 + .../lib/analysis/mock/cc_toolchain_config.bzl | 18 ++++++ .../google/devtools/build/lib/rules/cpp/BUILD | 2 + .../rules/cpp/CompileBuildVariablesTest.java | 59 +++++++++++++++++++ tools/cpp/unix_cc_toolchain_config.bzl | 27 +++++++++ 8 files changed, 190 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java index b2f1e4e2a31278..a9c060074594a3 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java @@ -312,6 +312,16 @@ public ImmutableList 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 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 +658,7 @@ static class CommandLineCcCompilationContext { private final ImmutableList quoteIncludeDirs; private final ImmutableList systemIncludeDirs; private final ImmutableList frameworkIncludeDirs; + private final ImmutableList externalIncludeDirs; private final ImmutableList defines; private final ImmutableList localDefines; @@ -656,12 +667,14 @@ static class CommandLineCcCompilationContext { ImmutableList quoteIncludeDirs, ImmutableList systemIncludeDirs, ImmutableList frameworkIncludeDirs, + ImmutableList externalIncludeDirs, ImmutableList defines, ImmutableList localDefines) { this.includeDirs = includeDirs; this.quoteIncludeDirs = quoteIncludeDirs; this.systemIncludeDirs = systemIncludeDirs; this.frameworkIncludeDirs = frameworkIncludeDirs; + this.externalIncludeDirs = externalIncludeDirs; this.defines = defines; this.localDefines = localDefines; } @@ -685,6 +698,8 @@ public static class Builder { private final TransitiveSetHelper systemIncludeDirs = new TransitiveSetHelper<>(); private final TransitiveSetHelper frameworkIncludeDirs = new TransitiveSetHelper<>(); + private final TransitiveSetHelper externalIncludeDirs = + new TransitiveSetHelper<>(); private final NestedSetBuilder looseHdrsDirs = NestedSetBuilder.stableOrder(); private final NestedSetBuilder declaredIncludeSrcs = NestedSetBuilder.stableOrder(); private final NestedSetBuilder nonCodeInputs = NestedSetBuilder.stableOrder(); @@ -749,6 +764,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); @@ -879,6 +895,26 @@ public Builder addFrameworkIncludeDirs(Iterable frameworkIncludeDi 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 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 +1059,7 @@ public CcCompilationContext build(ActionOwner owner, MiddlemanFactory middlemanF quoteIncludeDirs.getMergedResult(), systemIncludeDirs.getMergedResult(), frameworkIncludeDirs.getMergedResult(), + externalIncludeDirs.getMergedResult(), defines.getMergedResult(), ImmutableList.copyOf(localDefines)), constructedPrereq, diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java index a6b17aebb0a29a..21b8c617581768 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java @@ -1013,27 +1013,45 @@ private CcCompilationContext initializeCcCompilationContext(RuleContext ruleCont boolean siblingRepositoryLayout = configuration.isSiblingRepositoryLayout(); RepositoryName repositoryName = label.getRepository(); PathFragment repositoryPath = repositoryName.getExecPath(siblingRepositoryLayout); - ccCompilationContextBuilder.addQuoteIncludeDir(repositoryPath); - ccCompilationContextBuilder.addQuoteIncludeDir( + PathFragment genIncludeDir = siblingRepositoryLayout ? ruleContext.getGenfilesFragment() - : ruleContext.getGenfilesFragment().getRelative(repositoryPath)); - ccCompilationContextBuilder.addQuoteIncludeDir( + : ruleContext.getGenfilesFragment().getRelative(repositoryPath); + PathFragment binIncludeDir = siblingRepositoryLayout ? ruleContext.getBinFragment() - : ruleContext.getBinFragment().getRelative(repositoryPath)); + : ruleContext.getBinFragment().getRelative(repositoryPath); - ccCompilationContextBuilder.addSystemIncludeDirs(systemIncludeDirs); - ccCompilationContextBuilder.addFrameworkIncludeDirs(frameworkIncludeDirs); + ccCompilationContextBuilder.addQuoteIncludeDir(repositoryPath); + 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()) { @@ -1696,6 +1714,7 @@ private CcToolchainVariables setupCompileBuildVariables( getCopts(builder.getSourceFile(), sourceLabel), dotdFileExecPath, usePic, + ccCompilationContext.getExternalIncludeDirs(), additionalBuildVariables); return buildVariables.build(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariables.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariables.java index 0fea5934f4e805..866706acc14a01 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariables.java +++ b/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 userCompileFlags, String dotdFileExecPath, boolean usePic, + ImmutableList externalIncludeDirs, Map additionalBuildVariables) { buildVariables.addStringSequenceVariable( USER_COMPILE_FLAGS.getVariableName(), userCompileFlags); @@ -380,6 +389,12 @@ public static void setupSpecificVariables( buildVariables.addStringVariable(PIC.getVariableName(), ""); } + if (!externalIncludeDirs.isEmpty()) { + buildVariables.addStringSequenceVariable( + EXTERNAL_INCLUDE_PATHS.getVariableName(), + Iterables.transform(externalIncludeDirs, PathFragment::getSafePathString)); + } + buildVariables.addAllStringVariables(additionalBuildVariables); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java index b468bf67f155d3..3227396811e96a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java +++ b/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"; diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl b/src/test/java/com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl index e03c21205074ea..7301b717311b2e 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl +++ b/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, diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD index 2b9b8c08de7bcc..96f0d70bb21daf 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD @@ -363,6 +363,8 @@ java_test( deps = [ "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/rules/cpp", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/test/java/com/google/devtools/build/lib/analysis/util", "//src/test/java/com/google/devtools/build/lib/packages:testutil", "//third_party:guava", diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariablesTest.java index de4071552f06c9..b4d89ccd197813 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariablesTest.java +++ b/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,60 @@ 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"); + + ImmutableList.Builder entries = + ImmutableList.builder() + .add( + "/k8-fastbuild/bin/external/pkg/_virtual_includes/foo2", + "external/pkg", + "/k8-fastbuild/bin/external/pkg"); + if (analysisMock.isThisBazel()) { + entries.add("external/bazel_tools", "/k8-fastbuild/bin/external/bazel_tools"); + } + + assertThat( + CcToolchainVariables.toStringList( + variables, CompileBuildVariables.EXTERNAL_INCLUDE_PATHS.getVariableName()) + .stream() + .map(x -> removeOutDirectory(x)) + .collect(ImmutableList.toImmutableList())) + .containsExactlyElementsIn(entries.build()); + } + + private String removeOutDirectory(String s) { + return s.replace("blaze-out", "").replace("bazel-out", ""); + } } diff --git a/tools/cpp/unix_cc_toolchain_config.bzl b/tools/cpp/unix_cc_toolchain_config.bzl index 5dbaa86ab2ac0b..671d626e6e3667 100644 --- a/tools/cpp/unix_cc_toolchain_config.bzl +++ b/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,