Make the proto bundling behavior the default when using the new library. Take 2

--
MOS_MIGRATED_REVID=130406840

8 files changed, 97 insertions, 99 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleBinary.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleBinary.java
index 1d3ac278c7..2dbaa57487 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleBinary.java
@@ -94,16 +94,13 @@ public class AppleBinary implements RuleConfiguredTargetFactory {
     ObjcProvider.Builder objcProviderBuilder = new ObjcProvider.Builder();
 
     for (BuildConfiguration childConfig : childConfigurations) {
-      Optional<ObjcProvider> protosObjcProvider = Optional.absent();
       ObjcConfiguration objcConfiguration = childConfig.getFragment(ObjcConfiguration.class);
-      if (objcConfiguration.experimentalAutoTopLevelUnionObjCProtos()) {
-        ProtobufSupport protoSupport =
-            new ProtobufSupport(ruleContext)
-                .registerGenerationActions()
-                .registerCompilationActions();
+      ProtobufSupport protoSupport =
+          new ProtobufSupport(ruleContext, childConfig)
+              .registerGenerationActions()
+              .registerCompilationActions();
 
-        protosObjcProvider = Optional.of(protoSupport.getObjcProvider());
-      }
+      Optional<ObjcProvider> protosObjcProvider = protoSupport.getObjcProvider();
 
       IntermediateArtifacts intermediateArtifacts =
           ObjcRuleClasses.intermediateArtifacts(ruleContext, childConfig);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/BinaryLinkingTargetFactory.java b/src/main/java/com/google/devtools/build/lib/rules/objc/BinaryLinkingTargetFactory.java
index c584268458..88ac93a54d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/BinaryLinkingTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/BinaryLinkingTargetFactory.java
@@ -72,17 +72,11 @@ abstract class BinaryLinkingTargetFactory implements RuleConfiguredTargetFactory
   @Override
   public final ConfiguredTarget create(RuleContext ruleContext)
       throws InterruptedException, RuleErrorException {
-    ObjcConfiguration objcConfiguration = ObjcRuleClasses.objcConfiguration(ruleContext);
-
-    Optional<ObjcProvider> protosObjcProvider = Optional.absent();
-    Optional<XcodeProvider> protosXcodeProvider = Optional.absent();
-    if (objcConfiguration.experimentalAutoTopLevelUnionObjCProtos()) {
-      ProtobufSupport protoSupport =
-          new ProtobufSupport(ruleContext).registerGenerationActions().registerCompilationActions();
+    ProtobufSupport protoSupport =
+        new ProtobufSupport(ruleContext).registerGenerationActions().registerCompilationActions();
 
-      protosObjcProvider = Optional.of(protoSupport.getObjcProvider());
-      protosXcodeProvider = Optional.of(protoSupport.getXcodeProvider());
-    }
+    Optional<ObjcProvider> protosObjcProvider = protoSupport.getObjcProvider();
+    Optional<XcodeProvider> protosXcodeProvider = protoSupport.getXcodeProvider();
 
     ObjcCommon common = common(ruleContext, protosObjcProvider);
 
@@ -128,6 +122,7 @@ abstract class BinaryLinkingTargetFactory implements RuleConfiguredTargetFactory
 
     Optional<XcTestAppProvider> xcTestAppProvider;
     Optional<RunfilesSupport> maybeRunfilesSupport = Optional.absent();
+    ObjcConfiguration objcConfiguration = ObjcRuleClasses.objcConfiguration(ruleContext);
     switch (hasReleaseBundlingSupport) {
       case YES:
         AppleConfiguration appleConfiguration = ruleContext.getFragment(AppleConfiguration.class);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/IosTest.java b/src/main/java/com/google/devtools/build/lib/rules/objc/IosTest.java
index c15c84300a..bdb5fa3f47 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/IosTest.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/IosTest.java
@@ -84,17 +84,10 @@ public final class IosTest implements RuleConfiguredTargetFactory {
   @Override
   public final ConfiguredTarget create(RuleContext ruleContext)
       throws InterruptedException, RuleErrorException {
-    ObjcConfiguration objcConfiguration = ObjcRuleClasses.objcConfiguration(ruleContext);
-
-    Optional<ObjcProvider> protosObjcProvider = Optional.absent();
-    Optional<XcodeProvider> protosXcodeProvider = Optional.absent();
-
-    if (objcConfiguration.experimentalAutoTopLevelUnionObjCProtos()) {
-      ProtobufSupport protoSupport =
-          new ProtobufSupport(ruleContext).registerGenerationActions().registerCompilationActions();
-      protosObjcProvider = Optional.of(protoSupport.getObjcProvider());
-      protosXcodeProvider = Optional.of(protoSupport.getXcodeProvider());
-    }
+    ProtobufSupport protoSupport =
+        new ProtobufSupport(ruleContext).registerGenerationActions().registerCompilationActions();
+    Optional<ObjcProvider> protosObjcProvider = protoSupport.getObjcProvider();
+    Optional<XcodeProvider> protosXcodeProvider = protoSupport.getXcodeProvider();
 
     ObjcCommon common = common(ruleContext, protosObjcProvider);
 
@@ -168,6 +161,7 @@ public final class IosTest implements RuleConfiguredTargetFactory {
         .addXcodeSettings(xcodeProviderBuilder, common)
         .validateAttributes();
 
+    ObjcConfiguration objcConfiguration = ObjcRuleClasses.objcConfiguration(ruleContext);
     new ReleaseBundlingSupport(
             ruleContext,
             common.getObjcProvider(),
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 36821e9baa..ac2abad6a0 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
@@ -184,11 +184,9 @@ public class ObjcCommandLineOptions extends FragmentOptions {
   // TODO(b/28451644): Make this option the default behavior.
   @Option(
     name = "experimental_auto_top_level_union_objc_protos",
-    defaultValue = "false",
+    defaultValue = "true",
     category = "flags",
-    help =
-        "Specifies whether to use the experimental proto generation scheme, in which they are all "
-            + "generated and linked into the final linking target."
+    help = "This flag is a noop and scheduled for removal."
   )
   public boolean experimentalAutoTopLevelUnionObjCProtos;
 
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 fc7d5cfc0d..33c1fb5541 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
@@ -69,7 +69,6 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment {
   private final boolean useAbsolutePathsForActions;
   private final boolean prioritizeStaticLibs;
   private final boolean debugWithGlibcxx;
-  private final boolean experimentalAutoTopLevelUnionObjCProtos;
   @Nullable private final Label extraEntitlements;
   private final boolean deviceDebugEntitlements;
 
@@ -95,8 +94,6 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment {
     this.prioritizeStaticLibs = objcOptions.prioritizeStaticLibs;
     this.debugWithGlibcxx = objcOptions.debugWithGlibcxx;
     this.extraEntitlements = objcOptions.extraEntitlements;
-    this.experimentalAutoTopLevelUnionObjCProtos =
-        objcOptions.experimentalAutoTopLevelUnionObjCProtos;
     this.deviceDebugEntitlements = objcOptions.deviceDebugEntitlements;
   }
 
@@ -271,14 +268,6 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment {
   }
 
   /**
-   * Whether the experimental feature of only generating proto sources at the linking target is
-   * enabled or not.
-   */
-  public boolean experimentalAutoTopLevelUnionObjCProtos() {
-    return experimentalAutoTopLevelUnionObjCProtos;
-  }
-
-  /**
    * Returns whether device debug entitlements should be included when signing an application.
    *
    * <p>Note that debug entitlements should not be included in compilation mode {@code opt}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoAspect.java
index b2d733e42f..a33387479d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoAspect.java
@@ -37,7 +37,6 @@ public class ObjcProtoAspect extends NativeAspectClass implements ConfiguredAspe
   public AspectDefinition getDefinition(AspectParameters aspectParameters) {
     return new AspectDefinition.Builder(NAME)
         .attributeAspect("deps", this)
-        .requiresConfigurationFragments(ObjcConfiguration.class)
         .build();
   }
 
@@ -46,12 +45,6 @@ public class ObjcProtoAspect extends NativeAspectClass implements ConfiguredAspe
       ConfiguredTarget base, RuleContext ruleContext, AspectParameters parameters)
       throws InterruptedException {
     ConfiguredAspect.Builder aspectBuilder = new ConfiguredAspect.Builder(NAME, ruleContext);
-    ObjcConfiguration objcConfiguration = ruleContext.getFragment(ObjcConfiguration.class);
-
-    if (!objcConfiguration.experimentalAutoTopLevelUnionObjCProtos()) {
-      // Only process the aspect if the experimental flag is set.
-      return aspectBuilder.build();
-    }
 
     ObjcProtoProvider.Builder aspectObjcProtoProvider = new ObjcProtoProvider.Builder();
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java
index 7e22ff5812..54d004403d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.rules.objc;
 
 
+import com.google.common.base.Optional;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.RuleContext;
@@ -43,23 +44,19 @@ public class ObjcProtoLibrary implements RuleConfiguredTargetFactory {
   private ConfiguredTarget createProtobufTarget(RuleContext ruleContext)
       throws InterruptedException, RuleErrorException {
     NestedSetBuilder<Artifact> filesToBuild = NestedSetBuilder.stableOrder();
-    boolean experimentalAutoUnion =
-        ObjcRuleClasses.objcConfiguration(ruleContext).experimentalAutoTopLevelUnionObjCProtos();
 
     ProtobufSupport protoSupport =
         new ProtobufSupport(ruleContext).registerGenerationActions().addFilesToBuild(filesToBuild);
 
-    if (!experimentalAutoUnion) {
-      protoSupport.registerCompilationActions();
-    }
-
-    XcodeProvider xcodeProvider = protoSupport.getXcodeProvider();
+    Optional<XcodeProvider> xcodeProvider = protoSupport.getXcodeProvider();
 
-    new XcodeSupport(ruleContext).registerActions(xcodeProvider).addFilesToBuild(filesToBuild);
+    new XcodeSupport(ruleContext)
+        .registerActions(xcodeProvider.get())
+        .addFilesToBuild(filesToBuild);
 
     return ObjcRuleClasses.ruleConfiguredTarget(ruleContext, filesToBuild.build())
-        .addProvider(ObjcProvider.class, protoSupport.getObjcProvider())
-        .addProvider(XcodeProvider.class, xcodeProvider)
+        .addProvider(ObjcProvider.class, protoSupport.getObjcProvider().get())
+        .addProvider(XcodeProvider.class, xcodeProvider.get())
         .build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ProtobufSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ProtobufSupport.java
index 32118ac4fc..40c98124fd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ProtobufSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ProtobufSupport.java
@@ -29,6 +29,7 @@ import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -71,6 +72,7 @@ final class ProtobufSupport {
   private static final String UNIQUE_DIRECTORY_NAME = "_generated_protos";
 
   private final RuleContext ruleContext;
+  private final BuildConfiguration buildConfiguration;
   private final ProtoAttributes attributes;
 
   // Each entry of this map represents a generation action and a compilation action. The input set
@@ -98,7 +100,23 @@ final class ProtobufSupport {
    * @param ruleContext context this proto library is constructed in
    */
   public ProtobufSupport(RuleContext ruleContext) throws RuleErrorException {
+    this(ruleContext, null);
+  }
+
+  /**
+   * Creates a new proto support for the protobuf library. This support code bundles up all the
+   * transitive protos within the groups in which they were defined. We use that information to
+   * minimize the number of inputs per generation/compilation actions by only providing what is
+   * really needed to the actions.
+   *
+   * @param ruleContext context this proto library is constructed in
+   * @param buildConfiguration the configuration from which to get prerequisites when building proto
+   *     targets in a split configuration
+   */
+  public ProtobufSupport(RuleContext ruleContext, BuildConfiguration buildConfiguration)
+      throws RuleErrorException {
     this.ruleContext = ruleContext;
+    this.buildConfiguration = buildConfiguration;
     this.attributes = new ProtoAttributes(ruleContext);
     this.inputsToOutputsMap = getInputsToOutputsMap();
   }
@@ -163,26 +181,31 @@ final class ProtobufSupport {
       filesToBuild.addAll(generatedSources).addAll(generatedHeaders);
     }
 
-    if (!ObjcRuleClasses.objcConfiguration(ruleContext).experimentalAutoTopLevelUnionObjCProtos()) {
-      int actionId = 0;
-      for (ImmutableSet<Artifact> inputProtos : inputsToOutputsMap.keySet()) {
-        ImmutableSet<Artifact> outputProtos = inputsToOutputsMap.get(inputProtos);
-        IntermediateArtifacts intermediateArtifacts = getUniqueIntermediateArtifacts(actionId);
+    int actionId = 0;
+    for (ImmutableSet<Artifact> inputProtos : inputsToOutputsMap.keySet()) {
+      ImmutableSet<Artifact> outputProtos = inputsToOutputsMap.get(inputProtos);
+      IntermediateArtifacts intermediateArtifacts = getUniqueIntermediateArtifacts(actionId);
 
-        CompilationArtifacts compilationArtifacts =
-            getCompilationArtifacts(intermediateArtifacts, inputProtos, outputProtos);
+      CompilationArtifacts compilationArtifacts =
+          getCompilationArtifacts(intermediateArtifacts, inputProtos, outputProtos);
 
-        ObjcCommon common = getCommon(intermediateArtifacts, compilationArtifacts);
-        filesToBuild.addAll(common.getCompiledArchive().asSet());
-        actionId++;
-      }
+      ObjcCommon common = getCommon(intermediateArtifacts, compilationArtifacts);
+      filesToBuild.addAll(common.getCompiledArchive().asSet());
+      actionId++;
     }
 
     return this;
   }
 
-  /** Returns the ObjcProvider for this target. */
-  public ObjcProvider getObjcProvider() {
+  /**
+   * Returns the ObjcProvider for this target, or Optional.absent() if there were no protos to
+   * generate.
+   */
+  public Optional<ObjcProvider> getObjcProvider() {
+    if (inputsToOutputsMap.isEmpty()) {
+      return Optional.absent();
+    }
+
     Iterable<PathFragment> userHeaderSearchPaths =
         ImmutableList.of(getWorkspaceRelativeOutputDir());
     ObjcCommon.Builder commonBuilder = new ObjcCommon.Builder(ruleContext);
@@ -200,22 +223,24 @@ final class ProtobufSupport {
       actionId++;
     }
 
-    if (ObjcRuleClasses.objcConfiguration(ruleContext).experimentalAutoTopLevelUnionObjCProtos()) {
-      commonBuilder.addDirectDependencyHeaderSearchPaths(userHeaderSearchPaths);
-    } else {
-      commonBuilder.addUserHeaderSearchPaths(userHeaderSearchPaths);
-    }
-
-    return commonBuilder.build().getObjcProvider();
+    commonBuilder.addDirectDependencyHeaderSearchPaths(userHeaderSearchPaths);
+    return Optional.of(commonBuilder.build().getObjcProvider());
   }
 
-  /** Returns the XcodeProvider for this target. */
-  public XcodeProvider getXcodeProvider() throws RuleErrorException {
+  /**
+   * Returns the XcodeProvider for this target or Optional.absent() if there were no protos to
+   * generate.
+   */
+  public Optional<XcodeProvider> getXcodeProvider() throws RuleErrorException {
+    if (inputsToOutputsMap.isEmpty()) {
+      return Optional.absent();
+    }
+
     XcodeProvider.Builder xcodeProviderBuilder = new XcodeProvider.Builder();
     IntermediateArtifacts intermediateArtifacts =
         ObjcRuleClasses.intermediateArtifacts(ruleContext);
     new XcodeSupport(ruleContext, intermediateArtifacts, getXcodeLabel(getBundledProtosSuffix()))
-        .addXcodeSettings(xcodeProviderBuilder, getObjcProvider(), LIBRARY_STATIC);
+        .addXcodeSettings(xcodeProviderBuilder, getObjcProvider().get(), LIBRARY_STATIC);
 
     int actionId = 0;
     for (ImmutableSet<Artifact> inputProtos : inputsToOutputsMap.keySet()) {
@@ -234,12 +259,11 @@ final class ProtobufSupport {
       actionId++;
     }
 
-    return xcodeProviderBuilder.build();
+    return Optional.of(xcodeProviderBuilder.build());
   }
 
   private NestedSet<Artifact> getPortableProtoFilters() {
-    Iterable<ObjcProtoProvider> objcProtoProviders =
-        ruleContext.getPrerequisites("deps", Mode.TARGET, ObjcProtoProvider.class);
+    Iterable<ObjcProtoProvider> objcProtoProviders = getObjcProtoProviders();
 
     NestedSetBuilder<Artifact> portableProtoFilters = NestedSetBuilder.stableOrder();
     for (ObjcProtoProvider objcProtoProvider : objcProtoProviders) {
@@ -250,8 +274,7 @@ final class ProtobufSupport {
   }
 
   private NestedSet<Artifact> getProtobufHeaders() {
-    Iterable<ObjcProtoProvider> objcProtoProviders =
-        ruleContext.getPrerequisites("deps", Mode.TARGET, ObjcProtoProvider.class);
+    Iterable<ObjcProtoProvider> objcProtoProviders = getObjcProtoProviders();
 
     NestedSetBuilder<Artifact> protobufHeaders = NestedSetBuilder.stableOrder();
     for (ObjcProtoProvider objcProtoProvider : objcProtoProviders) {
@@ -261,8 +284,7 @@ final class ProtobufSupport {
   }
 
   private NestedSet<PathFragment> getProtobufHeaderSearchPaths() {
-    Iterable<ObjcProtoProvider> objcProtoProviders =
-        ruleContext.getPrerequisites("deps", Mode.TARGET, ObjcProtoProvider.class);
+    Iterable<ObjcProtoProvider> objcProtoProviders = getObjcProtoProviders();
 
     NestedSetBuilder<PathFragment> protobufHeaderSearchPaths = NestedSetBuilder.stableOrder();
     for (ObjcProtoProvider objcProtoProvider : objcProtoProviders) {
@@ -273,10 +295,8 @@ final class ProtobufSupport {
 
   private ImmutableSetMultimap<ImmutableSet<Artifact>, Artifact> getInputsToOutputsMap()
       throws RuleErrorException {
-    Iterable<ObjcProtoProvider> objcProtoProviders =
-        ruleContext.getPrerequisites("deps", Mode.TARGET, ObjcProtoProvider.class);
-    Iterable<ProtoSourcesProvider> protoProviders =
-        ruleContext.getPrerequisites("deps", Mode.TARGET, ProtoSourcesProvider.class);
+    Iterable<ObjcProtoProvider> objcProtoProviders = getObjcProtoProviders();
+    Iterable<ProtoSourcesProvider> protoProviders = getProtoSourcesProviders();
 
     ImmutableList.Builder<NestedSet<Artifact>> protoSets =
         new ImmutableList.Builder<NestedSet<Artifact>>();
@@ -415,10 +435,7 @@ final class ProtobufSupport {
             .addAdditionalHdrs(getGeneratedProtoOutputs(filteredInputProtos, ".pbobjc.h"))
             .addAdditionalHdrs(getProtobufHeaders());
 
-    boolean experimentalAutoUnion =
-        ObjcRuleClasses.objcConfiguration(ruleContext).experimentalAutoTopLevelUnionObjCProtos();
-
-    if (!experimentalAutoUnion || isLinkingTarget()) {
+    if (isLinkingTarget()) {
       compilationArtifacts.addNonArcSrcs(getGeneratedProtoOutputs(outputProtoFiles, ".pbobjc.m"));
     }
 
@@ -512,6 +529,24 @@ final class ProtobufSupport {
     return builder.build();
   }
 
+  private Iterable<ObjcProtoProvider> getObjcProtoProviders() {
+    if (buildConfiguration != null) {
+      return ruleContext
+          .getPrerequisitesByConfiguration("deps", Mode.SPLIT, ObjcProtoProvider.class)
+          .get(buildConfiguration);
+    }
+    return ruleContext.getPrerequisites("deps", Mode.TARGET, ObjcProtoProvider.class);
+  }
+
+  private Iterable<ProtoSourcesProvider> getProtoSourcesProviders() {
+    if (buildConfiguration != null) {
+      return ruleContext
+          .getPrerequisitesByConfiguration("deps", Mode.SPLIT, ProtoSourcesProvider.class)
+          .get(buildConfiguration);
+    }
+    return ruleContext.getPrerequisites("deps", Mode.TARGET, ProtoSourcesProvider.class);
+  }
+
   /**
    * Processes the case of the proto file name in the same fashion as the objective_c generator's
    * UnderscoresToCamelCase function.