From df593e90a34dc379d1c762ee8ef83bc0207c130d Mon Sep 17 00:00:00 2001 From: Manuel Klimek Date: Tue, 6 Oct 2015 13:59:57 +0000 Subject: Add a feature to require explicitly passing module maps. Currently, module maps contain both "use " entries that specify which modules the current module map depends on, and "extern module" entries that provide paths where to load the dependent module maps from. This change adds a feature "module_map_without_extern_module", which instructs blaze to not write the "extern module" entries into the module map. Instead, the crosstool needs to add -fmodule-file flags for each dependent module file where needed for the compile via the new build variable "dependent_module_map_files". Note that the feature is phrased negatively ("_without_") in order to simplify the roll-out of this feature: as long as crosstools do not specify any features, they still want the old behavior. We cannot make the feature positive and add it to the legacy configuration, as we currently cannot remove features that have already been set in the crosstool file. -- MOS_MIGRATED_REVID=104757413 --- .../build/lib/rules/cpp/CcLibraryHelper.java | 21 ++++++---- .../build/lib/rules/cpp/CppCompilationContext.java | 2 +- .../devtools/build/lib/rules/cpp/CppModel.java | 6 +++ .../build/lib/rules/cpp/CppModuleMapAction.java | 48 +++++++++++++++------- .../build/lib/rules/cpp/CppRuleClasses.java | 12 ++++++ .../build/lib/rules/objc/CompilationSupport.java | 3 +- 6 files changed, 66 insertions(+), 26 deletions(-) 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 5e97bce53c..ac06521f8c 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 @@ -789,15 +789,18 @@ public final class CcLibraryHelper { // TODO(bazel-team): addCppModuleMapToContext second-guesses whether module maps should // actually be enabled, so we need to double-check here. Who would write code like this? if (cppModuleMap != null) { - CppModuleMapAction action = new CppModuleMapAction(ruleContext.getActionOwner(), - cppModuleMap, - privateHeaders, - publicHeaders, - collectModuleMaps(), - additionalExportedHeaders, - featureConfiguration.isEnabled(CppRuleClasses.HEADER_MODULES), - featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAP_HOME_CWD), - featureConfiguration.isEnabled(CppRuleClasses.GENERATE_SUBMODULES)); + CppModuleMapAction action = + new CppModuleMapAction( + ruleContext.getActionOwner(), + cppModuleMap, + privateHeaders, + publicHeaders, + collectModuleMaps(), + additionalExportedHeaders, + featureConfiguration.isEnabled(CppRuleClasses.HEADER_MODULES), + featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAP_HOME_CWD), + featureConfiguration.isEnabled(CppRuleClasses.GENERATE_SUBMODULES), + !featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAP_WITHOUT_EXTERN_MODULE)); ruleContext.registerAction(action); } if (model.getGeneratesPicHeaderModule()) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java index 7658794e6c..d8527fe71b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java @@ -273,7 +273,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider { /** * @return modules maps from direct dependencies. */ - private NestedSet getDirectModuleMaps() { + public NestedSet getDirectModuleMaps() { NestedSetBuilder builder = NestedSetBuilder.stableOrder(); for (DepsContext depsContext : depsContexts) { builder.addTransitive(depsContext.directModuleMaps); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java index bd7ea07a74..262dd3794e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java @@ -346,6 +346,12 @@ public final class CppModel { buildVariables.addVariable("module_name", cppModuleMap.getName()); buildVariables.addVariable("module_map_file", cppModuleMap.getArtifact().getExecPathString()); + CcToolchainFeatures.Variables.NestedSequence.Builder sequence = + new CcToolchainFeatures.Variables.NestedSequence.Builder(); + for (Artifact artifact : context.getDirectModuleMaps()) { + sequence.addValue(artifact.getExecPathString()); + } + buildVariables.addSequence("dependent_module_map_files", sequence.build()); } if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES)) { buildVariables.addSequenceVariable("module_files", getHeaderModulePaths(builder, usePic)); 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 05da7857a1..758a5bbd80 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 @@ -58,11 +58,19 @@ public class CppModuleMapAction extends AbstractFileWriteAction { 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 generateSubmodules) { + private final boolean externDependencies; + + public CppModuleMapAction( + ActionOwner owner, + CppModuleMap cppModuleMap, + Iterable privateHeaders, + Iterable publicHeaders, + Iterable dependencies, + Iterable additionalExportedHeaders, + boolean compiledModule, + boolean moduleMapHomeIsCwd, + boolean generateSubmodules, + boolean externDependencies) { super(owner, ImmutableList.of(), cppModuleMap.getArtifact(), /*makeExecutable=*/false); this.cppModuleMap = cppModuleMap; @@ -73,6 +81,7 @@ public class CppModuleMapAction extends AbstractFileWriteAction { this.additionalExportedHeaders = ImmutableList.copyOf(additionalExportedHeaders); this.compiledModule = compiledModule; this.generateSubmodules = generateSubmodules; + this.externDependencies = externDependencies; } @Override @@ -93,11 +102,16 @@ public class CppModuleMapAction extends AbstractFileWriteAction { HashSet deduper = new HashSet<>(); for (Artifact artifact : publicHeaders) { appendHeader( - content, "", artifact.getExecPath(), leadingPeriods, /*canCompile=*/true, deduper); + content, "", artifact.getExecPath(), leadingPeriods, /*canCompile=*/ true, deduper); } for (Artifact artifact : privateHeaders) { - appendHeader(content, "private", artifact.getExecPath(), leadingPeriods, - /*canCompile=*/true, deduper); + appendHeader( + content, + "private", + artifact.getExecPath(), + leadingPeriods, + /*canCompile=*/ true, + deduper); } for (PathFragment additionalExportedHeader : additionalExportedHeaders) { appendHeader( @@ -107,13 +121,16 @@ public class CppModuleMapAction extends AbstractFileWriteAction { content.append(" use \"").append(dep.getName()).append("\"\n"); } content.append("}"); - for (CppModuleMap dep : dependencies) { - content.append("\nextern module \"") - .append(dep.getName()) - .append("\" \"") - .append(leadingPeriods) - .append(dep.getArtifact().getExecPath()) - .append("\""); + if (externDependencies) { + for (CppModuleMap dep : dependencies) { + content + .append("\nextern module \"") + .append(dep.getName()) + .append("\" \"") + .append(leadingPeriods) + .append(dep.getArtifact().getExecPath()) + .append("\""); + } } out.write(content.toString().getBytes(StandardCharsets.ISO_8859_1)); } @@ -178,6 +195,7 @@ public class CppModuleMapAction extends AbstractFileWriteAction { f.addBoolean(moduleMapHomeIsCwd); f.addBoolean(compiledModule); f.addBoolean(generateSubmodules); + f.addBoolean(externDependencies); return f.hexDigestAndReset(); } 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 b7bfaf85ef..77a9719923 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 @@ -100,6 +100,18 @@ public class CppRuleClasses { */ public static final String MODULE_MAP_HOME_CWD = "module_map_home_cwd"; + /** + * A string constant for the module_map_without_extern_module feature. + * + *

This features is a transitional feature; enabling it means that generated module maps + * will not have "extern module" declarations inside them; instead, the module maps need + * to be passed via the dependent_module_map_files build variable. + * + *

This variable is phrased negatively to aid the roll-out: currently, the default is that + * "extern module" declarations are generated. + */ + public static final String MODULE_MAP_WITHOUT_EXTERN_MODULE = "module_map_without_extern_module"; + /** * A string constant for the layering_check feature. */ diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java index 6ba0442d15..1c92409383 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java @@ -675,7 +675,8 @@ public final class CompilationSupport { ImmutableList.of(), /*compiledModule=*/ true, /*moduleMapHomeIsCwd=*/ false, - /*generateSubModules=*/ false)); + /*generateSubModules=*/ false, + /*externDependencies=*/ true)); } private void registerLinkAction(ObjcProvider objcProvider, ExtraLinkArgs extraLinkArgs, -- cgit v1.2.3