Automated rollback of commit 29b57c3afcfeb8e3fedcc2edcb0f28f13c784179.

*** Reason for rollback ***

Rolling forward with assets being included after relinking.

*** Original change description ***

Automated rollback of commit bab4b04ad09a571615d04aacadf8b3a2b820ba5f.

*** Reason for rollback ***

Appears to cause issues with the new apk.

*** Original change description ***

Relink instead of convert proto apks.

RELNOTES: None
PiperOrigin-RevId: 208245517

3 files changed, 186 insertions, 163 deletions

diff --git a/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourceShrinkingAction.java b/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourceShrinkingAction.java
index 2db283fdca..e9fa1cb338 100644
--- a/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourceShrinkingAction.java
+++ b/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourceShrinkingAction.java
@@ -95,7 +95,8 @@ public class Aapt2ResourceShrinkingAction {
       final ResourceLinker linker =
           ResourceLinker.create(
                   aapt2ConfigOptions.aapt2, executorService, scopedTmp.subDirectoryOf("linking"))
-              .profileUsing(profiler);
+              .profileUsing(profiler)
+              .dependencies(ImmutableList.of(StaticLibrary.from(aapt2ConfigOptions.androidJar)));
 
       final Set<String> packages =
           options
diff --git a/src/tools/android/java/com/google/devtools/build/android/ResourcesZip.java b/src/tools/android/java/com/google/devtools/build/android/ResourcesZip.java
index 3e1ecda49e..c4998ce917 100644
--- a/src/tools/android/java/com/google/devtools/build/android/ResourcesZip.java
+++ b/src/tools/android/java/com/google/devtools/build/android/ResourcesZip.java
@@ -261,7 +261,7 @@ public class ResourcesZip {
               shrunkApkProto,
               toolAttributes.getOrDefault(SdkConstants.ATTR_KEEP, ImmutableSet.of()),
               toolAttributes.getOrDefault(SdkConstants.ATTR_DISCARD, ImmutableSet.of()));
-      return new ShrunkProtoApk(shrunkApkProto, logFile);
+      return new ShrunkProtoApk(shrunkApkProto, logFile, ids);
     }
   }
 
