Get resource processing settings from method params instead of rule attrs

In a previous review, I left some setting controlled only by rule attributes.
Now actually specify them in Skylark method params.

Data binding and should mimic behavior in existing android_* rules.

For library, aapt_version cannot be passed in; we use the flag value.

I left the SDK attribute intact as it's private. People should be setting
defaults in rules but otherwise overriding SDK with flags, not parameters. This
mimics existing behavior.

RELNOTES: none
PiperOrigin-RevId: 195970351

7 files changed, 129 insertions, 62 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 678a5f67cd..c5872ca8cb 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
@@ -230,23 +230,14 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
         throws RuleErrorException {
       if (ruleContext.isLegalFragment(AndroidConfiguration.class)) {
         boolean hasAapt2 = AndroidSdkProvider.fromRuleContext(ruleContext).getAapt2() != null;
-        AndroidAaptVersion flag =
-            ruleContext.getFragment(AndroidConfiguration.class).getAndroidAaptVersion();
 
         if (ruleContext.getRule().isAttrDefined("aapt_version", STRING)) {
           // On rules that can choose a version, test attribute then flag choose the aapt version
           // target.
-          AndroidAaptVersion version =
-              fromString(ruleContext.attributes().get("aapt_version", STRING));
-          // version is null if the value is "auto"
-          version = version == AndroidAaptVersion.AUTO ? flag : version;
-
-          if (version == AAPT2 && !hasAapt2) {
-            ruleContext.throwWithRuleError(
-                "aapt2 processing requested but not available on the android_sdk");
-            return null;
-          }
-          return version == AndroidAaptVersion.AUTO ? AAPT : version;
+          return chooseTargetAaptVersion(
+              ruleContext,
+              ruleContext.getFragment(AndroidConfiguration.class),
+              ruleContext.attributes().get("aapt_version", STRING));
         } else {
           // On rules can't choose, assume aapt2 if aapt2 is present in the sdk.
           return hasAapt2 ? AAPT2 : AAPT;
@@ -254,6 +245,26 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
       }
       return null;
     }
+
+    @Nullable
+    public static AndroidAaptVersion chooseTargetAaptVersion(
+        RuleContext ruleContext, AndroidConfiguration androidConfig, @Nullable String versionString)
+        throws RuleErrorException {
+
+      boolean hasAapt2 = AndroidSdkProvider.fromRuleContext(ruleContext).getAapt2() != null;
+      AndroidAaptVersion flag = androidConfig.getAndroidAaptVersion();
+
+      AndroidAaptVersion version = fromString(versionString);
+      // version is null if the value is "auto"
+      version = version == AndroidAaptVersion.AUTO ? flag : version;
+
+      if (version == AAPT2 && !hasAapt2) {
+        ruleContext.throwWithRuleError(
+            "aapt2 processing requested but not available on the android_sdk");
+        return null;
+      }
+      return version == AndroidAaptVersion.AUTO ? AAPT : version;
+    }
   }
 
   /** Android configuration options. */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResources.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResources.java
index d548ddbcdf..7858091b3d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResources.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResources.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTa
 import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
 import com.google.devtools.build.lib.packages.RuleErrorConsumer;
+import com.google.devtools.build.lib.rules.android.AndroidConfiguration.AndroidAaptVersion;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Arrays;
 import java.util.LinkedHashSet;
@@ -399,9 +400,14 @@ public class AndroidResources {
   }
 
   /** Parses these resources. */
