Automated [] rollback of [].

*** Reason for rollback ***

Breaks many android targets in the depot.

*** Original change description ***

Trying again with checking for the presence of the "resources" attribute.

Change the resource dependency handling to separate between the transitive and direct resources from libraries.

This slightly increases the complexity of resource propagation. The initial algorithm was to simply merge all transitive ResourceContainers together with any new ResourceContainer and propagate them via the AndroidResourcesProvider. The new algorithm is encapsulated inside a new ResourceDependencies class which...

***

ROLLBACK_OF=105631635

--
MOS_MIGRATED_REVID=105752015

1 files changed, 37 insertions, 28 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java b/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java
index 42ffbae043..3c14b8ec6e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java
@@ -26,6 +26,9 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.config.CompilationMode;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.rules.android.AndroidResourcesProvider.ResourceContainer;
 import com.google.devtools.build.lib.rules.android.AndroidResourcesProvider.ResourceType;
 import com.google.devtools.build.lib.rules.android.LocalResourceContainer.Builder.InvalidAssetPath;
@@ -45,7 +48,7 @@ public final class ApplicationManifest {
       throw new RuleConfigurationException();
     }
     return new ApplicationManifest(Iterables.getOnlyElement(
-        resources.getDirectAndroidResources())
+        resources.getTransitiveAndroidResources())
         .getManifest());
   }
 
