Skip to content

Commit

Permalink
Only include generated resources when pseudo locales are asked for.
Browse files Browse the repository at this point in the history
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: 196111843
  • Loading branch information
corbinrsmith-work authored and Copybara-Service committed May 10, 2018
1 parent fc58231 commit c53e457
Show file tree
Hide file tree
Showing 5 changed files with 159 additions and 32 deletions.
28 changes: 28 additions & 0 deletions src/test/java/com/google/devtools/build/android/PathsSubject.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
package com.google.devtools.build.android;

import static com.google.common.truth.Truth.assertThat;
import static java.util.stream.Collectors.joining;

import com.google.common.base.Joiner;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.Subject;
import com.google.devtools.build.android.aapt2.ResourceCompiler.CompiledType;
import java.io.IOException;
import java.io.StringReader;
import java.io.StringWriter;
Expand Down Expand Up @@ -45,6 +47,14 @@
/** A testing utility that allows assertions against Paths. */
public class PathsSubject extends Subject<PathsSubject, Path> {

private static final String PATH_NORMALIZER =
String.format(
"(%s)/(.*/)(javatests)",
Arrays.stream(CompiledType.values())
.map(CompiledType::asPrefix)
.map(p -> String.format("(?:%s)", p))
.collect(joining("|")));

PathsSubject(FailureMetadata failureMetadata, @Nullable Path subject) {
super(failureMetadata, subject);
}
Expand All @@ -68,10 +78,27 @@ void containsAllArchivedFilesIn(String... paths) throws IOException {
new ZipFile(actual().toFile())
.stream()
.map(ZipEntry::getName)
.map(n -> n.replaceAll(PATH_NORMALIZER, "$1/$3"))
.collect(Collectors.toSet()))
.containsAllIn(Arrays.asList(paths));
}


void containsExactlyArchivedFilesIn(String... paths) throws IOException {
if (actual() == null) {
fail("should not be null.");
}
exists();

assertThat(
new ZipFile(actual().toFile())
.stream()
.map(ZipEntry::getName)
.map(n -> n.replaceAll(PATH_NORMALIZER, "$1/$3"))
.collect(Collectors.toSet()))
.containsExactly(paths);
}

void containsNoArchivedFilesIn(String... paths) throws IOException {
if (actual() == null) {
fail("should not be null.");
Expand All @@ -81,6 +108,7 @@ void containsNoArchivedFilesIn(String... paths) throws IOException {
new ZipFile(actual().toFile())
.stream()
.map(ZipEntry::getName)
.map(n -> n.replaceAll(PATH_NORMALIZER, "$1/$3"))
.collect(Collectors.toSet()))
.containsNoneIn(Arrays.asList(paths));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ private void readPackages(KeyValueConsumers consumers, ResourceTable resourceTab
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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
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;
Expand Down Expand Up @@ -87,6 +89,12 @@ protected long normalizeTime(String filename) {
}

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.
Expand All @@ -99,15 +107,18 @@ protected void addEntry(String rawName, byte[] content, int storageMethod) throw
CRC32 crc32 = new CRC32();
crc32.update(content);
entry.setCrc(crc32.getValue());
if (!Strings.isNullOrEmpty(comment)) {
entry.setComment(comment);
}

zip.putNextEntry(entry);
zip.write(content);
zip.closeEntry();
}

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);
Expand Down Expand Up @@ -412,6 +423,7 @@ public static Path archiveCompiledResources(
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);
Expand All @@ -424,7 +436,11 @@ public static Path archiveCompiledResources(
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
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;
Expand All @@ -60,6 +61,43 @@

/** 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) {
Expand Down Expand Up @@ -105,19 +143,28 @@ private CompileTask(
@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;
}
Expand Down Expand Up @@ -362,17 +409,33 @@ List<Path> getCompiledArtifacts() {
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);
}
Expand All @@ -393,7 +456,7 @@ public static ResourceCompiler create(
aapt2,
buildToolsVersion,
generatePseudoLocale
? Optional.of(compiledResources.resolve("generated"))
? Optional.of(compiledResources.resolve(CompiledType.GENERATED.asPrefix()))
: Optional.empty()));
}

Expand Down
Loading

0 comments on commit c53e457

Please sign in to comment.