Description redacted.

--
MOS_MIGRATED_REVID=131986739

2 files changed, 59 insertions, 65 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
index ee00868dc5..b47e914b78 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
@@ -447,7 +447,8 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
         .setJavaResourceZip(dexingOutput.javaResourceJar)
         .setNativeLibs(nativeLibs)
         .setUnsignedApk(unsignedApk)
-        .setSignedAndZipalignedApk(zipAlignedApk)
+        .setSignedApk(zipAlignedApk)
+        .setZipalignApk(true)
         .setApkName("apk")
         .registerActions(ruleContext, androidSemantics);
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java
index 5829294b2f..decbc0606f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ApkActionsBuilder.java
@@ -20,8 +20,6 @@ import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.rules.android.AndroidConfiguration.ApkSigningMethod;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import java.util.ArrayList;
-import java.util.List;
 
 /**
  * A class for coordinating APK building, signing and zipaligning.
@@ -42,8 +40,8 @@ public abstract class ApkActionsBuilder {
 
   Artifact unsignedApk;
   Artifact signedApk;
-  Artifact signedAndZipalignedApk;
   String apkName;
+  boolean zipalignApk = false;
 
   /** Sets the user-visible apkName that is included in the action progress messages. */
   public ApkActionsBuilder setApkName(String apkName) {
@@ -106,15 +104,15 @@ public abstract class ApkActionsBuilder {
     return this;
   }
 
-  /** Requests a signed but not necessarily zip aligned APK be built at the specified artifact. */
+  /** Requests a signed APK be built at the specified artifact. */
   public ApkActionsBuilder setSignedApk(Artifact signedApk) {
     this.signedApk = signedApk;
     return this;
   }
 
-  /** Requests a signed and zipaligned APK be built at the specified artifact. */
-  public ApkActionsBuilder setSignedAndZipalignedApk(Artifact signedAndZipalignedApk) {
-    this.signedAndZipalignedApk = signedAndZipalignedApk;
+  /** Requests that signed APKs are zipaligned. */
+  public ApkActionsBuilder setZipalignApk(boolean zipalign) {
+    this.zipalignApk = zipalign;
     return this;
   }
 
@@ -183,12 +181,25 @@ public abstract class ApkActionsBuilder {
     return actionBuilder.build(ruleContext);
   }
 
+  /** Uses the zipalign tool to align the zip boundaries for uncompressed resources by 4 bytes. */
+  Action[] zipalignApk(RuleContext ruleContext, Artifact inputApk, Artifact zipAlignedApk) {
+    return new SpawnAction.Builder()
+        .addInput(inputApk)
+        .addOutput(zipAlignedApk)
+        .setExecutable(AndroidSdkProvider.fromRuleContext(ruleContext).getZipalign())
+        .addArgument("4")
+        .addInputArgument(inputApk)
+        .addOutputArgument(zipAlignedApk)
+        .setProgressMessage("Zipaligning " + apkName)
+        .setMnemonic("AndroidZipAlign")
+        .build(ruleContext);
+  }
+
   /**
    * An implementation that uses ApkBuilderMain to both build and sign APKs and the Android SDK
    * zipalign tool to zipalign. This implementation only supports V1 signature scheme (JAR signing).
    */
   static class LegacySignerApkActionsBuilder extends ApkActionsBuilder {
-
     @Override
     public void registerActions(RuleContext ruleContext, AndroidSemantics semantics) {
       Preconditions.checkNotNull(
@@ -199,57 +210,37 @@ public abstract class ApkActionsBuilder {
             buildApk(ruleContext, unsignedApk, null, "Generating unsigned " + apkName));
       }
 
-      if (signedAndZipalignedApk != null) {
-        Artifact intermediateSignedApk = this.signedApk;
-        if (intermediateSignedApk == null) {
-          // If the caller requested a zipaligned APK but not a signed APK, we still need to build
-          // a signed APK as an intermediate so we construct an artifact.
-          intermediateSignedApk = AndroidBinary.getDxArtifact(
-              ruleContext, "signed_" + signedAndZipalignedApk.getFilename());
+      if (signedApk != null) {
+        // Legacy signing destroys zip aligning, so if zip aligning is requested we build an
+        // intermediate APK that is signed but not zip aligned then zip align it. If zip aligning
+        // is not requested then the output of the buildApk step is the final apk.
+        Artifact intermediateSignedApk;
+        if (zipalignApk) {
+          intermediateSignedApk =
+              AndroidBinary.getDxArtifact(ruleContext, "signed_" + signedApk.getFilename());
+        } else {
+          intermediateSignedApk = signedApk;
         }
+
         ruleContext.registerAction(buildApk(
             ruleContext,
             intermediateSignedApk,
             semantics.getApkDebugSigningKey(ruleContext),
             "Generating signed " + apkName));
-        ruleContext.registerAction(
-            zipalignApk(ruleContext, intermediateSignedApk, signedAndZipalignedApk));
-      } else if (signedApk != null) {
-        ruleContext.registerAction(buildApk(
-            ruleContext,
-            signedApk,
-            semantics.getApkDebugSigningKey(ruleContext),
-            "Generating signed " + apkName));
-      }
-    }
-
-    /** Last step in buildings an apk: align the zip boundaries by 4 bytes. */
-    private Action[] zipalignApk(RuleContext ruleContext, Artifact signedApk,
-        Artifact zipAlignedApk) {
-      List<String> args = new ArrayList<>();
-      // "4" is the only valid value for zipalign, according to:
-      // http://developer.android.com/guide/developing/tools/zipalign.html
-      args.add("4");
-      args.add(signedApk.getExecPathString());
-      args.add(zipAlignedApk.getExecPathString());
 
-      return new SpawnAction.Builder()
-          .addInput(signedApk)
-          .addOutput(zipAlignedApk)
-          .setExecutable(AndroidSdkProvider.fromRuleContext(ruleContext).getZipalign())
-          .addArguments(args)
-          .setProgressMessage("Zipaligning " + apkName)
-          .setMnemonic("AndroidZipAlign")
-          .build(ruleContext);
+        if (zipalignApk) {
+          ruleContext.registerAction(
+              zipalignApk(ruleContext, intermediateSignedApk, signedApk));
+        }
+      }
     }
   }
 
   /**
    * This implementation uses the executables from the apkbuilder and apksigner attributes of
-   * android_sdk to build and sign the SDKs. Zipaligning is done by the apksigner
-   * {@link com.android.apksigner.ApkSignerTool} between the V1 and V2 signature steps. The signer
-   * supports both V1 and V2 signing signatures configured by the {@link ApkSigningMethod}
-   * parameter.
+   * android_sdk to build and sign the SDKs. Zipaligning is done before the apksigner
+   * by the zipalign tool. The signer supports both V1 and V2 signing signatures configured by the
+   * {@link ApkSigningMethod} parameter.
    */
   static class SignerToolApkActionsBuilder extends ApkActionsBuilder {
     private final ApkSigningMethod signingMethod;
@@ -260,31 +251,32 @@ public abstract class ApkActionsBuilder {
 
     @Override
     public void registerActions(RuleContext ruleContext, AndroidSemantics semantics) {
-      // Only one should ever be specified. The only reason that both options exist are as a slight
-      // optimization for the legacy code path in which we only zip align full APKs, not split APKs.
-      Preconditions.checkState(
-          signedApk == null || signedAndZipalignedApk == null,
-          "ApkSignerTool cannot generate separate signedApk and signedAndZipalignedApk because "
-              + "zipaligning is done between the v1 signing and v2 signing in the same action.");
       Preconditions.checkNotNull(
           apkName, "APK name must be set to create progress messages for APK actions.");
 
-      Artifact finalApk = signedApk == null ? signedAndZipalignedApk : signedApk;
-      if (finalApk != null) {
-        Artifact intermediateUnsignedApk = this.unsignedApk;
+      if (signedApk != null) {
+        Artifact intermediateUnsignedApk = unsignedApk;
         if (intermediateUnsignedApk == null) {
           // If the caller did not request an unsigned APK, we still need to construct one so that
           // we can sign it. So we make up an intermediate artifact.
-          intermediateUnsignedApk = AndroidBinary.getDxArtifact(
-              ruleContext, "unsigned_" + finalApk.getFilename());
+          intermediateUnsignedApk =
+              AndroidBinary.getDxArtifact(ruleContext, "unsigned_" + signedApk.getFilename());
         }
         ruleContext.registerAction(
             buildApk(ruleContext, intermediateUnsignedApk, null, "Generating unsigned " + apkName));
-        ruleContext.registerAction(sign(
+
+        Artifact apkToSign = intermediateUnsignedApk;
+        if (zipalignApk) {
+          apkToSign =
+              AndroidBinary.getDxArtifact(ruleContext, "zipaligned_" + signedApk.getFilename());
+          ruleContext.registerAction(zipalignApk(ruleContext, intermediateUnsignedApk, apkToSign));
+        }
+
+        ruleContext.registerAction(signApk(
             ruleContext,
             semantics.getApkDebugSigningKey(ruleContext),
-            intermediateUnsignedApk,
-            finalApk));
+            apkToSign,
+            signedApk));
       } else if (unsignedApk != null) {
         ruleContext.registerAction(
             buildApk(ruleContext, unsignedApk, null, "Generating unsigned " + apkName));
@@ -292,10 +284,11 @@ public abstract class ApkActionsBuilder {
     }
 
     /**
-     * Signs and zip aligns an APK using the ApkSignerTool. Supports both the jar signing schema
-     * (v1) and the apk signing schema v2.
+     * Signs an APK using the ApkSignerTool. Supports both the jar signing scheme(v1) and the apk
+     * signing scheme v2. Note that zip alignment is preserved by this step. Furthermore,
+     * zip alignment cannot be performed after v2 signing without invalidating the signature.
      */
-    private Action[] sign(RuleContext ruleContext, Artifact signingKey, Artifact unsignedApk,
+    private Action[] signApk(RuleContext ruleContext, Artifact signingKey, Artifact unsignedApk,
         Artifact signedAndZipalignedApk) {
       return new SpawnAction.Builder()
           .setExecutable(AndroidSdkProvider.fromRuleContext(ruleContext).getApkSigner())