Automated rollback of commit feeccd8c0a5d97493cabfeb9481cf6f3800b9b84.

*** Reason for rollback ***

Rolling forward with correct handling for pseudo locale generation flags

*** Original change description ***

Only include generated resources when pseudo locales are asked for.

CompileResources: generate pseudo locales into a separate compiled resource file when the pseudo locale flag is true and the locale is default (e.g. absent). The generated resources must be first in the returned list of compiled resource paths. This allows the user to define custom values to the pseudo locales (which, for some obscure reason, is accepted as a reasonable practice by aapt{,2})
AndroidResourceOutputs: record the type of compiled resource in the comment field of the zip entry for fast comparison.
ResourceLinker: only include the generated resources when the pseudo locale is explicitly asked for. It's important to note that the ordering for compiled resources in the zip goes <generated>...<normal>...<default>. This means the default locale will overwrite the generated locale values. Annoying, but necessary, as that is current order before introducing this cl.

RELNOTES:None
PiperOrigin-RevId: 196159094

5 files changed, 140 insertions, 32 deletions

diff --git a/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java b/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java
index 025533ee6a..b903ebec66 100644
--- a/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java
+++ b/src/tools/android/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java
@@ -175,6 +175,7 @@ public class Aapt2ResourcePackagingAction {
               .conditionalKeepRules(aaptConfigOptions.conditionalKeepRules == TriState.YES)
               .filterToDensity(options.densities)
               .debug(aaptConfigOptions.debug)
+              .includeGeneratedLocales(aaptConfigOptions.generatePseudoLocale)
               .includeOnlyConfigs(aaptConfigOptions.resourceConfigs)
               .link(compiled)
               .copyPackageTo(options.packagePath)
diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidCompiledDataDeserializer.java b/src/tools/android/java/com/google/devtools/build/android/AndroidCompiledDataDeserializer.java
index 8cc58853c2..8c24031209 100644
--- a/src/tools/android/java/com/google/devtools/build/android/AndroidCompiledDataDeserializer.java
+++ b/src/tools/android/java/com/google/devtools/build/android/AndroidCompiledDataDeserializer.java
@@ -302,7 +302,7 @@ public class AndroidCompiledDataDeserializer implements AndroidDataDeserializer
               String source = sourcePool.get(sourceIndex);
               DataSource dataSource = DataSource.of(Paths.get(source));
 
-              Value resourceValue = resource.getConfigValue(0).getValue();
+              Value resourceValue = configValue.getValue();
 
               DataResource dataResource =
                   resourceValue.getItem().hasFile()
diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceOutputs.java b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceOutputs.java
index 41d942736f..cc19be8f0c 100644
--- a/src/tools/android/java/com/google/devtools/build/android/AndroidResourceOutputs.java
+++ b/src/tools/android/java/com/google/devtools/build/android/AndroidResourceOutputs.java
@@ -18,7 +18,9 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
 import com.google.common.collect.Ordering;
+import com.google.devtools.build.android.aapt2.ResourceCompiler;
 import java.io.BufferedOutputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.Closeable;
@@ -87,6 +89,12 @@ public class AndroidResourceOutputs {
     }
 
     protected void addEntry(String rawName, byte[] content, int storageMethod) throws IOException {
+      addEntry(rawName, content, storageMethod, null);
+    }
+
+    protected void addEntry(
+        String rawName, byte[] content, int storageMethod, @Nullable String comment)
+        throws IOException {
       // Fix the path for windows.
       String relativeName = rawName.replace('\\', '/');
       // Make sure the zip entry is not absolute.
@@ -99,6 +107,9 @@ public class AndroidResourceOutputs {
       CRC32 crc32 = new CRC32();
       crc32.update(content);
       entry.setCrc(crc32.getValue());
+      if (!Strings.isNullOrEmpty(comment)) {
+        entry.setComment(comment);
+      }
 
       zip.putNextEntry(entry);
       zip.write(content);
@@ -106,8 +117,8 @@ public class AndroidResourceOutputs {
     }
 
     protected void addEntry(ZipEntry entry, byte[] content) throws IOException {
-      //Create a new ZipEntry because there are occasional discrepancies
-      //between the metadata and written content.
+      // Create a new ZipEntry because there are occasional discrepancies
+      // between the metadata and written content.
       ZipEntry newEntry = new ZipEntry(entry.getName());
       zip.putNextEntry(newEntry);
       zip.write(content);
@@ -412,6 +423,7 @@ public class AndroidResourceOutputs {
     try (ZipBuilder builder = ZipBuilder.createFor(archiveOut)) {
       for (Path artifact : compiledArtifacts) {
         Path relativeName = artifact;
+
         // remove compiled resources prefix
         if (artifact.startsWith(compiledRoot)) {
           relativeName = compiledRoot.relativize(relativeName);
@@ -424,7 +436,11 @@ public class AndroidResourceOutputs {
                   relativeName.getNameCount());
         }
 
-        builder.addEntry(relativeName.toString(), Files.readAllBytes(artifact), ZipEntry.STORED);
+        builder.addEntry(
+            relativeName.toString(),
+            Files.readAllBytes(artifact),
+            ZipEntry.STORED,
+            ResourceCompiler.getCompiledType(relativeName.toString()).asComment());
       }
     }
     return archiveOut;
diff --git a/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceCompiler.java b/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceCompiler.java
index 68e12e146e..57cbcb533b 100644
--- a/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceCompiler.java
+++ b/src/tools/android/java/com/google/devtools/build/android/aapt2/ResourceCompiler.java
@@ -40,6 +40,7 @@ import java.nio.file.Path;
 import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -60,6 +61,43 @@ import javax.xml.stream.events.XMLEvent;
 
 /** Invokes aapt2 to compile resources. */
 public class ResourceCompiler {
+
+  /** Types of compiled resources. */
+  public enum CompiledType {
+    NORMAL(null),
+    GENERATED("generated"),
+    DEFAULT("default");
+
+    private final String prefix;
+
+    CompiledType(String prefix) {
+      this.prefix = prefix;
+    }
+
+    boolean prefixes(String filename) {
+      return prefix != null && filename.startsWith(prefix);
+    }
+
+    public String asPrefix() {
+      return prefix;
+    }
+
+    public String asComment() {
+      return prefix;
+    }
+
+    public String prefix(String path) {
+      return prefix + "/" + path;
+    }
+  }
+
+  public static CompiledType getCompiledType(String fileName) {
+    return Arrays.stream(CompiledType.values())
+        .filter(t -> t.prefixes(fileName))
+        .findFirst()
+        .orElse(CompiledType.NORMAL);
+  }
+
   static class CompileError extends Aapt2Exception {
 
     protected CompileError(Throwable e) {
@@ -105,19 +143,28 @@ public class ResourceCompiler {
     @Override
     public List<Path> call() throws Exception {
       final String directoryName = file.getParent().getFileName().toString();
-      Qualifiers qualifiers = Qualifiers.parseFrom(directoryName);
-      String filename = interpolateAapt2Filename(qualifiers, file.getFileName().toString());
+      final Qualifiers qualifiers = Qualifiers.parseFrom(directoryName);
+      final String filename = interpolateAapt2Filename(qualifiers, file.getFileName().toString());
 
-      List<Path> results = new ArrayList<>();
-      compile(directoryName, filename, results, compiledResourcesOut, file, false);
+      final List<Path> results = new ArrayList<>();
       if (qualifiers.asFolderType().equals(ResourceFolderType.VALUES)) {
         extractAttributes(directoryName, filename, results);
+      }
 
-        if (qualifiers.containDefaultLocale()) {
-          // aapt2 only generates pseudo locales for the default locale.
-          generatedResourcesOut.ifPresent(
-              out -> compile(directoryName, filename, results, out, file, true));
-        }
+      if (qualifiers.containDefaultLocale()
+          && qualifiers.asFolderType().equals(ResourceFolderType.VALUES)) {
+        compile(
+            directoryName,
+            filename,
+            results,
+            compiledResourcesOut.resolve(CompiledType.DEFAULT.asPrefix()),
+            file,
+            false);
+        // aapt2 only generates pseudo locales for the default locale.
+        generatedResourcesOut.ifPresent(
+            out -> compile(directoryName, filename, results, out, file, true));
+      } else {
+        compile(directoryName, filename, results, compiledResourcesOut, file, false);
       }
       return results;
     }
@@ -362,17 +409,33 @@ public class ResourceCompiler {
                     generatedResourcesOut)));
       }
 
-      ImmutableList.Builder<Path> builder = ImmutableList.builder();
+      ImmutableList.Builder<Path> compiled = ImmutableList.builder();
+      ImmutableList.Builder<Path> generated = ImmutableList.builder();
       List<Throwable> compilationErrors = new ArrayList<>();
       for (ListenableFuture<List<Path>> task : tasks) {
         try {
-          builder.addAll(task.get());
+          // Split the generated and non-generated resources into different collections.
+          // This allows the generated files to be placed first in the compile order,
+          // ensuring that the generated locale (en-XA and ar-XB) can be overwritten by
+          // user provided versions for those locales, as aapt2 will take the last value for
+          // a configuration when linking.
+          task.get()
+              .forEach(
+                  path -> {
+                    if (generatedResourcesOut.map(path::startsWith).orElse(false)) {
+                      generated.add(path);
+                    } else {
+                      compiled.add(path);
+                    }
+                  });
         } catch (InterruptedException | ExecutionException e) {
-          compilationErrors.add(Optional.ofNullable(e.getCause()).orElse(e));
+          compilationErrors.add(e.getCause() != null ? e.getCause() : e);
         }
       }
+      generated.addAll(compiled.build());
       if (compilationErrors.isEmpty()) {
-        return builder.build();
+        // ensure that the generated files are before the normal files.
+        return generated.build();
       }
       throw CompileError.of(compilationErrors);
     }
@@ -393,7 +456,7 @@ public class ResourceCompiler {
             aapt2,
             buildToolsVersion,
             generatePseudoLocale
-                ? Optional.of(compiledResources.resolve("generated"))
+                ? Optional.of(compiledResources.resolve(CompiledType.GENERATED.asPrefix()))
                 : Optional.empty()));
   }
 
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 c5c3236043..c758d2dbf9 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
@@ -20,12 +20,16 @@ import com.android.repository.Revision;
 import com.google.common.base.Joiner;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Streams;
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.devtools.build.android.AaptCommandBuilder;
 import com.google.devtools.build.android.AndroidResourceOutputs;
 import com.google.devtools.build.android.Profiler;
+import com.google.devtools.build.android.aapt2.ResourceCompiler.CompiledType;
+import com.google.devtools.build.android.ziputils.DirectoryEntry;
 import com.google.devtools.build.android.ziputils.ZipIn;
 import com.google.devtools.build.android.ziputils.ZipOut;
 import java.io.IOException;
@@ -49,7 +53,24 @@ import java.util.stream.Stream;
 /** Performs linking of {@link CompiledResources} using aapt2. */
 public class ResourceLinker {
 
+  private static final Predicate<String> IS_JAR = s -> s.endsWith(".jar");
   private boolean debug;
+  private static final Predicate<DirectoryEntry> IS_FLAT_FILE =
+      h -> h.getFilename().endsWith(".flat");
+
+  private static final Predicate<DirectoryEntry> COMMENT_ABSENT =
+      h -> Strings.isNullOrEmpty(h.getComment());
+
+  private static final Predicate<DirectoryEntry> USE_GENERATED =
+      COMMENT_ABSENT.or(
+          h -> ResourceCompiler.getCompiledType(h.getFilename()) == CompiledType.GENERATED);
+
+  private static final Predicate<DirectoryEntry> USE_DEFAULT =
+      COMMENT_ABSENT.or(
+          h -> ResourceCompiler.getCompiledType(h.getComment()) != CompiledType.GENERATED);
+
+  private static final ImmutableSet<String> PSEUDO_LOCALE_FILTERS =
+      ImmutableSet.of("en_XA", "ar_XB");
 
   /** Represents errors thrown during linking. */
   public static class LinkError extends Aapt2Exception {
@@ -63,6 +84,8 @@ public class ResourceLinker {
     }
   }
 
+  private boolean generatePseudoLocale;
+
   private static Logger logger = Logger.getLogger(ResourceLinker.class.getName());
 
   private final Path aapt2;
@@ -99,6 +122,11 @@ public class ResourceLinker {
     return new ResourceLinker(aapt2, executorService, workingDirectory);
   }
 
+  public ResourceLinker includeGeneratedLocales(boolean generatePseudoLocale) {
+    this.generatePseudoLocale = generatePseudoLocale;
+    return this;
+  }
+
   public ResourceLinker profileUsing(Profiler profiler) {
     this.profiler = profiler;
     return this;
@@ -184,9 +212,7 @@ public class ResourceLinker {
               .when(outputAsProto)
               .thenAdd("--proto-format")
               .addParameterableRepeated(
-                  "-R",
-                  compiledResourcesToPaths(compiled, s -> s.endsWith(".flat")),
-                  workingDirectory)
+                  "-R", compiledResourcesToPaths(compiled, IS_FLAT_FILE), workingDirectory)
               .addRepeated("-I", pathsToLinkAgainst)
               .add("--auto-add-overlay")
               .add("-o", outPath)
@@ -211,14 +237,11 @@ public class ResourceLinker {
                 .when(outputAsProto)
                 .thenAdd("--proto-format")
                 // only link against jars
-                .addRepeated(
-                    "-I",
-                    pathsToLinkAgainst.stream().filter(s -> s.endsWith(".jar")).collect(toList()))
+                .addRepeated("-I", pathsToLinkAgainst.stream().filter(IS_JAR).collect(toList()))
                 .add("-R", outPath)
                 // only include non-jars
                 .addRepeated(
-                    "-R",
-                    pathsToLinkAgainst.stream().filter(s -> !s.endsWith(".jar")).collect(toList()))
+                    "-R", pathsToLinkAgainst.stream().filter(IS_JAR.negate()).collect(toList()))
                 .add("--auto-add-overlay")
                 .add("-o", outPath.resolveSibling("transitive.apk"))
                 .add("--java", javaSourceDirectory)
@@ -237,7 +260,7 @@ public class ResourceLinker {
   }
 
   private List<String> compiledResourcesToPaths(
-      CompiledResources compiled, Predicate<String> shouldKeep) throws IOException {
+      CompiledResources compiled, Predicate<DirectoryEntry> shouldKeep) throws IOException {
     // Using sequential streams to maintain the overlay order for aapt2.
     return Stream.concat(include.stream(), Stream.of(compiled))
         .sequential()
@@ -249,7 +272,7 @@ public class ResourceLinker {
         .collect(toList());
   }
 
-  private Path filterZip(Path path, Predicate<String> shouldKeep) throws IOException {
+  private Path filterZip(Path path, Predicate<DirectoryEntry> shouldKeep) throws IOException {
     Path outPath =
         workingDirectory
             .resolve("filtered")
@@ -267,7 +290,7 @@ public class ResourceLinker {
       final ZipOut zipOut = new ZipOut(outChannel, outPath.toString());
       zipIn.scanEntries(
           (in, header, dirEntry, data) -> {
-            if (shouldKeep.test(header.getFilename())) {
+            if (shouldKeep.test(dirEntry)) {
               zipOut.nextEntry(dirEntry);
               zipOut.write(header);
               zipOut.write(data);
@@ -307,7 +330,7 @@ public class ResourceLinker {
       Path resourceIds = workingDirectory.resolve("ids.txt");
 
       profiler.startTask("fulllink");
-      logger.finer(
+      logger.fine(
           new AaptCommandBuilder(aapt2)
               .forBuildToolsVersion(buildToolsVersion)
               .forVariantType(VariantType.DEFAULT)
@@ -338,7 +361,12 @@ public class ResourceLinker {
               .addRepeated("-I", StaticLibrary.toPathStrings(linkAgainst))
               .addParameterableRepeated(
                   "-R",
-                  compiledResourcesToPaths(compiled, s -> s.endsWith(".flat")),
+                  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")
@@ -363,7 +391,7 @@ public class ResourceLinker {
             outPath, rTxt, proguardConfig, mainDexProguard, javaSourceDirectory, resourceIds);
       }
       final Path optimized = workingDirectory.resolve("optimized.apk");
-      logger.finer(
+      logger.fine(
           new AaptCommandBuilder(aapt2)
               .forBuildToolsVersion(buildToolsVersion)
               .forVariantType(VariantType.DEFAULT)