From d85aaea5d606c659807fff7cfef75781a59860bc Mon Sep 17 00:00:00 2001 From: allevato Date: Tue, 27 Feb 2018 09:13:35 -0800 Subject: Add --incompatible_strict_objc_module_maps. This flag changes the behavior of objc_library module map propagation so that module maps are only propagated to direct dependents, not transitive dependents. swift_library targets that import Objective-C code must then list those dependencies directly in its deps instead of depending on them being transitively present. PiperOrigin-RevId: 187184692 --- .../lib/rules/objc/ObjcCommandLineOptions.java | 15 +++++- .../devtools/build/lib/rules/objc/ObjcCommon.java | 9 +++- .../build/lib/rules/objc/ObjcConfiguration.java | 11 +++- .../build/lib/rules/objc/AppleBinaryTest.java | 2 +- .../build/lib/rules/objc/ObjcLibraryTest.java | 2 +- .../build/lib/rules/objc/ObjcProtoLibraryTest.java | 27 ++++++---- .../build/lib/rules/objc/ObjcRuleTestCase.java | 59 ++++++++++++++++++---- 7 files changed, 98 insertions(+), 27 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommandLineOptions.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommandLineOptions.java index 64637c39d8..c836e345fe 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommandLineOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommandLineOptions.java @@ -304,13 +304,26 @@ public class ObjcCommandLineOptions extends FragmentOptions { defaultValue = "null", converter = LabelConverter.class, documentationCategory = OptionDocumentationCategory.TOOLCHAIN, - effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.LOADING_AND_ANALYSIS}, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.LOADING_AND_ANALYSIS}, help = "Location of target that will provide the appropriate Apple SDK for the current build " + "configuration." ) public Label appleSdk; + @Option( + name = "incompatible_strict_objc_module_maps", + category = "incompatible changes", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.TOOLCHAIN, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.LOADING_AND_ANALYSIS}, + help = + "Propagates Objective-C module maps only to direct dependencies in the 'objc' provider, " + + "not to all transitive dependencies." + ) + public boolean strictObjcModuleMaps; + @Override public FragmentOptions getHost() { ObjcCommandLineOptions host = (ObjcCommandLineOptions) super.getHost(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java index 3019d7a1a3..fbfa23e121 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java @@ -545,8 +545,13 @@ public final class ObjcCommon { if (umbrellaHeader.isPresent()) { objcProvider.add(UMBRELLA_HEADER, umbrellaHeader.get()); } - objcProvider.add(MODULE_MAP, moduleMap.getArtifact()); - objcProvider.add(TOP_LEVEL_MODULE_MAP, moduleMap); + if (context.getFragment(ObjcConfiguration.class).useStrictObjcModuleMaps()) { + objcProvider.addForDirectDependents(MODULE_MAP, moduleMap.getArtifact()); + objcProvider.addForDirectDependents(TOP_LEVEL_MODULE_MAP, moduleMap); + } else { + objcProvider.add(MODULE_MAP, moduleMap.getArtifact()); + objcProvider.add(TOP_LEVEL_MODULE_MAP, moduleMap); + } } objcProvider diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java index 6139c2a541..d95e40ca7c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java @@ -80,6 +80,7 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment { private final int objcHeaderThinningPartitionSize; private final Label objcHeaderScannerTool; private final Label appleSdk; + private final boolean strictObjcModuleMaps; ObjcConfiguration(ObjcCommandLineOptions objcOptions, BuildConfiguration.Options options) { this.iosSimulatorDevice = @@ -115,6 +116,7 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment { this.objcHeaderThinningPartitionSize = objcOptions.objcHeaderThinningPartitionSize; this.objcHeaderScannerTool = objcOptions.objcHeaderScannerTool; this.appleSdk = objcOptions.appleSdk; + this.strictObjcModuleMaps = objcOptions.strictObjcModuleMaps; } @AutoCodec.Instantiator @@ -142,7 +144,8 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment { boolean experimentalHeaderThinning, int objcHeaderThinningPartitionSize, Label objcHeaderScannerTool, - Label appleSdk) { + Label appleSdk, + boolean strictObjcModuleMaps) { this.iosSimulatorVersion = iosSimulatorVersion; this.iosSimulatorDevice = iosSimulatorDevice; this.watchosSimulatorVersion = watchosSimulatorVersion; @@ -167,6 +170,7 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment { this.objcHeaderThinningPartitionSize = objcHeaderThinningPartitionSize; this.objcHeaderScannerTool = objcHeaderScannerTool; this.appleSdk = appleSdk; + this.strictObjcModuleMaps = strictObjcModuleMaps; } /** @@ -375,4 +379,9 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment { public Label getAppleSdk() { return appleSdk; } + + /** Returns true if Objective-C module maps should only be propagated to direct dependencies. */ + public boolean useStrictObjcModuleMaps() { + return strictObjcModuleMaps; + } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java index b940580870..cc23c7cf83 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java @@ -1460,7 +1460,7 @@ public class AppleBinaryTest extends ObjcRuleTestCase { @Test public void testCustomModuleMap() throws Exception { - checkCustomModuleMap(getRuleType()); + checkCustomModuleMapNotPropagatedByTargetUnderTest(getRuleType()); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java index ec3dea8d41..0a83e02975 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java @@ -1928,7 +1928,7 @@ public class ObjcLibraryTest extends ObjcRuleTestCase { @Test public void testCustomModuleMap() throws Exception { - checkCustomModuleMap(RULE_TYPE); + checkCustomModuleMapPropagatedByTargetUnderTest(RULE_TYPE); } private boolean containsObjcFeature(String srcName) throws Exception { diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibraryTest.java index 955ce658a7..2dbd7db711 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibraryTest.java @@ -399,27 +399,32 @@ public class ObjcProtoLibraryTest extends ObjcRuleTestCase { @Test public void testModulemapCreatedForNonLinkingTargets() throws Exception { - checkOnlyLibModuleMapsArePresentForTarget("//package:opl_protobuf"); + // TODO(b/73943026): Remove this flag once everyone has migrated to the new strict behavior and + // it is made the default. + useConfiguration("--incompatible_strict_objc_module_maps"); + + // The library target should propagate its module map. + ObjcProvider provider = providerForTarget("//package:opl_protobuf"); + assertThat(Artifact.toRootRelativePaths(provider.get(ObjcProvider.MODULE_MAP).toSet())) + .containsExactly("package/opl_protobuf.modulemaps/module.modulemap"); } @Test public void testModulemapNotCreatedForLinkingTargets() throws Exception { - checkOnlyLibModuleMapsArePresentForTarget("//package:opl_binary"); + // TODO(b/73943026): Remove this flag once everyone has migrated to the new strict behavior and + // it is made the default. + useConfiguration("--incompatible_strict_objc_module_maps"); + + // The binary target should not propagate the module map from the library it depends on. + ObjcProvider provider = providerForTarget("//package:opl_binary"); + assertThat(Artifact.toRootRelativePaths(provider.get(ObjcProvider.MODULE_MAP).toSet())) + .isEmpty(); } private static String sortedJoin(Iterable elements) { return Joiner.on('\n').join(Ordering.natural().immutableSortedCopy(elements)); } - private void checkOnlyLibModuleMapsArePresentForTarget(String target) throws Exception { - ObjcProvider provider = providerForTarget(target); - assertThat(Artifact.toRootRelativePaths(provider.get(ObjcProvider.MODULE_MAP).toSet())) - .containsExactly( - "package/opl_protobuf.modulemaps/module.modulemap", - TestConstants.TOOLS_REPOSITORY_PATH_PREFIX - + "objcproto/protobuf_lib.modulemaps/module.modulemap"); - } - @Test public void testObjcProvider() throws Exception { ConfiguredTarget target = getConfiguredTarget("//package:opl_protobuf"); diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java index 6da95762ab..9a8dfff2db 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java @@ -3351,19 +3351,40 @@ public abstract class ObjcRuleTestCase extends BuildViewTestCase { .isEqualTo("a.o"); } - protected void checkCustomModuleMap(RuleType ruleType) throws Exception { - useConfiguration("--experimental_objc_enable_module_maps"); - ruleType.scratchTarget(scratch, "deps", "['//z:testModuleMap']"); + protected void checkCustomModuleMapNotPropagatedByTargetUnderTest( + RuleType ruleType) throws Exception { + checkCustomModuleMap(ruleType, false); + } + + protected void checkCustomModuleMapPropagatedByTargetUnderTest( + RuleType ruleType) throws Exception { + checkCustomModuleMap(ruleType, true); + } + + private void checkCustomModuleMap(RuleType ruleType, boolean targetUnderTestShouldPropagate) + throws Exception { + useConfiguration( + "--experimental_objc_enable_module_maps", "--incompatible_strict_objc_module_maps"); + ruleType.scratchTarget(scratch, "deps", "['//z:a']"); + scratch.file("z/a.m"); + scratch.file("z/a.h"); scratch.file("z/b.m"); scratch.file("z/b.h"); scratch.file("y/module.modulemap", "module my_module_b { export *\n header b.h }"); - scratch.file("z/BUILD", + scratch.file( + "z/BUILD", + "objc_library(", + "name = 'testModuleMap',", + "hdrs = ['b.h'],", + "srcs = ['b.m'],", + "module_map = '//y:mm'", + ")", "objc_library(", - "name = 'testModuleMap',", - "hdrs = ['b.h'],", - "srcs = ['b.m'],", - "module_map = '//y:mm'", - ")"); + "name = 'a',", + "hdrs = ['a.h'],", + "srcs = ['a.m'],", + "deps = [':testModuleMap']", + ")"); scratch.file("y/BUILD", "filegroup(", "name = 'mm',", @@ -3374,12 +3395,30 @@ public abstract class ObjcRuleTestCase extends BuildViewTestCase { assertThat(compileActionA.getArguments()).doesNotContain("-fmodule-maps"); assertThat(compileActionA.getArguments()).doesNotContain("-fmodule-name"); + String x8664Genfiles = + configurationGenfiles("x86_64", ConfigurationDistinguisher.UNKNOWN, null); + + // The target with the module map should propagate it to its direct dependers... ObjcProvider provider = providerForTarget("//z:testModuleMap"); assertThat(Artifact.toExecPaths(provider.get(MODULE_MAP))) .containsExactly("y/module.modulemap"); + // ...and the target depending on //z:testModuleMap will see it (as well as its own)... + provider = providerForTarget("//z:a"); + assertThat(Artifact.toExecPaths(provider.get(MODULE_MAP))) + .containsExactly(x8664Genfiles + "/z/a.modulemaps/module.modulemap", "y/module.modulemap"); + provider = providerForTarget("//x:x"); - assertThat(Artifact.toExecPaths(provider.get(MODULE_MAP))).contains("y/module.modulemap"); + if (targetUnderTestShouldPropagate) { + // ...and //x:x should propagate //z:a but not //z:testModuleMap. + assertThat(Artifact.toExecPaths(provider.get(MODULE_MAP))) + .containsExactly( + x8664Genfiles + "/x/x.modulemaps/module.modulemap", + x8664Genfiles + "/z/a.modulemaps/module.modulemap"); + } else { + // ...but //x:x should not see them. + assertThat(Artifact.toExecPaths(provider.get(MODULE_MAP))).isEmpty(); + } } /** -- cgit v1.2.3