From 234d28e231de11e0da09d09cb0485c0de7f1318b Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 16 Mar 2015 14:04:16 +0000 Subject: Add a feature to put each header into it's own submodule. This will be required to keep the existing behavior when turning on C++ modules. The problem is that a #include of a header gets internally translated into an import of the header's (sub)module. Now, if all headers of a cc_library are in the same module, this means, a single import of one of them makes the symbols of all of them accessible. While this isn't inherently problematic, it breaks the similarity between a modules and non-modules build. This behavior is hidden behind a feature for now as I have just committed necessary changes to Clang, which will need to be release to stable, before we can flip everything. Also, don't put a header as both normal and private header if it is present in both. (Or don't output two submodules with the new implementation). -- MOS_MIGRATED_REVID=88723759 --- .../build/lib/rules/cpp/CcLibraryHelper.java | 3 +- .../build/lib/rules/cpp/CppModuleMapAction.java | 35 +++++++++++++++++----- .../build/lib/rules/cpp/CppRuleClasses.java | 30 ++++++++++++------- 3 files changed, 49 insertions(+), 19 deletions(-) (limited to 'src/main/java/com/google/devtools/build') diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java index a03fd9cd85..b530e9a04e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java @@ -760,7 +760,8 @@ public final class CcLibraryHelper { collectModuleMaps(), additionalExportedHeaders, featureConfiguration.isEnabled(CppRuleClasses.HEADER_MODULES), - featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAP_HOME_CWD)); + featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAP_HOME_CWD), + featureConfiguration.isEnabled(CppRuleClasses.GENERATE_SUBMODULES)); ruleContext.registerAction(action); } if (model.getGeneratesPicHeaderModule()) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java index 23005c37d1..a5b5a7ec0a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java @@ -30,6 +30,7 @@ import java.io.OutputStream; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collection; +import java.util.HashSet; import java.util.List; /** @@ -55,11 +56,12 @@ public class CppModuleMapAction extends AbstractFileWriteAction { private final ImmutableList dependencies; private final ImmutableList additionalExportedHeaders; private final boolean compiledModule; + private final boolean generateSubmodules; public CppModuleMapAction(ActionOwner owner, CppModuleMap cppModuleMap, Iterable privateHeaders, Iterable publicHeaders, Iterable dependencies, Iterable additionalExportedHeaders, - boolean compiledModule, boolean moduleMapHomeIsCwd) { + boolean compiledModule, boolean moduleMapHomeIsCwd, boolean generateSubmodules) { super(owner, ImmutableList.of(), cppModuleMap.getArtifact(), /*makeExecutable=*/false); this.cppModuleMap = cppModuleMap; @@ -69,6 +71,7 @@ public class CppModuleMapAction extends AbstractFileWriteAction { this.dependencies = ImmutableList.copyOf(dependencies); this.additionalExportedHeaders = ImmutableList.copyOf(additionalExportedHeaders); this.compiledModule = compiledModule; + this.generateSubmodules = generateSubmodules; } @Override @@ -85,15 +88,19 @@ public class CppModuleMapAction extends AbstractFileWriteAction { String leadingPeriods = moduleMapHomeIsCwd ? "" : Strings.repeat("../", segmentsToExecPath); content.append("module \"").append(cppModuleMap.getName()).append("\" {\n"); content.append(" export *\n"); + + HashSet deduper = new HashSet<>(); + for (Artifact artifact : publicHeaders) { + appendHeader( + content, "", artifact.getExecPath(), leadingPeriods, /*canCompile=*/true, deduper); + } for (Artifact artifact : privateHeaders) { appendHeader(content, "private", artifact.getExecPath(), leadingPeriods, - /*canCompile=*/true); - } - for (Artifact artifact : publicHeaders) { - appendHeader(content, "", artifact.getExecPath(), leadingPeriods, /*canCompile=*/true); + /*canCompile=*/true, deduper); } for (PathFragment additionalExportedHeader : additionalExportedHeaders) { - appendHeader(content, "", additionalExportedHeader, leadingPeriods, /*canCompile*/false); + appendHeader( + content, "", additionalExportedHeader, leadingPeriods, /*canCompile*/ false, deduper); } for (CppModuleMap dep : dependencies) { content.append(" use \"").append(dep.getName()).append("\"\n"); @@ -113,7 +120,15 @@ public class CppModuleMapAction extends AbstractFileWriteAction { } private void appendHeader(StringBuilder content, String visibilitySpecifier, PathFragment path, - String leadingPeriods, boolean canCompile) { + String leadingPeriods, boolean canCompile, HashSet deduper) { + if (deduper.contains(path)) { + return; + } + deduper.add(path); + if (generateSubmodules) { + content.append(" module \"").append(path).append("\" {\n"); + content.append(" export *\n "); + } content.append(" "); if (!visibilitySpecifier.isEmpty()) { content.append(visibilitySpecifier).append(" "); @@ -121,7 +136,11 @@ public class CppModuleMapAction extends AbstractFileWriteAction { if (!canCompile || !shouldCompileHeader(path)) { content.append("textual "); } - content.append("header \"").append(leadingPeriods).append(path).append("\"\n"); + content.append("header \"").append(leadingPeriods).append(path).append("\""); + if (generateSubmodules) { + content.append("\n }"); + } + content.append("\n"); } private boolean shouldCompileHeader(PathFragment path) { 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 34b4f1840e..9fa675d3dc 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 @@ -80,7 +80,7 @@ public class CppRuleClasses { * A string constant for the parse_headers feature. */ public static final String PARSE_HEADERS = "parse_headers"; - + /** * A string constant for the preprocess_headers feature. */ @@ -91,30 +91,40 @@ public class CppRuleClasses { * header_modules features. */ public static final String MODULE_MAPS = "module_maps"; - + /** * A string constant for the module_map_home_cwd feature. */ public static final String MODULE_MAP_HOME_CWD = "module_map_home_cwd"; - + /** * A string constant for the layering_check feature. */ public static final String LAYERING_CHECK = "layering_check"; - + /** * A string constant for the header_modules feature. */ public static final String HEADER_MODULES = "header_modules"; - + /** * A string constant for the use_header_modules feature. - * + * *

This feature is only used during rollout; we expect to default enable this once we * have verified that module-enabled compilation is stable enough. */ public static final String USE_HEADER_MODULES = "use_header_modules"; + /** + * A string constant for the generate_submodules feature. + * + *

This feature is only used temporarily to make the switch to using submodules easier. With + * submodules, each header of a cc_library is placed into a submodule of the module generated for + * the appropriate target. As this influences the layering_check semantics and needs to be synced + * with a clang release, we want to be able to switch back and forth easily. + */ + public static final String GENERATE_SUBMODULES = "generate_submodules"; + /** * A string constant for switching on that a header module file includes information about * all its dependencies, so we do not need to pass all transitive dependent header modules on @@ -122,12 +132,12 @@ public class CppRuleClasses { */ public static final String HEADER_MODULE_INCLUDES_DEPENDENCIES = "header_module_includes_dependencies"; - + /** * A string constant for the no_legacy_features feature. - * - *

If this feature is enabled, Bazel will not extend the crosstool configuration with the - * default legacy feature set. + * + *

If this feature is enabled, Bazel will not extend the crosstool configuration with the + * default legacy feature set. */ public static final String NO_LEGACY_FEATURES = "no_legacy_features"; } -- cgit v1.2.3