Stop transitively propagating headers transitively for ObjC protos.

PiperOrigin-RevId: 164590595

8 files changed, 62 insertions, 21 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibrary.java
index ea880d8f6d..fcea10bc03 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibrary.java
@@ -117,6 +117,7 @@ public class AppleStaticLibrary implements RuleConfiguredTargetFactory {
                   protosToAvoid,
                   ImmutableList.<ProtoSourcesProvider>of(),
                   objcProtoProviders,
+                  ImmutableList.of(),
                   ProtobufSupport.getTransitivePortableProtoFilters(objcProtoProviders))
               .registerGenerationActions()
               .registerCompilationActions();
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 925703d840..8a91ca9293 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
@@ -93,6 +93,7 @@ abstract class BinaryLinkingTargetFactory implements RuleConfiguredTargetFactory
                 ruleContext.getConfiguration(),
                 ImmutableList.<ProtoSourcesProvider>of(),
                 objcProtoProviders,
+                ImmutableList.of(),
                 ProtobufSupport.getTransitivePortableProtoFilters(objcProtoProviders))
             .registerGenerationActions()
             .registerCompilationActions();
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 61c59b32e1..779748e981 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
@@ -98,6 +98,7 @@ public final class IosTest implements RuleConfiguredTargetFactory {
                 ruleContext.getConfiguration(),
                 ImmutableList.<ProtoSourcesProvider>of(),
                 objcProtoProviders,
+                ImmutableList.of(),
                 ProtobufSupport.getTransitivePortableProtoFilters(objcProtoProviders))
             .registerGenerationActions()
             .registerCompilationActions();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java
index 6cf378355a..da5e230a2e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchBinarySupport.java
@@ -243,6 +243,7 @@ public class MultiArchBinarySupport {
                     protosToAvoid,
                     ImmutableList.<ProtoSourcesProvider>of(),
                     objcProtoProviders,
+                    ImmutableList.of(),
                     ProtobufSupport.getTransitivePortableProtoFilters(objcProtoProviders))
                 .registerGenerationActions()
                 .registerCompilationActions();
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 cd4553c5e1..ac22e9f64c 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
@@ -39,7 +39,6 @@ import static com.google.devtools.build.lib.rules.objc.ObjcProvider.LIBRARY;
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.LINKED_BINARY;
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.LINKMAP_FILE;
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.LINKOPT;
-import static com.google.devtools.build.lib.rules.objc.ObjcProvider.MODULE_MAP;
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.SDK_DYLIB;
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.SDK_FRAMEWORK;
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.SOURCE;
@@ -47,8 +46,6 @@ import static com.google.devtools.build.lib.rules.objc.ObjcProvider.STATIC_FRAME
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.STATIC_FRAMEWORK_FILE;
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.STORYBOARD;
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.STRINGS;
-import static com.google.devtools.build.lib.rules.objc.ObjcProvider.TOP_LEVEL_MODULE_MAP;
-import static com.google.devtools.build.lib.rules.objc.ObjcProvider.UMBRELLA_HEADER;
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.WEAK_SDK_FRAMEWORK;
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.XCASSETS_DIR;
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.XCDATAMODEL;
@@ -73,7 +70,6 @@ import com.google.devtools.build.lib.rules.apple.AppleToolchain;
 import com.google.devtools.build.lib.rules.cpp.CcLinkParams;
 import com.google.devtools.build.lib.rules.cpp.CcLinkParamsProvider;
 import com.google.devtools.build.lib.rules.cpp.CppCompilationContext;