-  public ParsedAndroidResources parse(RuleContext ruleContext, StampedAndroidManifest manifest)
-      throws InterruptedException, RuleErrorException {
-    return ParsedAndroidResources.parseFrom(ruleContext, this, manifest);
+  public ParsedAndroidResources parse(
+      RuleContext ruleContext,
+      StampedAndroidManifest manifest,
+      boolean enableDataBinding,
+      AndroidAaptVersion aaptVersion)
+      throws InterruptedException {
+    return ParsedAndroidResources.parseFrom(
+        ruleContext, this, manifest, enableDataBinding, aaptVersion);
   }
 
   /**
@@ -411,7 +417,13 @@ public class AndroidResources {
   public ValidatedAndroidResources process(
       RuleContext ruleContext, StampedAndroidManifest manifest, boolean neverlink)
       throws RuleErrorException, InterruptedException {
-    return parse(ruleContext, manifest).merge(ruleContext, neverlink).validate(ruleContext);
+    boolean enableDataBinding = DataBinding.isEnabled(ruleContext);
+    AndroidAaptVersion aaptVersion = AndroidAaptVersion.chooseTargetAaptVersion(ruleContext);
+    ResourceDependencies resourceDeps = ResourceDependencies.fromRuleDeps(ruleContext, neverlink);
+
+    return parse(ruleContext, manifest, enableDataBinding, aaptVersion)
+        .merge(ruleContext, resourceDeps, enableDataBinding, aaptVersion)
+        .validate(ruleContext, aaptVersion);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesInfo.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesInfo.java
index 6f1d665be4..f6626d8a04 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidResourcesInfo.java
@@ -25,10 +25,9 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
 
 /** A provider that supplies ResourceContainers from its transitive closure. */
 @SkylarkModule(
-  name = "AndroidResourcesInfo",
-  doc = "Android resources provided by a rule",
-  category = SkylarkModuleCategory.PROVIDER
-)
+    name = "AndroidResourcesInfo",
+    doc = "Android resources provided by a rule",
+    category = SkylarkModuleCategory.PROVIDER)
 @Immutable
 public class AndroidResourcesInfo extends NativeInfo {
 
@@ -113,20 +112,18 @@ public class AndroidResourcesInfo extends NativeInfo {
 
   /** Returns the transitive ResourceContainers for the label. */
   @SkylarkCallable(
-    name = "transitive_android_resources",
-    doc = "Returns the transitive android resources for the label.",
-    structField = true
-  )
+      name = "transitive_android_resources",
+      doc = "Returns the transitive android resources for the label.",
+      structField = true)
   public NestedSet<ValidatedAndroidData> getTransitiveAndroidResources() {
     return transitiveAndroidResources;
   }
 
   /** Returns the immediate ResourceContainers for the label. */
   @SkylarkCallable(
-    name = "direct_android_resources",
-    doc = "Returns the immediate android resources for the label.",
-    structField = true
-  )
+      name = "direct_android_resources",
+      doc = "Returns the immediate android resources for the label.",
+      structField = true)
   public NestedSet<ValidatedAndroidData> getDirectAndroidResources() {
     return directAndroidResources;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSkylarkData.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSkylarkData.java
index d0c3819e41..3566594fa1 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSkylarkData.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSkylarkData.java
@@ -24,6 +24,7 @@ import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.NativeInfo;
 import com.google.devtools.build.lib.packages.NativeProvider;
 import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
+import com.google.devtools.build.lib.rules.android.AndroidConfiguration.AndroidAaptVersion;
 import com.google.devtools.build.lib.rules.android.AndroidLibraryAarInfo.Aar;
 import com.google.devtools.build.lib.rules.java.JavaCompilationInfoProvider;
 import com.google.devtools.build.lib.rules.java.JavaInfo;
@@ -116,7 +117,19 @@ public class AndroidSkylarkData {
             named = true,
             doc =
                 "Defaults to False. If true, resources will not be exposed to targets that depend"
-                    + " on them.")
+                    + " on them."),
+        @Param(
+            name = "custom_package",
+            positional = false,
+            defaultValue = "None",
+            type = String.class,
+            noneable = true,
+            named = true,
+            doc =
+                "The Android application package to stamp the manifest with. If not provided, the"
+                    + " current Java package, derived from the location of this target's BUILD"
+                    + " file, will be used. For example, given a BUILD file in"
+                    + " 'java/com/foo/bar/BUILD', the package would be 'com.foo.bar'."),
       },
       doc =
           "Creates an AndroidResourcesInfo from this target's resource dependencies, ignoring local"
@@ -125,13 +138,20 @@ public class AndroidSkylarkData {
               + " manifest will be generated and included in the provider - this path should not"
               + " be used when an explicit manifest is specified.")
   public static AndroidResourcesInfo resourcesFromDeps(
-      SkylarkRuleContext ctx, SkylarkList<AndroidResourcesInfo> deps, boolean neverlink)
+      SkylarkRuleContext ctx,
+      SkylarkList<AndroidResourcesInfo> deps,
+      boolean neverlink,
+      Object customPackage)
       throws EvalException, InterruptedException {
+    String pkg = fromNoneable(customPackage, String.class);
+    if (pkg == null) {
+      pkg = AndroidManifest.getDefaultPackage(ctx.getRuleContext());
+    }
     return ResourceApk.processFromTransitiveLibraryData(
             ctx.getRuleContext(),
             ResourceDependencies.fromProviders(deps, /* neverlink = */ neverlink),
             AssetDependencies.empty(),
-            StampedAndroidManifest.createEmpty(ctx.getRuleContext(), /* exported = */ false))
+            StampedAndroidManifest.createEmpty(ctx.getRuleContext(), pkg, /* exported = */ false))
         .toResourceInfo(ctx.getLabel());
   }
 