@@ -277,16 +277,18 @@ public class ResourcesZip {
   static class ShrunkProtoApk {
     private final Path apk;
     private final Path report;
+    private final Path ids;
 
-    ShrunkProtoApk(Path apk, Path report) {
+    ShrunkProtoApk(Path apk, Path report, Path ids) {
       this.apk = apk;
       this.report = report;
+      this.ids = ids;
     }
 
     ShrunkProtoApk writeBinaryTo(ResourceLinker linker, Path binaryOut, boolean writeAsProto)
         throws IOException {
       Files.copy(
-          writeAsProto ? apk : linker.optimizeApk(linker.convertToBinary(apk)),
+          writeAsProto ? apk : linker.link(ProtoApk.readFrom(apk), ids),
           binaryOut,
           StandardCopyOption.REPLACE_EXISTING);
       return this;
diff --git a/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceLinker.java b/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceLinker.java
index 8930883dd8..32590f2a7e 100644
--- a/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceLinker.java
+++ b/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceLinker.java
@@ -13,6 +13,15 @@
 // limitations under the License.
 package com.google.devtools.build.android.aapt2;
 
+import static com.google.devtools.build.android.ziputils.DataDescriptor.EXTCRC;
+import static com.google.devtools.build.android.ziputils.DataDescriptor.EXTLEN;
+import static com.google.devtools.build.android.ziputils.DataDescriptor.EXTSIZ;
+import static com.google.devtools.build.android.ziputils.DirectoryEntry.CENCRC;
+import static com.google.devtools.build.android.ziputils.DirectoryEntry.CENLEN;
+import static com.google.devtools.build.android.ziputils.DirectoryEntry.CENSIZ;
+import static com.google.devtools.build.android.ziputils.DirectoryEntry.CENTIM;
+import static com.google.devtools.build.android.ziputils.LocalFileHeader.LOCFLG;
+import static com.google.devtools.build.android.ziputils.LocalFileHeader.LOCTIM;
 import static java.util.stream.Collectors.toList;
 
 import com.android.builder.core.VariantType;
@@ -31,7 +40,11 @@ import com.google.devtools.build.android.AndroidResourceOutputs;
 import com.google.devtools.build.android.FullyQualifiedName;
 import com.google.devtools.build.android.Profiler;
 import com.google.devtools.build.android.aapt2.ResourceCompiler.CompiledType;
+import com.google.devtools.build.android.ziputils.DataDescriptor;
 import com.google.devtools.build.android.ziputils.DirectoryEntry;
+import com.google.devtools.build.android.ziputils.DosTime;
+import com.google.devtools.build.android.ziputils.EntryHandler;
+import com.google.devtools.build.android.ziputils.LocalFileHeader;
 import com.google.devtools.build.android.ziputils.ZipIn;
 import com.google.devtools.build.android.ziputils.ZipOut;
 import java.io.IOException;
@@ -40,6 +53,7 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -77,7 +91,6 @@ public class ResourceLinker {
 
   private static final ImmutableSet<String> PSEUDO_LOCALE_FILTERS =
       ImmutableSet.of("en_XA", "ar_XB");
-  private static final String OPTIMIZED_EXTENSION = ".optimized-apk";
 
   /** Represents errors thrown during linking. */
   public static class LinkError extends Aapt2Exception {
@@ -335,22 +348,20 @@ public class ResourceLinker {
     return fileName.substring(0, lastIndex).concat(".").concat(newExtension);
   }
 
-  public Path convertToBinary(Path apk) {
-    if (apk.getFileName().toString().endsWith(BINARY_EXTENSION)) {
-      return apk;
-    }
+  private Path convertToBinary(ProtoApk apk) {
+    Path apkPath = apk.asApkPath();
     try {
       profiler.startTask("convertToBinary");
       final Path outPath =
           workingDirectory.resolveSibling(
-              replaceExtension(apk.getFileName().toString(), BINARY_EXTENSION));
+              replaceExtension(apkPath.getFileName().toString(), BINARY_EXTENSION));
       logger.fine(
           new AaptCommandBuilder(aapt2)
               .add("convert")
               .add("-o", outPath)
               .add("--output-format", "binary")
-              .add(apk.toString())
-              .execute("Converting " + apk));
+              .add(apkPath.toString())
+              .execute("Converting " + apkPath));
       profiler.recordEndOf("convertToBinary");
       return outPath;
     } catch (IOException e) {
@@ -358,182 +369,188 @@ public class ResourceLinker {
     }
   }
 
-  public Path convertToProto(Path apk) {
-    if (apk.getFileName().toString().endsWith(PROTO_EXTENSION)) {
-      return apk;
-    }
-    try {
-      profiler.startTask("convertToProto");
-      final Path outPath =
-          workingDirectory.resolveSibling(
-              replaceExtension(apk.getFileName().toString(), PROTO_EXTENSION));
-      logger.fine(
-          new AaptCommandBuilder(aapt2)
-              .add("convert")
-              .add("-o", outPath)
-              .add("--output-format", "proto")
-              .add(apk.toString())
-              .execute("Converting " + apk));
-      profiler.recordEndOf("convertToProto");
-      return outPath;
-    } catch (IOException e) {
-      throw new LinkError(e);
-    }
+  private ProtoApk linkProtoApk(
+      CompiledResources compiled,
+      Path rTxt,
+      Path proguardConfig,
+      Path mainDexProguard,
+      Path javaSourceDirectory,
+      Path resourceIds)
+      throws IOException {
+    profiler.startTask("fulllink");
+    final Path linked = workingDirectory.resolve("bin." + PROTO_EXTENSION);
+    logger.fine(
+        new AaptCommandBuilder(aapt2)
+            .forBuildToolsVersion(buildToolsVersion)
+            .forVariantType(VariantType.DEFAULT)
+            .add("link")
+            .whenVersionIsAtLeast(new Revision(23))
+            .thenAdd("--no-version-vectors")
+            // Turn off namespaced resources
+            .add("--no-static-lib-packages")
+            .when(Objects.equals(logger.getLevel(), Level.FINE))
+            .thenAdd("-v")
+            .add("--manifest", compiled.getManifest())
+            // Enables resource redefinition and merging
+            .add("--auto-add-overlay")
+            // Always link to proto, as resource shrinking needs the extra information.
+            .add("--proto-format")
+            .when(debug)
+            .thenAdd("--debug-mode")
+            .add("--custom-package", customPackage)
+            .when(densities.size() == 1)
+            .thenAddRepeated("--preferred-density", densities)
+            .add("--stable-ids", compiled.getStableIds())
+            .addRepeated(
+                "-A",
+                Streams.concat(
+                        assetDirs.stream().map(Path::toString),
+                        compiled.getAssetsStrings().stream())
+                    .collect(toList()))
+            .addRepeated("-I", StaticLibrary.toPathStrings(linkAgainst))
+            .addParameterableRepeated(
+                "-R",
+                compiledResourcesToPaths(
+                    compiled,
+                    generatePseudoLocale
+                            && resourceConfigs.stream().anyMatch(PSEUDO_LOCALE_FILTERS::contains)
+                        ? IS_FLAT_FILE.and(USE_GENERATED)
+                        : IS_FLAT_FILE.and(USE_DEFAULT)),
+                workingDirectory)
+            // Never compress apks.
+            .add("-0", "apk")
+            // Add custom no-compress extensions.
+            .addRepeated("-0", uncompressedExtensions)
+            // Filter by resource configuration type.
+            .when(!resourceConfigs.isEmpty())
+            .thenAdd("-c", Joiner.on(',').join(resourceConfigs))
+            .add("--output-text-symbols", rTxt)
+            .add("--emit-ids", resourceIds)
+            .add("--java", javaSourceDirectory)
+            .add("--proguard", proguardConfig)
+            .add("--proguard-main-dex", mainDexProguard)
+            .when(conditionalKeepRules)
+            .thenAdd("--proguard-conditional-keep-rules")
+            .add("-o", linked)
+            .execute(String.format("Linking %s", compiled.getManifest())));
+    profiler.recordEndOf("fulllink");
+    return ProtoApk.readFrom(
+        densities.size() < 2 ? linked : optimizeForDensities(compiled, linked));
   }
 
-  public Path optimizeApk(Path apk) {
-    try {
-      if (apk.getFileName().toString().endsWith(OPTIMIZED_EXTENSION)) {
-        return apk;
-      }
-
-      profiler.startTask("optimizeApk");
-      final Path outPath =
-          workingDirectory.resolveSibling(
-              replaceExtension(apk.getFileName().toString(), OPTIMIZED_EXTENSION));
-      logger.fine(
-          new AaptCommandBuilder(aapt2)
-              .forBuildToolsVersion(buildToolsVersion)
-              .forVariantType(VariantType.DEFAULT)
-              .add("optimize")
-              .when(Objects.equals(logger.getLevel(), Level.FINE))
-              .thenAdd("-v")
-              .add("-o", outPath)
-              .add(apk.toString())
-              .execute(String.format("Optimizing %s", apk)));
-      return outPath;
-    } catch (IOException e) {
-      throw new LinkError(e);
+  private Path combineApks(Path protoApk, Path binaryApk, Path workingDirectory)
+      throws IOException {
+    // Linking against apk as a static library elides assets, amoung other things.
+    // So, copy the missing details to the new apk.
+    profiler.startTask("combine");
+    final Path combined = workingDirectory.resolve("combined.apk");
+    try (FileChannel nonResourceChannel = FileChannel.open(protoApk, StandardOpenOption.READ);
+        FileChannel resourceChannel = FileChannel.open(binaryApk, StandardOpenOption.READ);
+        FileChannel outChannel =
+            FileChannel.open(combined, StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE)) {
+      final ZipIn resourcesIn = new ZipIn(resourceChannel, binaryApk.toString());
+      final ZipIn nonResourcesIn = new ZipIn(nonResourceChannel, protoApk.toString());
+      final ZipOut zipOut = new ZipOut(outChannel, combined.toString());
+
+      Set<String> skip = new HashSet<>();
+      skip.add("resources.pb");
+      final EntryHandler entryHandler =
+          (in, header, dirEntry, data) -> {
+            final String filename = dirEntry.getFilename();
+            // Make sure we aren't copying the same entry twice.
+            if (!skip.contains(filename)) {
+              skip.add(filename);
+              String comment = dirEntry.getComment();
+              byte[] extra = dirEntry.getExtraData();
+              zipOut.nextEntry(
+                  dirEntry.clone(filename, extra, comment).set(CENTIM, DosTime.EPOCH.time));
+              zipOut.write(header.clone(filename, extra).set(LOCTIM, DosTime.EPOCH.time));
+              zipOut.write(data);
+              if ((header.get(LOCFLG) & LocalFileHeader.SIZE_MASKED_FLAG) != 0) {
+                DataDescriptor desc =
+                    DataDescriptor.allocate()
+                        .set(EXTCRC, dirEntry.get(CENCRC))
+                        .set(EXTSIZ, dirEntry.get(CENSIZ))
+                        .set(EXTLEN, dirEntry.get(CENLEN));
+                zipOut.write(desc);
+              }
+            }
+          };
+      resourcesIn.scanEntries(entryHandler);
+      nonResourcesIn.scanEntries(entryHandler);
+      zipOut.close();
+      return combined;
     } finally {
-      profiler.recordEndOf("optimizeApk");
+      profiler.recordEndOf("combine");
     }
   }
 
+  private Path extractAttributes(CompiledResources compiled) throws IOException {
+    profiler.startTask("attributes");
+    Path attributes = workingDirectory.resolve("tool.attributes");
+    // extract tool annotations from the compile resources.
+    final SdkToolAttributeWriter writer = new SdkToolAttributeWriter(attributes);
+    Stream.concat(include.stream(), Stream.of(compiled))
+        .parallel()
+        .map(AndroidCompiledDataDeserializer.create()::readAttributes)
+        .map(Map::entrySet)
+        .flatMap(Set::stream)
+        .distinct()
+        .forEach(e -> e.getValue().writeResource((FullyQualifiedName) e.getKey(), writer));
+    writer.flush();
+    profiler.recordEndOf("attributes");
+    return attributes;
+  }
+
+  private Path optimizeForDensities(CompiledResources compiled, Path binary) throws IOException {
+    profiler.startTask("optimize");
+    final Path optimized = workingDirectory.resolve("optimized." + PROTO_EXTENSION);
+    logger.fine(
+        new AaptCommandBuilder(aapt2)
+            .forBuildToolsVersion(buildToolsVersion)
+            .forVariantType(VariantType.DEFAULT)
+            .add("optimize")
+            .when(Objects.equals(logger.getLevel(), Level.FINE))
+            .thenAdd("-v")
+            .add("--target-densities", densities.stream().collect(Collectors.joining(",")))
+            .add("-o", optimized)
+            .add(binary.toString())
+            .execute(String.format("Optimizing %s", compiled.getManifest())));
+    profiler.recordEndOf("optimize");
+    return optimized;
+  }
+
+  /** Links compiled resources into an apk */
   public PackagedResources link(CompiledResources compiled) {
     try {
-      final Path outPath =
-          workingDirectory.resolve("bin." + (outputAsProto ? PROTO_EXTENSION : BINARY_EXTENSION));
       Path rTxt = workingDirectory.resolve("R.txt");
       Path proguardConfig = workingDirectory.resolve("proguard.cfg");
       Path mainDexProguard = workingDirectory.resolve("proguard.maindex.cfg");
       Path javaSourceDirectory = Files.createDirectories(workingDirectory.resolve("java"));
       Path resourceIds = workingDirectory.resolve("ids.txt");
-
-      profiler.startTask("fulllink");
-      logger.fine(
-          new AaptCommandBuilder(aapt2)
-              .forBuildToolsVersion(buildToolsVersion)
-              .forVariantType(VariantType.DEFAULT)
-              .add("link")
-              .whenVersionIsAtLeast(new Revision(23))
-              .thenAdd("--no-version-vectors")
-              // Turn off namespaced resources
-              .add("--no-static-lib-packages")
-              .when(Objects.equals(logger.getLevel(), Level.FINE))
-              .thenAdd("-v")
-              .add("--manifest", compiled.getManifest())
-              // Enables resource redefinition and merging
-              .add("--auto-add-overlay")
-              .when(outputAsProto)
-              .thenAdd("--proto-format")
-              .when(debug)
-              .thenAdd("--debug-mode")
-              .add("--custom-package", customPackage)
-              .when(densities.size() == 1)
-              .thenAddRepeated("--preferred-density", densities)
-              .add("--stable-ids", compiled.getStableIds())
-              .addRepeated(
-                  "-A",
-                  Streams.concat(
-                          assetDirs.stream().map(Path::toString),
-                          compiled.getAssetsStrings().stream())
-                      .collect(toList()))
-              .addRepeated("-I", StaticLibrary.toPathStrings(linkAgainst))
-              .addParameterableRepeated(
-                  "-R",
-                  compiledResourcesToPaths(
-                      compiled,
-                      generatePseudoLocale
-                              && resourceConfigs.stream().anyMatch(PSEUDO_LOCALE_FILTERS::contains)
-                          ? IS_FLAT_FILE.and(USE_GENERATED)
-                          : IS_FLAT_FILE.and(USE_DEFAULT)),
-                  workingDirectory)
-              // Never compress apks.
-              .add("-0", "apk")
-              // Add custom no-compress extensions.
-              .addRepeated("-0", uncompressedExtensions)
-              // Filter by resource configuration type.
-              .when(!resourceConfigs.isEmpty())
-              .thenAdd("-c", Joiner.on(',').join(resourceConfigs))
-              .add("--output-text-symbols", rTxt)
-              .add("--emit-ids", resourceIds)
-              .add("--java", javaSourceDirectory)
-              .add("--proguard", proguardConfig)
-              .add("--proguard-main-dex", mainDexProguard)
-              .when(conditionalKeepRules)
-              .thenAdd("--proguard-conditional-keep-rules")
-              .add("-o", outPath)
-              .execute(String.format("Linking %s", compiled.getManifest())));
-      profiler.recordEndOf("fulllink").startTask("attributes");
-
-      final Path attributes = workingDirectory.resolve("tool.attributes");
-      // extract tool annotations from the compile resources.
-      final SdkToolAttributeWriter writer = new SdkToolAttributeWriter(attributes);
-      Stream.concat(include.stream(), Stream.of(compiled))
-          .parallel()
-          .map(AndroidCompiledDataDeserializer.create()::readAttributes)
-          .map(Map::entrySet)
-          .flatMap(Set::stream)
-          .distinct()
-          .forEach(e -> e.getValue().writeResource((FullyQualifiedName) e.getKey(), writer));
-      writer.flush();
-
-      profiler.recordEndOf("attributes");
-      if (densities.size() < 2) {
-        final Path protoApk = convertToProto(outPath);
+      try (ProtoApk protoApk =
+          linkProtoApk(
+              compiled, rTxt, proguardConfig, mainDexProguard, javaSourceDirectory, resourceIds)) {
         return PackagedResources.of(
-            outputAsProto ? protoApk : convertToBinary(outPath), // convert proto to apk
-            protoApk,
+            outputAsProto
+                ? protoApk.asApkPath()
+                : link(protoApk, resourceIds), // convert proto to binary
+            protoApk.asApkPath(),
             rTxt,
             proguardConfig,
             mainDexProguard,
             javaSourceDirectory,
             resourceIds,
-            attributes);
+            extractAttributes(compiled));
       }
 
-      profiler.startTask("optimize");
-      final Path optimized = workingDirectory.resolve("optimized." + OPTIMIZED_EXTENSION);
-      logger.fine(
-          new AaptCommandBuilder(aapt2)
-              .forBuildToolsVersion(buildToolsVersion)
-              .forVariantType(VariantType.DEFAULT)
-              .add("optimize")
-              .when(Objects.equals(logger.getLevel(), Level.FINE))
-              .thenAdd("-v")
-              .add("--target-densities", densities.stream().collect(Collectors.joining(",")))
-              .add("-o", optimized)
-              .add(outPath.toString())
-              .execute(String.format("Optimizing %s", compiled.getManifest())));
-      profiler.recordEndOf("optimize");
-
-      final Path protoApk = convertToProto(optimized);
-      return PackagedResources.of(
-          outputAsProto ? protoApk : convertToBinary(optimized), // convert proto to binary
-          protoApk,
-          rTxt,
-          proguardConfig,
-          mainDexProguard,
-          javaSourceDirectory,
-          resourceIds,
-          attributes);
     } catch (IOException e) {
       throw new LinkError(e);
     }
   }
 
   /** Link a proto apk to produce an apk. */
-  public Path link(ProtoApk protoApk) {
+  public Path link(ProtoApk protoApk, Path resourceIds) {
     try {
       final Path protoApkPath = protoApk.asApkPath();
       final Path working =
@@ -549,12 +566,15 @@ public class ResourceLinker {
               .add("link")
               .when(Objects.equals(logger.getLevel(), Level.FINE))
               .thenAdd("-v")
+              .whenVersionIsAtLeast(new Revision(23))
+              .thenAdd("--no-version-vectors")
+              .add("--stable-ids", resourceIds)
               .add("--manifest", manifest)
               .addRepeated("-I", StaticLibrary.toPathStrings(linkAgainst))
-              .add("-R", convertToBinary(protoApkPath))
+              .add("-R", convertToBinary(protoApk))
               .add("-o", apk.toString())
               .execute(String.format("Re-linking %s", protoApkPath)));
-      return apk;
+      return combineApks(protoApkPath, apk, working);
     } catch (IOException e) {
       throw new LinkError(e);
     }