-import com.google.devtools.build.lib.rules.cpp.CppModuleMap;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -559,13 +555,7 @@ public final class ObjcCommon {
       }
 
       if (hasModuleMap) {
-        CppModuleMap moduleMap = intermediateArtifacts.moduleMap();
-        Optional<Artifact> umbrellaHeader = moduleMap.getUmbrellaHeader();
-        if (umbrellaHeader.isPresent()) {
-          objcProvider.add(UMBRELLA_HEADER, umbrellaHeader.get());
-        }
-        objcProvider.add(MODULE_MAP, moduleMap.getArtifact());
-        objcProvider.add(TOP_LEVEL_MODULE_MAP, moduleMap);
+        objcProvider.addModuleMap(intermediateArtifacts.moduleMap());
       }
 
       objcProvider
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 92f96ec95d..8839892deb 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
@@ -55,12 +55,17 @@ public class ObjcProtoLibrary implements RuleConfiguredTargetFactory {
     Iterable<ObjcProtoProvider> objcProtoProviders =
         ruleContext.getPrerequisites("deps", Mode.TARGET, ObjcProtoProvider.class);
 
+    Iterable<ObjcProvider> additionalDependencies =
+        ruleContext.getPrerequisites(
+            ObjcRuleClasses.PROTO_LIB_ATTR, Mode.TARGET, ObjcProvider.SKYLARK_CONSTRUCTOR);
+
     ProtobufSupport protoSupport =
         new ProtobufSupport(
                 ruleContext,
                 ruleContext.getConfiguration(),
                 protoProviders,
                 objcProtoProviders,
+                additionalDependencies,
                 getPortableProtoFilters(ruleContext, objcProtoProviders, protoProviders))
             .registerGenerationActions()
             .addFilesToBuild(filesToBuild);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java
index 51a3c435fa..4039722fbd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java
@@ -18,6 +18,7 @@ import static com.google.devtools.build.lib.collect.nestedset.Order.LINK_ORDER;
 import static com.google.devtools.build.lib.collect.nestedset.Order.STABLE_ORDER;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Optional;
 import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
@@ -750,6 +751,19 @@ public final class ObjcProvider extends Info {
     }
 
     /**
+     * Convenience method to add a modulemap to this ObjcProvider.
+     */
+    public Builder addModuleMap(CppModuleMap moduleMap) {
+      Optional<Artifact> umbrellaHeader = moduleMap.getUmbrellaHeader();
+      if (umbrellaHeader.isPresent()) {
+        add(UMBRELLA_HEADER, umbrellaHeader.get());
+      }
+      add(MODULE_MAP, moduleMap.getArtifact());
+      add(TOP_LEVEL_MODULE_MAP, moduleMap);
+      return this;
+    }
+
+    /**
      * Add all elements from providers, and propagate them to any (transitive) dependers on this
      * ObjcProvider.
      */
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 9784155a41..fb1a75fdfb 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
@@ -71,6 +71,7 @@ final class ProtobufSupport {
   private final IntermediateArtifacts intermediateArtifacts;
   private final Set<PathFragment> dylibHandledProtoPaths;
   private final Iterable<ObjcProtoProvider> objcProtoProviders;
+  private final Iterable<ObjcProvider> dependencyObjcProviders;
   private final NestedSet<Artifact> portableProtoFilters;
 
   // Each entry of this map represents a generation action and a compilation action. The input set
@@ -100,12 +101,15 @@ final class ProtobufSupport {
    *     targets in a split configuration
    * @param protoProviders the list of ProtoSourcesProviders that this proto support should process
    * @param objcProtoProviders the list of ObjcProtoProviders that this proto support should process
+   * @param dependencyObjcProviders the list of extra objc dependencies to add to this support's
+   *     ObjcProvider
    */
   public ProtobufSupport(
       RuleContext ruleContext,
       BuildConfiguration buildConfiguration,
       Iterable<ProtoSourcesProvider> protoProviders,
       Iterable<ObjcProtoProvider> objcProtoProviders,
+      Iterable<ObjcProvider> dependencyObjcProviders,
       NestedSet<Artifact> portableProtoFilters) {
     this(
         ruleContext,
@@ -113,6 +117,7 @@ final class ProtobufSupport {
         NestedSetBuilder.<Artifact>stableOrder().build(),
         protoProviders,
         objcProtoProviders,
+        dependencyObjcProviders,
         portableProtoFilters);
   }
 
@@ -130,6 +135,8 @@ final class ProtobufSupport {
    *     symbols
    * @param protoProviders the list of ProtoSourcesProviders that this proto support should process
    * @param objcProtoProviders the list of ObjcProtoProviders that this proto support should process
+   * @param dependencyObjcProviders the list of extra objc dependencies to add to this support's
+   *     ObjcProvider
    */
   public ProtobufSupport(
       RuleContext ruleContext,
@@ -137,12 +144,14 @@ final class ProtobufSupport {
       NestedSet<Artifact> dylibHandledProtos,
       Iterable<ProtoSourcesProvider> protoProviders,
       Iterable<ObjcProtoProvider> objcProtoProviders,
+      Iterable<ObjcProvider> dependencyObjcProviders,
       NestedSet<Artifact> portableProtoFilters) {
     this.ruleContext = ruleContext;
     this.buildConfiguration = buildConfiguration;
     this.attributes = new ProtoAttributes(ruleContext);
     this.dylibHandledProtoPaths = runfilesPaths(dylibHandledProtos.toSet());
     this.objcProtoProviders = objcProtoProviders;
+    this.dependencyObjcProviders = dependencyObjcProviders;
     this.portableProtoFilters = portableProtoFilters;
     this.intermediateArtifacts =
         ObjcRuleClasses.intermediateArtifacts(ruleContext, buildConfiguration);
@@ -257,12 +266,7 @@ final class ProtobufSupport {
       return Optional.absent();
     }
 
-    Iterable<PathFragment> includes = ImmutableList.of(getWorkspaceRelativeOutputDir());
-    ObjcCommon.Builder commonBuilder = new ObjcCommon.Builder(ruleContext);
-
-    if (!isLinkingTarget()) {
-      commonBuilder.setIntermediateArtifacts(intermediateArtifacts).setHasModuleMap();
-    }
+    ObjcProvider.Builder objcProviderBuilder = new ObjcProvider.Builder();
 
     int actionId = 0;
     for (ImmutableSet<Artifact> inputProtos : inputsToOutputsMap.keySet()) {
@@ -273,17 +277,41 @@ final class ProtobufSupport {
           getCompilationArtifacts(intermediateArtifacts, inputProtos, outputProtos);
 
       ObjcCommon common = getCommon(intermediateArtifacts, compilationArtifacts);
-      commonBuilder.addDepObjcProviders(ImmutableSet.of(common.getObjcProvider()));
+      if (isLinkingTarget()) {
+        // If ProtobufSupport is being called from a linking target (i.e. apple_binary), we
+        // propagate the generated headers (contained inside common.getObjcProvider()) into the
+        // ObjcProvider, in case the generated headers are used from dependent ios_test targets,
+        // which uses XcTestAppProvider to access those headers.
+        // TODO(b/64152968): Remove conditional when ios_test is gone and only propagate to
+        //     direct dependents.
+        objcProviderBuilder.addTransitiveAndPropagate(common.getObjcProvider());
+      } else {
+        // If ProtobufSupport is being called from a non-linking target (i.e. objc_proto_library),
+        // we propagate the headers only to direct dependents of this target. This avoids over
+        // accumulation of header files when a top-level target depends on many objc_proto_library
+        // targets.
+        objcProviderBuilder.addAsDirectDeps(common.getObjcProvider());
+      }
       actionId++;
     }
 
+    // Same as with the headers, we only propagate the includes to direct dependents for non-linking
+    // targets (i.e. objc_proto_library), and we fully propagate the includes for linking targets
+    // in case ios_test dependents need to use the headers.
+    // TODO(b/64152968): Remove conditional when ios_test is gone and only propagate to direct
+    //     dependents.
+    Iterable<PathFragment> includes = ImmutableList.of(getWorkspaceRelativeOutputDir());
     if (isLinkingTarget()) {
-      commonBuilder.addIncludes(includes);
+      objcProviderBuilder.addAll(ObjcProvider.INCLUDE, includes);
     } else {
-      commonBuilder.addDirectDependencyIncludes(includes);
+      objcProviderBuilder.addAllForDirectDependents(ObjcProvider.INCLUDE, includes);
+      // For non linking targets, also create and propagate this target's module map.
+      objcProviderBuilder.addModuleMap(intermediateArtifacts.moduleMap());
     }
 
-    return Optional.of(commonBuilder.build().getObjcProvider());
+    objcProviderBuilder.addTransitiveAndPropagate(dependencyObjcProviders);
+
+    return Optional.of(objcProviderBuilder.build());
   }
 
   private NestedSet<Artifact> getProtobufHeaders() {