Stop warning about resource filters with "deprecated" qualifiers

Resource qualifiers of the form "en_US" are deprecated for resources (in favor
of qualifiers of the form "en-rUS") but are apparently the recommended (if not
only) way of specifying locales in resource filter strings.

Removing the warning (and reverting any changes that tried following it) should
solve this problem - the original filter strings from the rule are passed
unmodified to aapt, but these filters are still "fixed" so that
android_ide_common (which doesn't support the "deprecated" qualifier form) can
still be used to filter resources in analysis.

RELNOTES: none
PiperOrigin-RevId: 183830253

2 files changed, 29 insertions, 98 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactory.java b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactory.java
index 4a9d2bf5c7..7f2b6ae570 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactory.java
@@ -254,7 +254,7 @@ public class ResourceFilterFactory {
     ImmutableList.Builder<FolderConfiguration> filterBuilder = ImmutableList.builder();
     for (String filter : configFilters) {
       addIfNotNull(
-          getFolderConfiguration(ruleErrorConsumer, filter),
+          getFolderConfiguration(filter),
           filter,
           filterBuilder,
           ruleErrorConsumer,
@@ -264,13 +264,12 @@ public class ResourceFilterFactory {
     return filterBuilder.build();
   }
 
-  private FolderConfiguration getFolderConfiguration(
-      RuleErrorConsumer ruleErrorConsumer, String filter) {
+  private FolderConfiguration getFolderConfiguration(String filter) {
 
     // Clean up deprecated representations of resource qualifiers that FolderConfiguration can't
     // handle.
     for (DeprecatedQualifierHandler handler : deprecatedQualifierHandlers) {
-      filter = handler.fixAttributeIfNeeded(ruleErrorConsumer, filter);
+      filter = handler.fixAttributeIfNeeded(filter);
     }
 
     return FolderConfiguration.getConfigForQualifierString(filter);
@@ -281,7 +280,6 @@ public class ResourceFilterFactory {
     private final String replacement;
     private final String description;
 
-    private boolean warnedForAttribute = false;
     private boolean warnedForResources = false;
 
     private DeprecatedQualifierHandler(String pattern, String replacement, String description) {
@@ -290,26 +288,14 @@ public class ResourceFilterFactory {
       this.description = description;
     }
 
-    private String fixAttributeIfNeeded(RuleErrorConsumer ruleErrorConsumer, String qualifier) {
+    private String fixAttributeIfNeeded(String qualifier) {
       Matcher matcher = pattern.matcher(qualifier);
 
       if (!matcher.matches()) {
         return qualifier;
       }
 
-      String fixed = matcher.replaceFirst(replacement);
-      // We don't want to spam users. Only warn about this kind of issue once per target.
-      // TODO(asteinb): Will this cause problems when settings are propagated via dynamic
-      // configuration?
-      if (!warnedForAttribute) {
-        ruleErrorConsumer.attributeWarning(
-            RESOURCE_CONFIGURATION_FILTERS_NAME,
-            String.format(
-                "When referring to %s, use of qualifier '%s' is deprecated. Use '%s' instead.",
-                description, matcher.group(), fixed));
-        warnedForAttribute = true;
-      }
-      return fixed;
+      return matcher.replaceFirst(replacement);
     }
 
     private String fixResourceIfNeeded(
@@ -339,7 +325,16 @@ public class ResourceFilterFactory {
     }
   }
 
-  /** List of deprecated qualifiers that should currently by handled with a warning */
+  /**
+   * List of deprecated qualifiers that are not supported by {@link FolderConfiguration}.
+   *
+   * For resources, we should warn if these qualifiers are encountered, since aapt supports the
+   * fixed version (and aapt2 only supports that version).
+   *
+   * For resource filters, however, aapt only supports this old version. Convert the qualifiers so
+   * that they can be parsed by FolderConfiguration, but do not warn (since they are, actually, what
+   * aapt expects) and save the original qualifier strings to be passed to aapt.
+   */
   private final List<DeprecatedQualifierHandler> deprecatedQualifierHandlers =
       ImmutableList.of(
           /*
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactoryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactoryTest.java
index aabcf0435e..7564c5d376 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/ResourceFilterFactoryTest.java
@@ -137,10 +137,8 @@ public class ResourceFilterFactoryTest extends ResourceTestBase {
         ImmutableList.of("values-en/foo.xml", "values-en-rUS/foo.xml"),
         ImmutableList.of("values-fr/foo.xml", "values-en-rCA/foo.xml"));
 
-    assertThat(
-            errorConsumer.getAndClearAttributeWarnings(
-                ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME))
-        .hasSize(1);
+    errorConsumer.assertNoAttributeWarnings(
+        ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME);
   }
 
   @Test
@@ -152,10 +150,8 @@ public class ResourceFilterFactoryTest extends ResourceTestBase {
         ImmutableList.of("values-en/foo.xml", "values-en-rUS/foo.xml"),
         ImmutableList.of("values-fr/foo.xml", "values-en-rCA/foo.xml"));
 
-    assertThat(
-            errorConsumer.getAndClearAttributeWarnings(
-                ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME))
-        .hasSize(1);
+    errorConsumer.assertNoAttributeWarnings(
+        ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME);
   }
 
   @Test
@@ -167,10 +163,8 @@ public class ResourceFilterFactoryTest extends ResourceTestBase {
         ImmutableList.of("values-en/foo.xml", "values-en-rUS/foo.xml"),
         ImmutableList.of("values-fr/foo.xml", "values-en-rCA/foo.xml"));
 
-    assertThat(
-            errorConsumer.getAndClearAttributeWarnings(
-                ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME))
-        .hasSize(1);
+    errorConsumer.assertNoAttributeWarnings(
+        ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME);
   }
 
   @Test
@@ -182,10 +176,8 @@ public class ResourceFilterFactoryTest extends ResourceTestBase {
         ImmutableList.of("values-en/foo.xml", "values-en-rUS/foo.xml"),
         ImmutableList.of("values-fr/foo.xml", "values-en-rCA/foo.xml"));
 
-    assertThat(
-            errorConsumer.getAndClearAttributeWarnings(
-                ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME))
-        .hasSize(1);
+    errorConsumer.assertNoAttributeWarnings(
+        ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME);
   }
 
   @Test
@@ -202,10 +194,8 @@ public class ResourceFilterFactoryTest extends ResourceTestBase {
         // Serbian in explicitly Cyrillic characters should be filtered out.
         ImmutableList.of("values-b+sr+Cyrl/foo.xml"));
 
-    assertThat(
-            errorConsumer.getAndClearAttributeWarnings(
-                ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME))
-        .hasSize(1);
+    errorConsumer.assertNoAttributeWarnings(
+        ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME);
   }
 
   @Test
@@ -235,10 +225,8 @@ public class ResourceFilterFactoryTest extends ResourceTestBase {
         // Spanish with another region specified should be filtered out.
         ImmutableList.of("values-es-rUS/foo.xml"));
 
-    assertThat(
-            errorConsumer.getAndClearAttributeWarnings(
-                ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME))
-        .hasSize(1);
+    errorConsumer.assertNoAttributeWarnings(
+        ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME);
   }
 
   @Test
@@ -257,54 +245,6 @@ public class ResourceFilterFactoryTest extends ResourceTestBase {
         ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME);
   }
 
-  /**
-   * Tests that filtering with deprecated resource qualifiers only warns once for each type of
-   * problem in each resource filter object.
-   */
-  @Test
-  public void testFilterDeprecatedQualifierFormatsAttributeWarnings() throws RuleErrorException {
-    ImmutableList<String> badQualifiers =
-        ImmutableList.of("en_US", "sr-Latn", "es-419", "mcc310-en_US", "sr_rLatn", "es_419");
-
-    ImmutableList<String> expectedWarnings =
-        ImmutableList.of(
-            "When referring to Serbian in Latin characters, use of qualifier 'sr-Latn' is"
-                + " deprecated. Use 'b+sr+Latn' instead.",
-            "When referring to Spanish for Latin America and the Caribbean, use of qualifier"
-                + " 'es-419' is deprecated. Use 'b+es+419' instead.",
-            "When referring to locale qualifiers with regions, use of qualifier 'en_US' is"
-                + " deprecated. Use 'en-rUS' instead.");
-
-    ResourceFilterFactory filter =
-        new ResourceFilterFactory(
-            badQualifiers, ImmutableList.of(), FilterBehavior.FILTER_IN_ANALYSIS);
-
-    doFilter(filter, ImmutableList.of());
-    assertThat(
-            errorConsumer.getAndClearAttributeWarnings(
-                ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME))
-        .containsExactlyElementsIn(expectedWarnings);
-    errorConsumer.assertNoRuleWarnings();
-
-    // Filtering again with this filter should not produce additional warnings
-    doFilter(filter, ImmutableList.of());
-    errorConsumer.assertNoAttributeWarnings(
-        ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME);
-    errorConsumer.assertNoRuleWarnings();
-
-    // Filtering with a new filter should produce warnings again, since it is working on a different
-    // target
-    filter =
-        new ResourceFilterFactory(
-            badQualifiers, ImmutableList.of(), FilterBehavior.FILTER_IN_ANALYSIS);
-    doFilter(filter, ImmutableList.of());
-    assertThat(
-            errorConsumer.getAndClearAttributeWarnings(
-                ResourceFilterFactory.RESOURCE_CONFIGURATION_FILTERS_NAME))
-        .containsExactlyElementsIn(expectedWarnings);
-    errorConsumer.assertNoRuleWarnings();
-  }
-
   @Test
   public void testFilterDeprecatedQualifierFormatsRuleWarnings() throws RuleErrorException {
     ImmutableList<Artifact> badResources =
@@ -494,14 +434,10 @@ public class ResourceFilterFactoryTest extends ResourceTestBase {
     assertThat(transitiveToDiscard.getResourcesRoots()).isEmpty();
 
     ResourceContainer transitiveToKeep = transitiveContainers.get(1);
-    assertThat(transitiveToKeep.getResources())
-        .containsExactly(transitiveResourceToKeep);
+    assertThat(transitiveToKeep.getResources()).containsExactly(transitiveResourceToKeep);
     assertThat(transitiveToKeep.getResourcesRoots())
         .containsExactly(
-            transitiveResourceToKeep
-                .getExecPath()
-                .getParentDirectory()
-                .getParentDirectory());
+            transitiveResourceToKeep.getExecPath().getParentDirectory().getParentDirectory());
 
     // We tell the resource processing actions to ignore references to filtered resources from
     // dependencies.