Make AndroidRuleClasses#hasProguardSpecs less fragile.

Keeps the semantics the same.

RELNOTES: None
PiperOrigin-RevId: 173899927

2 files changed, 62 insertions, 11 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
index d3231d101e..c832717eb5 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
@@ -321,9 +321,10 @@ public final class AndroidRuleClasses {
   public static boolean hasProguardSpecs(AttributeMap rule) {
     // The below is a hack to support configurable attributes (proguard_specs seems like
     // too valuable an attribute to make nonconfigurable, and we don't currently
-    // have the ability to know the configuration when determining implicit outputs).
-    // An IllegalArgumentException gets triggered if the attribute instance is configurable.
-    // We assume, heuristically, that means every configurable value is a non-empty list.
+    // have the ability to know the configuration when determining implicit outputs). So if the
+    // attribute is configurable, we create the proguard implicit output. At analysis time, we know
+    // the actual value of proguard_specs, and if it is empty we do not use the proguarded jar for
+    // dexing. If the user explicitly tries to build the proguard jar, it will fail.
     //
     // TODO(bazel-team): find a stronger approach for this. One simple approach is to somehow
     // receive 'rule' as an AggregatingAttributeMapper instead of a RawAttributeMapper,
@@ -332,12 +333,8 @@ public final class AndroidRuleClasses {
     // to somehow determine implicit outputs after the configuration is known. A third
     // approach is to refactor the Android rule logic to avoid these dependencies in the
     // first place.
-    try {
-      return !rule.get("proguard_specs", LABEL_LIST).isEmpty();
-    } catch (IllegalArgumentException e) {
-      // We assume at this point the attribute instance is configurable.
-      return true;
-    }
+    return rule.isConfigurable("proguard_specs")
+        || !rule.get("proguard_specs", LABEL_LIST).isEmpty();
   }
 
   public static final SafeImplicitOutputsFunction ANDROID_BINARY_IMPLICIT_OUTPUTS =
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
index 8793c07a57..a9d892c79f 100644
--- 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
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.rules.android;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
@@ -40,6 +41,8 @@ import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.packages.BuildType;
+import com.google.devtools.build.lib.packages.FileTarget;
+import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.util.BazelMockAndroidSupport;
 import com.google.devtools.build.lib.rules.android.AndroidRuleClasses.MultidexMode;
 import com.google.devtools.build.lib.rules.android.deployinfo.AndroidDeployInfoOuterClass.AndroidDeployInfo;
@@ -2330,17 +2333,24 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
   // regression test for b/14288948
   @Test
   public void testEmptyListAsProguardSpec() throws Exception {
-    scratchConfiguredTarget("java/foo", "abin",
+    scratch.file(
+        "java/foo/BUILD",
         "android_binary(",
         "    name = 'abin',",
         "    srcs = ['a.java'],",
         "    proguard_specs = [],",
         "    manifest = 'AndroidManifest.xml')");
+    Rule rule = getTarget("//java/foo:abin").getAssociatedRule();
+    assertNoEvents();
+    ImmutableList<String> implicitOutputFilenames =
+        rule.getOutputFiles().stream().map(FileTarget::getName).collect(toImmutableList());
+    assertThat(implicitOutputFilenames).doesNotContain("abin_proguard.jar");
   }
 
   @Test
   public void testConfigurableProguardSpecsEmptyList() throws Exception {
-    scratchConfiguredTarget("java/foo", "abin",
+    scratch.file(
+        "java/foo/BUILD",
         "android_binary(",
         "    name = 'abin',",
         "    srcs = ['a.java'],",
@@ -2348,7 +2358,11 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
         "        '" + BuildType.Selector.DEFAULT_CONDITION_KEY + "': [],",
         "    }),",
         "    manifest = 'AndroidManifest.xml')");
+    Rule rule = getTarget("//java/foo:abin").getAssociatedRule();
     assertNoEvents();
+    ImmutableList<String> implicitOutputFilenames =
+        rule.getOutputFiles().stream().map(FileTarget::getName).collect(toImmutableList());
+    assertThat(implicitOutputFilenames).contains("abin_proguard.jar");
   }
 
   @Test
@@ -3810,4 +3824,44 @@ public class AndroidBinaryTest extends AndroidBuildViewTestCase {
     AndroidInstrumentationInfo provider = b1.get(AndroidInstrumentationInfo.PROVIDER);
     assertThat(provider).isNull();
   }
+
+  /**
+   * 'proguard_specs' attribute gets read by an implicit outputs function: the
+   * current heuristic is that if this attribute is configurable, we assume its
+   * contents are non-empty and thus create the mybinary_proguard.jar output.
+   * Test that here.
+   */
+  @Test
+  public void testConfigurableProguardSpecs() throws Exception {
+    scratch.file("conditions/BUILD",
+        "config_setting(",
+        "    name = 'a',",
+        "    values = {'test_arg': 'a'})",
+        "config_setting(",
+        "    name = 'b',",
+        "    values = {'test_arg': 'b'})");
+    scratchConfiguredTarget("java/foo", "abin",
+        "android_binary(",
+        "    name = 'abin',",
+        "    srcs = ['a.java'],",
+        "    proguard_specs = select({",
+        "        '//conditions:a': [':file1.pro'],",
+        "        '//conditions:b': [],",
+        "        '//conditions:default': [':file3.pro'],",
+        "    }) + [",
+        // Add a long list here as a regression test for b/68238721
+        "        'file4.pro',",
+        "        'file5.pro',",
+        "        'file6.pro',",
+        "        'file7.pro',",
+        "        'file8.pro',",
+        "    ],",
+        "    manifest = 'AndroidManifest.xml')");
+    checkProguardUse(
+        "//java/foo:abin",
+        "abin_proguard.jar",
+        /*expectMapping=*/ false,
+        /*passes=*/ null,
+        getAndroidJarPath());
+  }
 }