Use dynamically configured resource filtering

Actually hook up the resource filtering configuration transitions to
AndroidConfiguration's topLevelConfigurationHook.

Dynamic configuration is inefficient when multiple configurations are used.
Multiple configurations result in splitting the build graph and duplicating
work.

To avoid using multiple configurations as much as possible, dynamically
configured resource filtering will only be applied for top-level android_binary
targets. When android_binary targets are included as dependencies, it's very
likely that multiple binaries with many shared dependencies but different
configurations would be used. Only applying dynamic filtering to top-level
binaries removes this concern.

It is still possible to build multiple top-level binary targets with different
configurations at once. Previous versions of this code considered not using
dynamic configuration in that case as well, but that raised some unanswered
questions about whether Bazel's invariants should allow modifying one target's
configuration based solely on targets that happen to be building in parallel.
As a result, that optimization is not included for now; we assume that
developers manually building multiple similar binaries at once is relatively
rare (plus, dynamically configured resource filtering in general is not turned
on by default and will not be turned on by default until we're confident the
benefits outweigh the costs).

RELNOTES: none
PiperOrigin-RevId: 163464415

2 files changed, 42 insertions, 35 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
index e34d0d3d03..b94fbab04c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
@@ -29,10 +29,13 @@ import com.google.devtools.build.lib.analysis.config.ConfigurationEnvironment;
 import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
+import com.google.devtools.build.lib.analysis.config.PatchTransition;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
 import com.google.devtools.build.lib.packages.Attribute.SplitTransition;
 import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
+import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.DynamicMode;
 import com.google.devtools.build.lib.rules.cpp.CppOptions.DynamicModeConverter;
 import com.google.devtools.build.lib.rules.cpp.CppOptions.LibcTopLabelConverter;
@@ -947,4 +950,12 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
 
     return configurationDistinguisher.suffix + "_" + resourceFilterSuffix;
   }
+
+  @Nullable
+  @Override
+  public PatchTransition topLevelConfigurationHook(Target toTarget) {
+    return resourceFilter.getTopLevelPatchTransition(
+        toTarget.getAssociatedRule().getRuleClass(),
+        AggregatingAttributeMapper.of(toTarget.getAssociatedRule()));
+  }
 }
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 14624286de..b8a25b19df 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
@@ -244,6 +244,7 @@ public class ResourceFilter {
 
     return filterBuilder.build();
   }
+
   private FolderConfiguration getFolderConfiguration(
       RuleErrorConsumer ruleErrorConsumer, String filter) {
 
@@ -320,30 +321,30 @@ public class ResourceFilter {
   }
 
   /** List of deprecated qualifiers that should currently by handled with a warning */
-  private final List<DeprecatedQualifierHandler> deprecatedQualifierHandlers = ImmutableList.of(
-      /*
-       * Aapt used to expect locale configurations of form 'en_US'. It now also supports the correct
-       * 'en-rUS' format. For backwards comparability, use a regex to convert filters with locales
-       * in the old format to filters with locales of the correct format.
-       *
-       * The correct format for locales is defined at
-       * https://developer.android.com/guide/topics/resources/providing-resources.html#LocaleQualifier
-       *
-       * TODO(bazel-team): Migrate consumers away from the old Aapt locale format, then remove this
-       * replacement.
-       *
-       * The regex is a bit complicated to avoid modifying potential new qualifiers that contain
-       * underscores. Specifically, it searches for the entire beginning of the resource qualifier,
-       * including (optionally) MCC and MNC, and then the locale itself.
-       */
-      new DeprecatedQualifierHandler(
-          "^((mcc[0-9]{3}-(mnc[0-9]{3}-)?)?[a-z]{2})_([A-Z]{2}).*",
-          "$1-r$4", "locale qualifiers with regions"),
-      new DeprecatedQualifierHandler(
-          "sr[_\\-]r?Latn.*", "b+sr+Latn", "Serbian in Latin characters"),
-      new DeprecatedQualifierHandler(
-          "es[_\\-]419.*", "b+es+419", "Spanish for Latin America and the Caribbean")
-  );
+  private final List<DeprecatedQualifierHandler> deprecatedQualifierHandlers =
+      ImmutableList.of(
+          /*
+           * Aapt used to expect locale configurations of form 'en_US'. It now also supports the
+           * correct 'en-rUS' format. For backwards comparability, use a regex to convert filters
+           * with locales in the old format to filters with locales of the correct format.
+           *
+           * The correct format for locales is defined at
+           * https://developer.android.com/guide/topics/resources/providing-resources.html#LocaleQualifier
+           *
+           * TODO(bazel-team): Migrate consumers away from the old Aapt locale format, then remove
+           * this replacement.
+           *
+           * The regex is a bit complicated to avoid modifying potential new qualifiers that contain
+           * underscores. Specifically, it searches for the entire beginning of the resource
+           * qualifier, including (optionally) MCC and MNC, and then the locale itself.
+           */
+          new DeprecatedQualifierHandler(
+              "^((mcc[0-9]{3}-(mnc[0-9]{3}-)?)?[a-z]{2})_([A-Z]{2}).*",
+              "$1-r$4", "locale qualifiers with regions"),
+          new DeprecatedQualifierHandler(
+              "sr[_\\-]r?Latn.*", "b+sr+Latn", "Serbian in Latin characters"),
+          new DeprecatedQualifierHandler(
+              "es[_\\-]419.*", "b+es+419", "Spanish for Latin America and the Caribbean"));
 
   private ImmutableList<Density> getDensities(RuleErrorConsumer ruleErrorConsumer) {
     ImmutableList.Builder<Density> densityBuilder = ImmutableList.builder();
@@ -762,24 +763,19 @@ public class ResourceFilter {
   // Transitions for dealing with dynamically configured resource filtering:
 
   @Nullable
-  PatchTransition getTopLevelPatchTransition(
-      String ruleClass, int topLevelTargetCount, AttributeMap attrs) {
+  PatchTransition getTopLevelPatchTransition(String ruleClass, AttributeMap attrs) {
     if (!usesDynamicConfiguration()) {
       // We're not using dynamic configuration, so we don't need to make a transition
       return null;
     }
 
-    if (topLevelTargetCount != 1 || !ruleClass.equals("android_binary")) {
-      // The presence of other top-level targets means we would potentially encounter multiple
-      // resource filtering settings, which, when combined with dynamic configuration, would
-      // probably split the build graph and slow everything down. For now, just use static resource
-      // filtering instead.
-      return REMOVE_DYNAMICALLY_CONFIGURED_RESOURCE_FILTERING_TRANSITION;
-    }
-
-    if (!ResourceFilter.hasFilters(attrs)) {
+    if (!ruleClass.equals("android_binary") || !ResourceFilter.hasFilters(attrs)) {
       // This target doesn't specify any filtering settings, so dynamically configured resource
       // filtering would be a waste of time.
+      // If the target's dependencies include android_binary targets, those dependencies might
+      // specify filtering settings, but we don't apply them dynamically since the chances of
+      // encountering differing settings (leading to splitting the build graph and poor overall
+      // performance) are high.
       return REMOVE_DYNAMICALLY_CONFIGURED_RESOURCE_FILTERING_TRANSITION;
     }