Always pass resource filters to aapt

It turns out that the code to filter in analysis, based on android_ide_common,
isn't as strict as aapt is.

In particular, given a language filter (like 'en'), aapt rejects language and
region resources (like 'en-rUS') but android_ide_common does not.

We could try to just patch this behavior, but it's probably indicative of
larger inconsistancies between android_ide_common and aapt. As a result, always
pass resource filters to aapt. In most cases, if we already filtered in
analysis, few if any resources will be filtered out by aapt, so we'll still
keep most of the performance gains we expected from filtering in analysis.

RELNOTES: none
PiperOrigin-RevId: 169254335

3 files changed, 3 insertions, 61 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java
index 385466e230..1ab58922e9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesProcessorBuilder.java
@@ -443,7 +443,9 @@ public class AndroidResourcesProcessorBuilder {
       builder.addExecPath("--packagePath", apkOut);
       outs.add(apkOut);
     }
-    if (resourceFilter.shouldPropagateConfigs(ruleContext)) {
+    if (resourceFilter.hasConfigurationFilters()) {
+      // Always pass filters to aapt, even if we filtered in analysis, since aapt is stricter and
+      // might remove resources that we previously accepted.
       builder.add("--resourceConfigs", resourceFilter.getConfigurationFilterString());
     }
     if (resourceFilter.hasDensities()) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java
index 5470f1f4ae..01303e528c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilter.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.rules.android;
 
 import com.android.ide.common.resources.configuration.DensityQualifier;
 import com.android.ide.common.resources.configuration.FolderConfiguration;
-import com.android.ide.common.resources.configuration.LocaleQualifier;
 import com.android.ide.common.resources.configuration.VersionQualifier;
 import com.android.resources.Density;
 import com.android.resources.ResourceFolderType;
@@ -74,12 +73,6 @@ public class ResourceFilter {
    * themselves to trigger pseudolocalization, and the other filters to prevent aapt from filtering
    * matching resources out.
    */
-  private static final ImmutableSet<LocaleQualifier> PSEUDOLOCATION_LOCALES =
-      ImmutableSet.of(
-          LocaleQualifier.getQualifier("en-rXA"),
-          LocaleQualifier.getQualifier("ar-rXB"),
-          LocaleQualifier.getQualifier("en-rXC"));
-
   @VisibleForTesting
   static enum FilterBehavior {
     /**
@@ -719,36 +712,6 @@ public class ResourceFilter {
     return filterBehavior == FilterBehavior.FILTER_IN_ANALYSIS_WITH_DYNAMIC_CONFIGURATION;
   }
 
-  /**
-   * @return whether resource configuration filters should be propagated to the resource processing
-   *     action and eventually to aapt.
-   */
-  public boolean shouldPropagateConfigs(RuleErrorConsumer ruleErrorConsumer) {
-    if (!hasConfigurationFilters()) {
-      // There are no filters to propagate
-      return false;
-    }
-
-    if (!isPrefiltering()) {
-      // The filters were not applied in analysis and must be applied in execution
-      return true;
-    }
-
-    for (FolderConfiguration config : getConfigurationFilters((ruleErrorConsumer))) {
-      for (LocaleQualifier pseudoLocale : PSEUDOLOCATION_LOCALES) {
-        if (pseudoLocale.equals(config.getLocaleQualifier())) {
-          // A pseudolocale was used, and needs to be propagated to aapt. Propagate all
-          // configuration values so that aapt does not filter out those that were skipped.
-          return true;
-        }
-      }
-    }
-
-    // Filtering already happened, and there's no reason to have aapt waste its time trying to
-    // filter again. Don't propagate the filters.
-    return false;
-  }
-
   /*
    * TODO: Stop tracking these once {@link FilterBehavior#FILTER_IN_ANALYSIS} is fully replaced by
    * {@link FilterBehavior#FILTER_IN_ANALYSIS_WITH_DYNAMIC_CONFIGURATION}.
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java
index 9db5879fac..2fc4e372db 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterTest.java
@@ -697,27 +697,4 @@ public class ResourceFilterTest extends ResourceTestBase {
 
     return androidOptions;
   }
-
-  @Test
-  public void testHasPseudolocationFilters() throws Exception {
-    // Basic pseudolocation filters
-    for (String filter :
-        ImmutableList.of("en_XA", "en-rXA", "ar_XB", "ar-rXB", "en_XC", "en-rXC")) {
-      assertHasPseudolocationFilters(filter).isTrue();
-    }
-
-    // Additional qualifiers should not mask whether pseudolocation filters are used
-    assertHasPseudolocationFilters("en-rXA-port").isTrue();
-
-    // Without pseudolocation, even though the locale is similar
-    for (String filter : ImmutableList.of("", "en", "ar", "en-rUS")) {
-      assertHasPseudolocationFilters(filter).isFalse();
-    }
-  }
-
-  private BooleanSubject assertHasPseudolocationFilters(String filters) throws Exception {
-    return assertThat(
-        makeResourceFilter(filters, "", FilterBehavior.FILTER_IN_ANALYSIS)
-            .shouldPropagateConfigs(errorConsumer));
-  }
 }