@@ -315,6 +335,15 @@ public class AndroidSkylarkData {
             doc =
                 "Defaults to False. If passed as True, these resources will not be inherited by"
                     + " targets that depend on this one."),
+        @Param(
+            name = "enable_data_binding",
+            positional = false,
+            defaultValue = "False",
+            type = Boolean.class,
+            named = true,
+            doc =
+                "Defaults to False. If True, processes data binding expressions in layout"
+                    + " resources."),
       },
       doc =
           "Merges this target's resources together with resources inherited from dependencies."
@@ -330,7 +359,8 @@ public class AndroidSkylarkData {
       AndroidManifestInfo manifest,
       SkylarkList<ConfiguredTarget> resources,
       SkylarkList<AndroidResourcesInfo> deps,
-      boolean neverlink)
+      boolean neverlink,
+      boolean enableDataBinding)
       throws EvalException, InterruptedException {
 
     ImmutableList<FileProvider> fileProviders =
@@ -341,11 +371,22 @@ public class AndroidSkylarkData {
             .collect(ImmutableList.toImmutableList());
 
     try {
+      AndroidAaptVersion aaptVersion =
+          AndroidCommon.getAndroidConfig(ctx.getRuleContext()).getAndroidAaptVersion();
+
       ValidatedAndroidResources validated =
           AndroidResources.from(ctx.getRuleContext(), fileProviders, "resources")
-              .parse(ctx.getRuleContext(), manifest.asStampedManifest())
-              .merge(ctx.getRuleContext(), ResourceDependencies.fromProviders(deps, neverlink))
-              .validate(ctx.getRuleContext());
+              .parse(
+                  ctx.getRuleContext(),
+                  manifest.asStampedManifest(),
+                  enableDataBinding,
+                  aaptVersion)
+              .merge(
+                  ctx.getRuleContext(),
+                  ResourceDependencies.fromProviders(deps, neverlink),
+                  enableDataBinding,
+                  aaptVersion)
+              .validate(ctx.getRuleContext(), aaptVersion);
 
       JavaInfo javaInfo =
           JavaInfo.Builder.create()
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/MergedAndroidResources.java b/src/main/java/com/google/devtools/build/lib/rules/android/MergedAndroidResources.java
index 8229b3aabf..181752cdbb 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/MergedAndroidResources.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/MergedAndroidResources.java
@@ -38,14 +38,17 @@ public class MergedAndroidResources extends ParsedAndroidResources {
   private final ProcessedAndroidManifest manifest;
 
   public static MergedAndroidResources mergeFrom(
-      RuleContext ruleContext, ParsedAndroidResources parsed, ResourceDependencies resourceDeps)
-      throws InterruptedException, RuleErrorException {
+      RuleContext ruleContext,
+      ParsedAndroidResources parsed,
+      ResourceDependencies resourceDeps,
+      boolean enableDataBinding,
+      AndroidAaptVersion aaptVersion)
+      throws InterruptedException {
 
     AndroidConfiguration androidConfiguration = AndroidCommon.getAndroidConfig(ruleContext);
 
     boolean useCompiledMerge =
-        AndroidAaptVersion.chooseTargetAaptVersion(ruleContext) == AndroidAaptVersion.AAPT2
-            && androidConfiguration.skipParsingAction();
+        aaptVersion == AndroidAaptVersion.AAPT2 && androidConfiguration.skipParsingAction();
 
     Preconditions.checkState(
         !useCompiledMerge || parsed.getCompiledSymbols() != null,
@@ -58,7 +61,7 @@ public class MergedAndroidResources extends ParsedAndroidResources {
             .setThrowOnResourceConflict(androidConfiguration.throwOnResourceConflict())
             .setUseCompiledMerge(useCompiledMerge);
 
-    if (DataBinding.isEnabled(ruleContext)) {
+    if (enableDataBinding) {
       builder.setDataBindingInfoZip(DataBinding.getLayoutInfoFile(ruleContext));
     }
 
@@ -142,12 +145,12 @@ public class MergedAndroidResources extends ParsedAndroidResources {
   /**
    * Validates and packages this rule's resources.
    *
-   * <p>See {@link ValidatedAndroidResources#validateFrom(RuleContext, MergedAndroidResources)}.
-   * This method is a convenience method for calling that one.
+   * <p>See {@link ValidatedAndroidResources#validateFrom(RuleContext, MergedAndroidResources,
+   * AndroidAaptVersion)}. This method is a convenience method for calling that one.
    */
-  public ValidatedAndroidResources validate(RuleContext ruleContext)
-      throws InterruptedException, RuleErrorException {
-    return ValidatedAndroidResources.validateFrom(ruleContext, this);
+  public ValidatedAndroidResources validate(RuleContext ruleContext, AndroidAaptVersion aaptVersion)
+      throws InterruptedException {
+    return ValidatedAndroidResources.validateFrom(ruleContext, this, aaptVersion);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ParsedAndroidResources.java b/src/main/java/com/google/devtools/build/lib/rules/android/ParsedAndroidResources.java
index eddaa71b25..8778414076 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ParsedAndroidResources.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ParsedAndroidResources.java
@@ -32,16 +32,19 @@ public class ParsedAndroidResources extends AndroidResources
   private final StampedAndroidManifest manifest;
 
   public static ParsedAndroidResources parseFrom(
-      RuleContext ruleContext, AndroidResources resources, StampedAndroidManifest manifest)
-      throws RuleErrorException, InterruptedException {
+      RuleContext ruleContext,
+      AndroidResources resources,
+      StampedAndroidManifest manifest,
+      boolean enableDataBinding,
+      AndroidAaptVersion aaptVersion)
+      throws InterruptedException {
 
-    boolean isAapt2 =
-        AndroidAaptVersion.chooseTargetAaptVersion(ruleContext) == AndroidAaptVersion.AAPT2;
+    boolean isAapt2 = aaptVersion == AndroidAaptVersion.AAPT2;
 
     AndroidResourceParsingActionBuilder builder =
         new AndroidResourceParsingActionBuilder(ruleContext);
 
-    if (DataBinding.isEnabled(ruleContext) && isAapt2) {
+    if (enableDataBinding && isAapt2) {
       // TODO(corysmith): Centralize the data binding processing and zipping into a single
       // action. Data binding processing needs to be triggered here as well as the merger to
       // avoid aapt2 from throwing an error during compilation.
@@ -123,14 +126,14 @@ public class ParsedAndroidResources extends AndroidResources
   }
 
   /** Merges this target's resources with resources from dependencies. */
-  public MergedAndroidResources merge(RuleContext ruleContext, boolean neverlink)
-      throws InterruptedException, RuleErrorException {
-    return merge(ruleContext, ResourceDependencies.fromRuleDeps(ruleContext, neverlink));
-  }
-
-  MergedAndroidResources merge(RuleContext ruleContext, ResourceDependencies resourceDeps)
-      throws InterruptedException, RuleErrorException {
-    return MergedAndroidResources.mergeFrom(ruleContext, this, resourceDeps);
+  MergedAndroidResources merge(
+      RuleContext ruleContext,
+      ResourceDependencies resourceDeps,
+      boolean enableDataBinding,
+      AndroidAaptVersion aaptVersion)
+      throws InterruptedException {
+    return MergedAndroidResources.mergeFrom(
+        ruleContext, this, resourceDeps, enableDataBinding, aaptVersion);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ValidatedAndroidResources.java b/src/main/java/com/google/devtools/build/lib/rules/android/ValidatedAndroidResources.java
index 528c53fd35..69ca6ace74 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ValidatedAndroidResources.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ValidatedAndroidResources.java
@@ -55,8 +55,8 @@ public class ValidatedAndroidResources extends MergedAndroidResources
    * </ul>
    */
   public static ValidatedAndroidResources validateFrom(
-      RuleContext ruleContext, MergedAndroidResources merged)
-      throws InterruptedException, RuleErrorException {
+      RuleContext ruleContext, MergedAndroidResources merged, AndroidAaptVersion aaptVersion)
+      throws InterruptedException {
     AndroidResourceValidatorActionBuilder builder =
         new AndroidResourceValidatorActionBuilder(ruleContext)
             .setJavaPackage(merged.getJavaPackage())
@@ -72,7 +72,7 @@ public class ValidatedAndroidResources extends MergedAndroidResources
                 ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_LIBRARY_APK))
             .withDependencies(merged.getResourceDependencies());
 
-    if (AndroidAaptVersion.chooseTargetAaptVersion(ruleContext) == AndroidAaptVersion.AAPT2) {
+    if (aaptVersion == AndroidAaptVersion.AAPT2) {
       builder
           .setCompiledSymbols(merged.getCompiledSymbols())
           .setAapt2RTxtOut(