Change shouldShrinkResourceCycles to only be true with local proguard…

…_specs PiperOrigin-RevId: 482038865 Change-Id: I671ac511b1669892e60554c83f89b520a0d7c07e
bazelbuild · Oct 18, 2022 · cee1a58 · cee1a58
1 parent e899d85
commit cee1a58
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 14 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
@@ -49,7 +49,6 @@
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory;
 import com.google.devtools.build.lib.analysis.RuleContext;
-import com.google.devtools.build.lib.analysis.RuleErrorConsumer;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
@@ -252,8 +251,8 @@ private static RuleConfiguredTargetBuilder init(
     }
 
     boolean shrinkResourceCycles =
-        shouldShrinkResourceCycles(
-            dataContext.getAndroidConfig(), ruleContext, dataContext.isResourceShrinkingEnabled());
+        dataContext.shouldShrinkResourceCycles(
+            ruleContext, dataContext.isResourceShrinkingEnabled());
     ProcessedAndroidData processedAndroidData =
         ProcessedAndroidData.processBinaryDataFrom(
             dataContext,
@@ -1130,15 +1129,6 @@ static ImmutableList<Artifact> getProguardSpecs(
     return proguardSpecs;
   }
 
-  /** Returns {@code true} if resource shrinking should be performed. */
-  static boolean shouldShrinkResourceCycles(
-      AndroidConfiguration androidConfig,
-      RuleErrorConsumer errorConsumer,
-      boolean shrinkResources) {
-    boolean global = androidConfig.useAndroidResourceCycleShrinking();
-    return global && shrinkResources;
-  }
-
   private static ResourceApk shrinkResources(
       RuleContext ruleContext,
       AndroidDataContext dataContext,

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java
@@ -289,6 +289,23 @@ boolean isResourceShrinkingEnabled() {
     return state == TriState.YES;
   }
 
+  /**
+   * Returns {@code true} if resource shrinking should be performed. This should be true when the
+   * resource cycle shrinking flag is enabled, resource shrinking itself is enabled, and the build
+   * is ProGuarded/optimized. The last condition is important because resource cycle shrinking
+   * generates non-final fields that are not inlined by javac. In non-optimized builds, these can
+   * noticeably increase Apk size.
+   */
+  boolean shouldShrinkResourceCycles(RuleErrorConsumer errorConsumer, boolean shrinkResources) {
+    boolean isProguarded =
+        ruleContext.attributes().has(ProguardHelper.PROGUARD_SPECS, BuildType.LABEL_LIST)
+            && !ruleContext
+                .getPrerequisiteArtifacts(ProguardHelper.PROGUARD_SPECS)
+                .list()
+                .isEmpty();
+    return isProguarded && getAndroidConfig().useAndroidResourceCycleShrinking() && shrinkResources;
+  }
+
   boolean useResourcePathShortening() {
     // Use resource path shortening iff:
     //   1) --experimental_android_resource_path_shortening

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkData.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkData.java
@@ -480,8 +480,7 @@ public AndroidBinaryDataInfo processBinaryData(
                   ctx,
                   errorReporter,
                   stampedManifest,
-                  AndroidBinary.shouldShrinkResourceCycles(
-                      ctx.getAndroidConfig(), errorReporter, settings.shrinkResources),
+                  ctx.shouldShrinkResourceCycles(errorReporter, settings.shrinkResources),
                   manifestValueMap,
                   AndroidResources.from(
                       errorReporter,

diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
@@ -4120,6 +4120,45 @@ public void testAapt2ResourceCycleShinkingWithoutResourceShrinking() throws Exce
     assertThat(packageArgs).doesNotContain("--conditionalKeepRules");
   }
 
+  @Test
+  public void testAapt2ResourceCycleShrinkingDisabledNoProguardSpecs() throws Exception {
+    useConfiguration("--experimental_android_resource_cycle_shrinking=true");
+    scratch.file(
+        "java/com/google/android/hello/BUILD",
+        "android_binary(name = 'hello',",
+        "               srcs = ['Foo.java'],",
+        "               manifest = 'AndroidManifest.xml',",
+        "               resource_files = ['res/values/strings.xml'],",
+        "               shrink_resources = 1)");
+
+    ConfiguredTargetAndData targetAndData =
+        getConfiguredTargetAndData("//java/com/google/android/hello:hello");
+
+    Artifact jar = getResourceClassJar(targetAndData);
+    assertThat(getGeneratingAction(jar).getMnemonic()).isEqualTo("RClassGenerator");
+    // Final fields should still be generated for non-ProGuarded builds.
+    assertThat(getGeneratingSpawnActionArgs(jar)).contains("--finalFields");
+  }
+
+  @Test
+  public void testAapt2ResourceCycleShrinkingDisabledNoProguardSpecsApplicationResources()
+      throws Exception {
+    useConfiguration("--experimental_android_resource_cycle_shrinking=true");
+    scratch.file(
+        "java/com/google/android/hello/BUILD",
+        "android_binary(name = 'hello',",
+        "               srcs = ['Foo.java'],",
+        "               manifest = 'AndroidManifest.xml',",
+        "               shrink_resources = 1)");
+
+    ConfiguredTargetAndData targetAndData =
+        getConfiguredTargetAndData("//java/com/google/android/hello:hello");
+
+    Artifact jar = getResourceClassJar(targetAndData);
+    assertThat(getGeneratingAction(jar).getMnemonic()).isEqualTo("RClassGenerator");
+    // Final fields should still be generated for non-ProGuarded builds.
+    assertThat(getGeneratingSpawnActionArgs(jar)).contains("--finalFields");
+  }
 
   @Test
   public void testOnlyProguardSpecs() throws Exception {