@@ -88,7 +91,7 @@ public final class ApplicationManifest {
     AndroidResourcesProvider resourcesProvider = AndroidCommon.getAndroidResources(ruleContext);
     if (resourcesProvider != null) {
       ResourceContainer resourceContainer = Iterables.getOnlyElement(
-          resourcesProvider.getDirectAndroidResources());
+          resourcesProvider.getTransitiveAndroidResources());
       return resourceContainer.getRenameManifestPackage();
     } else {
       return null;
@@ -165,10 +168,9 @@ public final class ApplicationManifest {
   }
 
   public ApplicationManifest mergeWith(RuleContext ruleContext,
-      ResourceDependencies resourceDeps) {
-    Iterable<Artifact> mergeeManifests = getMergeeManifests(resourceDeps.getResources()); 
-    if (!Iterables.isEmpty(mergeeManifests)) {
-      Iterable<Artifact> exportedManifests = mergeeManifests;
+      Iterable<ResourceContainer> resourceContainers) {
+    if (!Iterables.isEmpty(getMergeeManifests(resourceContainers))) {
+      Iterable<Artifact> exportedManifests = getMergeeManifests(resourceContainers);
       Artifact outputManifest = ruleContext.getUniqueDirectoryArtifact(
           ruleContext.getRule().getName() + "_merged", "AndroidManifest.xml",
           ruleContext.getBinOrGenfilesDirectory());
@@ -196,7 +198,7 @@ public final class ApplicationManifest {
   public ResourceApk packWithAssets(
       Artifact resourceApk,
       RuleContext ruleContext,
-      ResourceDependencies resourceDeps,
+      NestedSet<ResourceContainer> resourceContainers,
       Artifact rTxt,
       boolean incremental,
       Artifact proguardCfg) throws InterruptedException {
@@ -212,7 +214,7 @@ public final class ApplicationManifest {
 
       return createApk(resourceApk,
           ruleContext,
-          resourceDeps,
+          resourceContainers,
           rTxt,
           null, /* configurationFilters */
           ImmutableList.<String>of(), /* uncompressedExtensions */
@@ -235,7 +237,7 @@ public final class ApplicationManifest {
   public ResourceApk packWithDataAndResources(
       Artifact resourceApk,
       RuleContext ruleContext,
-      ResourceDependencies resourceDeps,
+      NestedSet<ResourceContainer> resourceContainers,
       Artifact rTxt,
       Artifact symbolsTxt,
       List<String> configurationFilters,
@@ -262,7 +264,7 @@ public final class ApplicationManifest {
 
       return createApk(resourceApk,
           ruleContext,
-          resourceDeps,
+          resourceContainers,
           rTxt,
           symbolsTxt,
           configurationFilters,
@@ -285,7 +287,7 @@ public final class ApplicationManifest {
 
   private ResourceApk createApk(Artifact resourceApk,
       RuleContext ruleContext,
-      ResourceDependencies resourceDeps,
+      NestedSet<ResourceContainer> resourceContainers,
       Artifact rTxt,
       Artifact symbolsTxt,
       List<String> configurationFilters,
@@ -303,10 +305,7 @@ public final class ApplicationManifest {
             .withROutput(rTxt)
             .withSymbolsFile(symbolsTxt)
             .buildFromRule(ruleContext, resourceApk),
-        resourceDeps.getResources(),  // TODO(bazel-team): Figure out if we really need to check
-        // the ENTIRE transitive closure, or just the direct dependencies. Given that each rule with
-        // resources would check for inline resources, we can rely on the previous rule to have
-        // checked its dependencies.
+        resourceContainers,
         ruleContext);
 
     AndroidResourcesProcessorBuilder builder =
@@ -317,7 +316,7 @@ public final class ApplicationManifest {
             .setJavaPackage(resourceContainer.getJavaPackage())
             .setDebug(ruleContext.getConfiguration().getCompilationMode() != CompilationMode.OPT)
             .withPrimary(resourceContainer)
-            .withDependencies(resourceDeps)
+            .withDependencies(resourceContainers)
             .setDensities(densities)
             .setProguardOut(proguardCfg)
             .setApplicationId(applicationId)
@@ -332,9 +331,16 @@ public final class ApplicationManifest {
     }
 
     ResourceContainer processed = builder.build(ruleContext);
+    NestedSet<ResourceContainer> transitiveResources =
+        NestedSetBuilder.<ResourceContainer>naiveLinkOrder()
+            // TODO(bazel-team): If this is replaced with .addTransitive(), a few tests fail.
+            // Investigate.
+            .addAll(resourceContainers)
+            .add(processed)
+            .build();
 
     return new ResourceApk(
-        resourceApk, processed.getJavaSourceJar(), resourceDeps, processed, manifest,
+        resourceApk, processed.getJavaSourceJar(), transitiveResources, processed, manifest,
         proguardCfg, false);
   }
 
@@ -357,7 +363,15 @@ public final class ApplicationManifest {
   /** Uses the resource apk from the resources attribute, as opposed to recompiling. */
   public ResourceApk useCurrentResources(RuleContext ruleContext, Artifact proguardCfg) {
     ResourceContainer resourceContainer = Iterables.getOnlyElement(
-        AndroidCommon.getAndroidResources(ruleContext).getDirectAndroidResources());
+        AndroidCommon.getAndroidResources(ruleContext).getTransitiveAndroidResources());
+    NestedSet<ResourceContainer> resourceContainers =
+        NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER);
+
+    NestedSet<ResourceContainer> transitiveResources =
+        NestedSetBuilder.<ResourceContainer>naiveLinkOrder()
+            .addAll(resourceContainers)
+            .add(resourceContainer)
+            .build();
 
     new AndroidAaptActionHelper(
         ruleContext,
@@ -367,7 +381,7 @@ public final class ApplicationManifest {
     return new ResourceApk(
         resourceContainer.getApk(),
         null /* javaSrcJar */,
-        ResourceDependencies.empty(),
+        transitiveResources,
         resourceContainer,
         manifest,
         proguardCfg,
@@ -384,7 +398,7 @@ public final class ApplicationManifest {
   public ResourceApk packWithResources(
       Artifact resourceApk,
       RuleContext ruleContext,
-      ResourceDependencies resourceDeps,
+      NestedSet<ResourceContainer> resourceContainers,
       boolean createSource,
       Artifact proguardCfg) throws InterruptedException {
 
@@ -392,15 +406,10 @@ public final class ApplicationManifest {
         ruleContext.getPrerequisite("resources", Mode.TARGET);
     ResourceContainer resourceContainer = Iterables.getOnlyElement(
         resourcesPrerequisite.getProvider(AndroidResourcesProvider.class)
-        .getDirectAndroidResources());
-    // It's ugly, but flattening now is more performant given the rest of the checks.
-    List<ResourceContainer> resourceContainers =
-        ImmutableList.<ResourceContainer>builder()
-        //.add(resourceContainer)
-        .addAll(resourceDeps.getResources()).build();
+            .getTransitiveAndroidResources());
 
     // Dealing with Android library projects
-    if (Iterables.size(resourceDeps.getResources()) > 1) {
+    if (Iterables.size(resourceContainers) > 1) {
       if (resourceContainer.getConstantsInlined()
           && !resourceContainer.getArtifacts(ResourceType.RESOURCES).isEmpty()) {
         ruleContext.ruleError("This android_binary depends on an android_library, so the"
@@ -461,7 +470,7 @@ public final class ApplicationManifest {
     aaptActionHelper.createGenerateProguardAction(proguardCfg);
 
     return new ResourceApk(resourceApk, updatedResources.getJavaSourceJar(),
-        resourceDeps, updatedResources, manifest, proguardCfg, true);
+        resourceContainers, updatedResources, manifest, proguardCfg, true);
   }
 
   public Artifact getManifest() {