Optimize PackageAndroidResources action latency: - Cache configuration qualifiers in AndroidCompiledDataDeserializer and fast-path default configurations, eliminating expensive redundant FolderConfiguration allocations and stream mappings. - Pre-compute source pool normalized directory arrays and replace stream pipelines in findImpliedPrivateResources with indexed lookups. - Hoist ResourceName and visibility lookups outside ConfigValue loops in consumeResourceTable. - Parse Android manifests concurrently in ResourceLinker.extractPackages. PiperOrigin-RevId: 975774813 Change-Id: I3401a87061761e69cb5d14a56e761b0a680b4763
diff --git a/src/tools/java/com/google/devtools/build/android/AndroidCompiledDataDeserializer.java b/src/tools/java/com/google/devtools/build/android/AndroidCompiledDataDeserializer.java index 2186884..a71d7bf 100644 --- a/src/tools/java/com/google/devtools/build/android/AndroidCompiledDataDeserializer.java +++ b/src/tools/java/com/google/devtools/build/android/AndroidCompiledDataDeserializer.java
@@ -14,8 +14,8 @@ package com.google.devtools.build.android; import static com.google.common.base.Predicates.not; +import static com.google.common.base.Strings.isNullOrEmpty; import static com.google.common.base.Verify.verify; -import static java.util.stream.Collectors.toList; import android.aapt.pb.internal.ResourcesInternal.CompiledFile; import com.android.aapt.ConfigurationOuterClass.Configuration; @@ -73,6 +73,9 @@ import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Stopwatch; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -87,20 +90,20 @@ import com.google.devtools.build.android.resources.ResourceTypeEnum; import com.google.devtools.build.android.resources.Visibility; import com.google.devtools.build.android.xml.ResourcesAttribute.AttributeType; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.protobuf.ExtensionRegistry; import com.google.protobuf.InvalidProtocolBufferException; import java.io.IOException; import java.io.InputStream; -import java.io.UnsupportedEncodingException; import java.nio.ByteBuffer; import java.nio.ByteOrder; +import java.nio.charset.StandardCharsets; import java.nio.file.FileSystem; import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; -import java.util.Arrays; import java.util.Enumeration; import java.util.HashSet; import java.util.LinkedHashMap; @@ -263,14 +266,32 @@ this.includeFileContentsForValidation = includeFileContentsForValidation; } + // 10,000 entries accommodates all distinct configuration qualifier permutations across + // dependency resource tables in persistent worker processes (typically hundreds to low + // thousands for large apps) while bounding cache memory overhead to ~a few hundred KB. + private static final LoadingCache<Configuration, ImmutableList<String>> QUALIFIER_CACHE = + CacheBuilder.newBuilder() + .maximumSize(10_000) + .build( + new CacheLoader<Configuration, ImmutableList<String>>() { + @Override + public ImmutableList<String> load(Configuration protoConfig) { + return computeQualifiers(protoConfig); + } + }); + private static void consumeResourceTable( DependencyInfo dependencyInfo, KeyValueConsumers consumers, ResourceTable resourceTable, VisibilityRegistry registry) - throws UnsupportedEncodingException, InvalidProtocolBufferException { - List<String> sourcePool = + throws InvalidProtocolBufferException { + ImmutableList<String> sourcePool = decodeSourcePool(resourceTable.getSourcePool().getData().toByteArray()); + Path[] sourcePoolPaths = new Path[sourcePool.size()]; + for (int i = 0; i < sourcePool.size(); i++) { + sourcePoolPaths[i] = Path.of(sourcePool.get(i)); + } ReferenceResolver resolver = ReferenceResolver.asRoot(); for (Package resourceTablePackage : resourceTable.getPackageList()) { @@ -282,8 +303,12 @@ ResourceType resourceType = ResourceTypeEnum.get(resourceFormatType.getName()); for (Resources.Entry resource : resourceFormatType.getEntryList()) { - if (!"android".equals(packageName)) { + if (!packageName.equals("android")) { // This means this resource is not in the android sdk, add it to the set. + ResourceName resourceName = + ResourceName.create(packageName, resourceType, resource.getName()); + Visibility visibility = registry.getVisibility(resourceName); + for (ConfigValue configValue : resource.getConfigValueList()) { FullyQualifiedName fqn = createAndRecordFqn( @@ -292,13 +317,10 @@ resourceType, resource, convertToQualifiers(configValue.getConfig())); - Visibility visibility = - registry.getVisibility( - ResourceName.create(packageName, resourceType, resource.getName())); int sourceIndex = configValue.getValue().getSource().getPathIdx(); - String source = sourcePool.get(sourceIndex); - DataSource dataSource = DataSource.of(dependencyInfo, Paths.get(source)); + Path sourcePath = sourcePoolPaths[sourceIndex]; + DataSource dataSource = DataSource.of(dependencyInfo, sourcePath); Value resourceValue = configValue.getValue(); DataResource dataResource = @@ -359,6 +381,7 @@ return FullyQualifiedName.fromReference(reference, packageName); } + @CanIgnoreReturnValue public FullyQualifiedName register(FullyQualifiedName fullyQualifiedName) { // The default is that the name can be inlined. qualifiedReferenceInlineStatus.put(fullyQualifiedName, InlineStatus.INLINEABLE); @@ -372,10 +395,11 @@ return false; } - return InlineStatus.INLINEABLE.equals(qualifiedReferenceInlineStatus.get(reference)); + return qualifiedReferenceInlineStatus.get(reference) == InlineStatus.INLINEABLE; } /** Update the reference's inline state. */ + @CanIgnoreReturnValue public FullyQualifiedName markInlined(FullyQualifiedName reference) { qualifiedReferenceInlineStatus.put(reference, InlineStatus.INLINED); return reference; @@ -399,7 +423,11 @@ } // TODO(b/146498565): remove this and use 'Configuration' directly, which is typesafe and free. - private static List<String> convertToQualifiers(Configuration protoConfig) { + static ImmutableList<String> convertToQualifiers(Configuration protoConfig) { + return QUALIFIER_CACHE.getUnchecked(protoConfig); + } + + private static ImmutableList<String> computeQualifiers(Configuration protoConfig) { FolderConfiguration configuration = new FolderConfiguration(); if (protoConfig.getMcc() > 0) { configuration.setCountryCodeQualifier(new CountryCodeQualifier(protoConfig.getMcc())); @@ -517,9 +545,14 @@ } - return Arrays.stream(configuration.getQualifiers()) - .map(ResourceQualifier::getFolderSegment) - .collect(toList()); + ResourceQualifier[] qualifiers = configuration.getQualifiers(); + ImmutableList.Builder<String> result = ImmutableList.builderWithExpectedSize(qualifiers.length); + for (ResourceQualifier qualifier : qualifiers) { + if (qualifier != null) { + result.add(qualifier.getFolderSegment()); + } + } + return result.build(); } /** @@ -709,8 +742,8 @@ private static ResourceContainer readResourceContainer( ZipFile zipFile, boolean includeFileContentsForValidation) throws IOException { - List<ResourceTable> resourceTables = new ArrayList<>(); - List<CompiledFileWithData> compiledFiles = new ArrayList<>(); + ImmutableList.Builder<ResourceTable> resourceTables = ImmutableList.builder(); + ImmutableList.Builder<CompiledFileWithData> compiledFiles = ImmutableList.builder(); Enumeration<? extends ZipEntry> resourceFiles = zipFile.entries(); while (resourceFiles.hasMoreElements()) { @@ -784,7 +817,7 @@ } } } - return ResourceContainer.create(resourceTables, compiledFiles); + return ResourceContainer.create(resourceTables.build(), compiledFiles.build()); } /** Rounds {@code n} up to the nearest multiple of {@code k}. */ @@ -801,31 +834,39 @@ * resource_files} (b/148110689), we perform the classification on a per-directory basis, so that * marking something in {@code foo/res} public has no impact on {@code bar/res}. */ - private static VisibilityRegistry computeResourceVisibility(ResourceContainer resourceContainer) - throws UnsupportedEncodingException { + private static VisibilityRegistry computeResourceVisibility(ResourceContainer resourceContainer) { if (!ResourceCompiler.USE_VISIBILITY_FROM_AAPT2) { return new VisibilityRegistry(ImmutableSet.of(), ImmutableSet.of()); } // decode source pools ahead of time to avoid having to repeatedly do so later. - List<List<String>> sourcePools = new ArrayList<>(); + ImmutableList.Builder<Path[]> normalizedSourcePoolsBuilder = + ImmutableList.builderWithExpectedSize(resourceContainer.resourceTables().size()); for (ResourceTable resourceTable : resourceContainer.resourceTables()) { - sourcePools.add(decodeSourcePool(resourceTable.getSourcePool().getData().toByteArray())); + ImmutableList<String> sourcePool = + decodeSourcePool(resourceTable.getSourcePool().getData().toByteArray()); + Path[] normalizedDirs = new Path[sourcePool.size()]; + for (int j = 0; j < sourcePool.size(); j++) { + normalizedDirs[j] = getNormalizedResourceDirectory(sourcePool.get(j)); + } + normalizedSourcePoolsBuilder.add(normalizedDirs); } + ImmutableList<Path[]> normalizedSourcePools = normalizedSourcePoolsBuilder.build(); - PublicResources publicResources = findExplicitlyPublicResources(resourceContainer, sourcePools); + PublicResources publicResources = + findExplicitlyPublicResources(resourceContainer, normalizedSourcePools); return new VisibilityRegistry( publicResources.explicitlyPublicResources(), - findImpliedPrivateResources(resourceContainer, sourcePools, publicResources)); + findImpliedPrivateResources(resourceContainer, normalizedSourcePools, publicResources)); } private static PublicResources findExplicitlyPublicResources( - ResourceContainer resourceContainer, List<List<String>> sourcePools) { + ResourceContainer resourceContainer, ImmutableList<Path[]> normalizedSourcePools) { Set<ResourceName> explicitlyPublicResources = new HashSet<>(); Set<Path> directoriesWithPublicResources = new HashSet<>(); for (int i = 0; i < resourceContainer.resourceTables().size(); i++) { ResourceTable resourceTable = resourceContainer.resourceTables().get(i); - List<String> sourcePool = sourcePools.get(i); + Path[] normalizedSourcePool = normalizedSourcePools.get(i); for (Package pkg : resourceTable.getPackageList()) { for (Resources.Type type : pkg.getTypeList()) { @@ -837,9 +878,11 @@ explicitlyPublicResources.add( ResourceName.create(pkg.getPackageName(), resourceType, entry.getName())); - directoriesWithPublicResources.add( - getNormalizedResourceDirectory( - sourcePool.get(entry.getVisibility().getSource().getPathIdx()))); + int pathIdx = entry.getVisibility().getSource().getPathIdx(); + Path dir = normalizedSourcePool[pathIdx]; + if (dir != null) { + directoriesWithPublicResources.add(dir); + } } } } @@ -849,14 +892,16 @@ private static ImmutableSet<ResourceName> findImpliedPrivateResources( ResourceContainer resourceContainer, - List<List<String>> sourcePools, + ImmutableList<Path[]> normalizedSourcePools, PublicResources publicResources) { - Set<ResourceName> explicitlyPublicResources = publicResources.explicitlyPublicResources(); - Set<Path> directoriesWithPublicResources = publicResources.directoriesWithPublicResources(); + ImmutableSet<ResourceName> explicitlyPublicResources = + publicResources.explicitlyPublicResources(); + ImmutableSet<Path> directoriesWithPublicResources = + publicResources.directoriesWithPublicResources(); Set<ResourceName> impliedPrivateResources = new HashSet<>(); for (int i = 0; i < resourceContainer.resourceTables().size(); i++) { ResourceTable resourceTable = resourceContainer.resourceTables().get(i); - List<String> sourcePool = sourcePools.get(i); + Path[] normalizedSourcePool = normalizedSourcePools.get(i); for (Package pkg : resourceTable.getPackageList()) { for (Resources.Type type : pkg.getTypeList()) { @@ -873,13 +918,15 @@ continue; // we already figured out a classification for this resource. } - boolean inDirectoryWithPublic = - entry.getConfigValueList().stream() - .map( - configValue -> - getNormalizedResourceDirectory( - sourcePool.get(configValue.getValue().getSource().getPathIdx()))) - .anyMatch(directoriesWithPublicResources::contains); + boolean inDirectoryWithPublic = false; + for (ConfigValue configValue : entry.getConfigValueList()) { + int pathIdx = configValue.getValue().getSource().getPathIdx(); + Path dir = normalizedSourcePool[pathIdx]; + if (dir != null && directoriesWithPublicResources.contains(dir)) { + inDirectoryWithPublic = true; + break; + } + } if (inDirectoryWithPublic) { impliedPrivateResources.add(resourceName); } @@ -894,8 +941,8 @@ || impliedPrivateResources.contains(resourceName)) { continue; // we already figured out a classification for this resource. } - if (directoriesWithPublicResources.contains( - getNormalizedResourceDirectory(compiledFile.getSourcePath()))) { + Path dir = getNormalizedResourceDirectory(compiledFile.getSourcePath()); + if (dir != null && directoriesWithPublicResources.contains(dir)) { impliedPrivateResources.add(resourceName); } } @@ -906,14 +953,32 @@ * Returns the resource directory (e.g. {@code java/com/pkg/res/}) which contains a file, * stripping off any {@code blaze-*} prefix for normalization. */ - private static Path getNormalizedResourceDirectory(String filename) { - Path resDir = Paths.get(filename).getParent().getParent(); - if (resDir.getName(0).toString().startsWith("blaze-")) { - // strip off stuff like blaze-out/k8-fastbuild/bin/. - return resDir.subpath(3, resDir.getNameCount()); - } else { - return resDir; + @Nullable + static Path getNormalizedResourceDirectory(@Nullable Path file) { + if (file == null) { + return null; } + Path parent = file.getParent(); + if (parent == null) { + return null; + } + Path grandParent = parent.getParent(); + if (grandParent == null) { + return null; + } + if (grandParent.getNameCount() > 3 && grandParent.getName(0).toString().startsWith("blaze-")) { + return grandParent.subpath(3, grandParent.getNameCount()); + } else { + return grandParent; + } + } + + @Nullable + static Path getNormalizedResourceDirectory(@Nullable String filename) { + if (isNullOrEmpty(filename)) { + return null; + } + return getNormalizedResourceDirectory(Path.of(filename)); } private static byte[] readBytesAndSkipPadding(LittleEndianDataInputStream input, int size) @@ -928,7 +993,7 @@ return result; } - private static List<String> decodeSourcePool(byte[] bytes) throws UnsupportedEncodingException { + private static ImmutableList<String> decodeSourcePool(byte[] bytes) { ByteBuffer byteBuffer = ByteBuffer.wrap(bytes).order(ByteOrder.LITTLE_ENDIAN); int stringCount = byteBuffer.getInt(8); @@ -937,7 +1002,7 @@ // Position the ByteBuffer after the metadata byteBuffer.position(28); - List<String> strings = new ArrayList<>(); + ImmutableList.Builder<String> strings = ImmutableList.builderWithExpectedSize(stringCount); for (int i = 0; i < stringCount; i++) { int stringOffset = stringsStart + byteBuffer.getInt(); @@ -958,7 +1023,7 @@ stringOffset += (length >= 0x80 ? 2 : 1); - strings.add(new String(bytes, stringOffset, length, "UTF8")); + strings.add(new String(bytes, stringOffset, length, StandardCharsets.UTF_8)); } else { // TODO(b/148817379): this next block of lines is forming an int with holes in it. int characterCount = byteBuffer.get(stringOffset) & 0xFFFF; @@ -976,11 +1041,11 @@ stringOffset += 2 * (length >= 0x8000 ? 2 : 1); - strings.add(new String(bytes, stringOffset, length, "UTF16")); + strings.add(new String(bytes, stringOffset, length, StandardCharsets.UTF_16LE)); } } - return strings; + return strings.build(); } @AutoValue @@ -990,9 +1055,10 @@ abstract ImmutableList<CompiledFileWithData> compiledFiles(); static ResourceContainer create( - List<ResourceTable> resourceTables, List<CompiledFileWithData> compiledFiles) { + ImmutableList<ResourceTable> resourceTables, + ImmutableList<CompiledFileWithData> compiledFiles) { return new AutoValue_AndroidCompiledDataDeserializer_ResourceContainer( - ImmutableList.copyOf(resourceTables), ImmutableList.copyOf(compiledFiles)); + resourceTables, compiledFiles); } } @@ -1032,8 +1098,8 @@ } private static class VisibilityRegistry { - private final Set<ResourceName> explicitlyPublicResources; - private final Set<ResourceName> impliedPrivateResources; + private final ImmutableSet<ResourceName> explicitlyPublicResources; + private final ImmutableSet<ResourceName> impliedPrivateResources; VisibilityRegistry( Set<ResourceName> explicitlyPublicResources, Set<ResourceName> impliedPrivateResources) {
diff --git a/src/tools/java/com/google/devtools/build/android/aapt2/ResourceLinker.java b/src/tools/java/com/google/devtools/build/android/aapt2/ResourceLinker.java index c4dea57..dd300e3 100644 --- a/src/tools/java/com/google/devtools/build/android/aapt2/ResourceLinker.java +++ b/src/tools/java/com/google/devtools/build/android/aapt2/ResourceLinker.java
@@ -33,6 +33,7 @@ import com.android.builder.core.DefaultManifestParser; import com.android.builder.core.VariantTypeImpl; import com.android.repository.Revision; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; @@ -44,6 +45,7 @@ import com.google.common.collect.Streams; import com.google.common.io.ByteSource; import com.google.common.io.ByteStreams; +import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.devtools.build.android.AaptCommandBuilder; import com.google.devtools.build.android.AndroidCompiledDataDeserializer; @@ -72,6 +74,7 @@ import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.util.Collection; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -641,19 +644,49 @@ } } - private Path extractPackages(CompiledResources compiled) throws IOException { + @VisibleForTesting + Path extractPackages(CompiledResources compiled) throws IOException { + profiler.startTask("packages"); Path packages = workingDirectory.resolve("packages"); - try (BufferedWriter writer = Files.newBufferedWriter(packages, StandardOpenOption.CREATE_NEW)) { - for (CompiledResources resources : FluentIterable.from(include).append(compiled)) { - writer.append( - new DefaultManifestParser( - resources.getManifest().toFile(), - /* canParseManifest= */ () -> true, - /* isManifestFileRequired= */ true, - /* issueReporter= */ null) - .getPackage()); - writer.newLine(); + ImmutableList<CompiledResources> allResources = + ImmutableList.<CompiledResources>builderWithExpectedSize(include.size() + 1) + .addAll(include) + .add(compiled) + .build(); + + ImmutableList.Builder<ListenableFuture<String>> packageFuturesBuilder = + ImmutableList.builderWithExpectedSize(allResources.size()); + Map<Path, ListenableFuture<String>> manifestCache = new HashMap<>(); + for (CompiledResources resources : allResources) { + Path manifest = resources.getManifest(); + ListenableFuture<String> future = manifestCache.get(manifest); + if (future == null) { + future = + executorService.submit( + () -> + new DefaultManifestParser( + manifest.toFile(), + /* canParseManifest= */ () -> true, + /* isManifestFileRequired= */ true, + /* issueReporter= */ null) + .getPackage()); + manifestCache.put(manifest, future); } + packageFuturesBuilder.add(future); + } + ImmutableList<ListenableFuture<String>> packageFutures = packageFuturesBuilder.build(); + + try (BufferedWriter writer = Files.newBufferedWriter(packages, StandardOpenOption.CREATE_NEW)) { + for (ListenableFuture<String> future : packageFutures) { + try { + writer.append(future.get()); + writer.newLine(); + } catch (Exception e) { + throw new RuntimeException(e); + } + } + } finally { + profiler.recordEndOf("packages"); } return packages; } @@ -678,7 +711,8 @@ } Preconditions.checkState( - !optimizeThroughput, "Invalid state: calling optimize() when optimizeThroughput is enabled"); + !optimizeThroughput, + "Invalid state: calling optimize() when optimizeThroughput is enabled"); profiler.startTask("optimize"); final Path optimized =
diff --git a/src/tools/javatests/com/google/devtools/build/android/AndroidDataSerializerAndDeserializerTest.java b/src/tools/javatests/com/google/devtools/build/android/AndroidDataSerializerAndDeserializerTest.java index 94529e3..7502d9c 100644 --- a/src/tools/javatests/com/google/devtools/build/android/AndroidDataSerializerAndDeserializerTest.java +++ b/src/tools/javatests/com/google/devtools/build/android/AndroidDataSerializerAndDeserializerTest.java
@@ -13,18 +13,29 @@ // limitations under the License. package com.google.devtools.build.android; +import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.android.ParsedAndroidDataBuilder.file; import static com.google.devtools.build.android.ParsedAndroidDataBuilder.xml; +import com.android.aapt.ConfigurationOuterClass.Configuration; +import com.android.aapt.ConfigurationOuterClass.Configuration.KeysHidden; +import com.android.aapt.ConfigurationOuterClass.Configuration.NavHidden; +import com.android.aapt.ConfigurationOuterClass.Configuration.Orientation; +import com.android.aapt.ConfigurationOuterClass.Configuration.ScreenLayoutLong; +import com.android.aapt.ConfigurationOuterClass.Configuration.ScreenLayoutSize; +import com.android.aapt.ConfigurationOuterClass.Configuration.ScreenRound; +import com.android.aapt.ConfigurationOuterClass.Configuration.Touchscreen; +import com.android.aapt.ConfigurationOuterClass.Configuration.UiModeNight; +import com.android.aapt.ConfigurationOuterClass.Configuration.UiModeType; import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; import com.google.common.jimfs.Jimfs; -import com.google.common.truth.Truth; import com.google.devtools.build.android.xml.IdXmlResourceValue; import com.google.devtools.build.android.xml.ResourcesAttribute; import java.nio.file.FileSystem; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -69,7 +80,7 @@ AndroidDataDeserializer deserializer = AndroidParsedDataDeserializer.create(); TestMapConsumer<DataAsset> assets = TestMapConsumer.ofAssets(); deserializer.read(binaryPath, KeyValueConsumers.of(null, null, assets)); - Truth.assertThat(assets).isEqualTo(expected.getPrimary().getAssets()); + assertThat(assets).isEqualTo(expected.getPrimary().getAssets()); } @Test @@ -96,7 +107,7 @@ resources, // combining null // assets )); - Truth.assertThat(resources).isEqualTo(expected.getPrimary().getCombiningResources()); + assertThat(resources).isEqualTo(expected.getPrimary().getCombiningResources()); } @Test @@ -122,7 +133,7 @@ null, // combining null // assets )); - Truth.assertThat(resources).isEqualTo(expected.getPrimary().getOverwritingResources()); + assertThat(resources).isEqualTo(expected.getPrimary().getOverwritingResources()); } @Test @@ -176,8 +187,8 @@ combining, null // assets )); - Truth.assertThat(overwriting).isEqualTo(expected.getPrimary().getOverwritingResources()); - Truth.assertThat(combining).isEqualTo(expected.getPrimary().getCombiningResources()); + assertThat(overwriting).isEqualTo(expected.getPrimary().getOverwritingResources()); + assertThat(combining).isEqualTo(expected.getPrimary().getCombiningResources()); } @Test @@ -215,11 +226,10 @@ AndroidDataDeserializer deserializer = AndroidParsedDataDeserializer.create(); deserializer.read(binaryPath, primary); - Truth.assertThat(primary.overwritingConsumer) + assertThat(primary.overwritingConsumer) .isEqualTo(expected.getPrimary().getOverwritingResources()); - Truth.assertThat(primary.combiningConsumer) - .isEqualTo(expected.getPrimary().getCombiningResources()); - Truth.assertThat(primary.assetConsumer).isEqualTo(expected.getPrimary().getAssets()); + assertThat(primary.combiningConsumer).isEqualTo(expected.getPrimary().getCombiningResources()); + assertThat(primary.assetConsumer).isEqualTo(expected.getPrimary().getAssets()); } @Test @@ -260,8 +270,177 @@ ); deserializer.read(binaryPath, primary); - Truth.assertThat(primary.overwritingConsumer).isEqualTo(Collections.emptyMap()); - Truth.assertThat(primary.combiningConsumer).isEqualTo(Collections.emptyMap()); + assertThat(primary.overwritingConsumer).isEqualTo(Collections.emptyMap()); + assertThat(primary.combiningConsumer).isEqualTo(Collections.emptyMap()); + } + + @Test + public void testCompiledDataDeserializerCreation() { + AndroidCompiledDataDeserializer deserializer = + AndroidCompiledDataDeserializer.create(/* includeFileContentsForValidation= */ false); + assertThat(deserializer).isNotNull(); + AndroidCompiledDataDeserializer validatingDeserializer = + AndroidCompiledDataDeserializer.create(/* includeFileContentsForValidation= */ true); + assertThat(validatingDeserializer).isNotNull(); + } + + @Test + public void testNormalizedResourceDirectory_blazePrefixStripped() { + assertThat( + AndroidCompiledDataDeserializer.getNormalizedResourceDirectory( + Paths.get("blaze-out/k8-opt/bin/com/example/res/values/strings.xml"))) + .isEqualTo(Paths.get("com/example/res")); + assertThat( + AndroidCompiledDataDeserializer.getNormalizedResourceDirectory( + Paths.get("blaze-out/bin/res/values/strings.xml"))) + .isEqualTo(Paths.get("blaze-out/bin/res")); + } + + @Test + public void testNormalizedResourceDirectory_nonBlazePathPreserved() { + assertThat( + AndroidCompiledDataDeserializer.getNormalizedResourceDirectory( + Paths.get("com/example/res/values/strings.xml"))) + .isEqualTo(Paths.get("com/example/res")); + assertThat( + AndroidCompiledDataDeserializer.getNormalizedResourceDirectory( + Paths.get("a/b/c/d/res/values/strings.xml"))) + .isEqualTo(Paths.get("a/b/c/d/res")); + } + + @Test + public void testNormalizedResourceDirectory_shortPathsAndNulls() { + assertThat( + AndroidCompiledDataDeserializer.getNormalizedResourceDirectory( + Paths.get("values/strings.xml"))) + .isNull(); + assertThat(AndroidCompiledDataDeserializer.getNormalizedResourceDirectory((Path) null)) + .isNull(); + assertThat(AndroidCompiledDataDeserializer.getNormalizedResourceDirectory((String) null)) + .isNull(); + assertThat(AndroidCompiledDataDeserializer.getNormalizedResourceDirectory("")).isNull(); + } + + @Test + public void testConvertToQualifiers_defaultInstance() { + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers(Configuration.getDefaultInstance())) + .isEmpty(); + } + + @Test + public void testConvertToQualifiers_individualQualifiers() { + // MCC & MNC + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder().setMcc(310).setMnc(260).build())) + .containsAtLeast("mcc310", "mnc260"); + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder().setMnc(0xffff).build())) + .containsExactly("mnc000"); + + // Locale + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder().setLocale("en-US").build())) + .containsExactly("en-rUS"); + + // Layout Direction + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder() + .setLayoutDirection(Configuration.LayoutDirection.LAYOUT_DIRECTION_LTR) + .build())) + .containsExactly("ldltr"); + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder() + .setLayoutDirection(Configuration.LayoutDirection.LAYOUT_DIRECTION_RTL) + .build())) + .containsExactly("ldrtl"); + + // Dimensions & Smallest Screen Width + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder().setSmallestScreenWidthDp(600).build())) + .containsExactly("sw600dp"); + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder().setScreenWidthDp(400).setScreenHeightDp(600).build())) + .containsAtLeast("w400dp", "h600dp"); + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder().setScreenWidth(1024).setScreenHeight(768).build())) + .containsExactly("1024x768"); + + // Screen Layout Size & Long + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder() + .setScreenLayoutSize(ScreenLayoutSize.SCREEN_LAYOUT_SIZE_LARGE) + .setScreenLayoutLong(ScreenLayoutLong.SCREEN_LAYOUT_LONG_LONG) + .build())) + .containsAtLeast("large", "long"); + + // Screen Round & Orientation + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder() + .setScreenRound(ScreenRound.SCREEN_ROUND_ROUND) + .setOrientation(Orientation.ORIENTATION_PORT) + .build())) + .containsAtLeast("round", "port"); + + // UI Mode & Night Mode + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder() + .setUiModeType(UiModeType.UI_MODE_TYPE_TELEVISION) + .setUiModeNight(UiModeNight.UI_MODE_NIGHT_NIGHT) + .build())) + .containsAtLeast("television", "night"); + + // Density + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder().setDensity(320).build())) + .containsExactly("xhdpi"); + + // Touchscreen & Keyboard + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder() + .setTouchscreen(Touchscreen.TOUCHSCREEN_FINGER) + .setKeysHidden(KeysHidden.KEYS_HIDDEN_KEYSEXPOSED) + .setKeyboard(Configuration.Keyboard.KEYBOARD_QWERTY) + .build())) + .containsAtLeast("finger", "keysexposed", "qwerty"); + + // Navigation & NavHidden + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder() + .setNavHidden(NavHidden.NAV_HIDDEN_NAVEXPOSED) + .setNavigation(Configuration.Navigation.NAVIGATION_DPAD) + .build())) + .containsAtLeast("navexposed", "dpad"); + + // SdkVersion + assertThat( + AndroidCompiledDataDeserializer.convertToQualifiers( + Configuration.newBuilder().setSdkVersion(28).build())) + .containsExactly("v28"); + } + + @Test + public void testConvertToQualifiers_cachedLookup() { + Configuration config = + Configuration.newBuilder().setMcc(310).setLocale("fr-FR").setSdkVersion(30).build(); + ImmutableList<String> first = AndroidCompiledDataDeserializer.convertToQualifiers(config); + ImmutableList<String> second = AndroidCompiledDataDeserializer.convertToQualifiers(config); + assertThat(first).containsAtLeast("mcc310", "fr-rFR", "v30"); + assertThat(second).isSameInstanceAs(first); } private static class TestMapConsumer<T extends DataValue> @@ -277,7 +456,7 @@ return new TestMapConsumer<>(new HashMap<DataKey, DataResource>()); } - public TestMapConsumer(Map<DataKey, T> target) { + private TestMapConsumer(Map<DataKey, T> target) { this.target = target; } @@ -342,7 +521,7 @@ } @Override - public Set<java.util.Map.Entry<DataKey, T>> entrySet() { + public Set<Entry<DataKey, T>> entrySet() { return target.entrySet(); }
diff --git a/src/tools/javatests/com/google/devtools/build/android/ValidateAndLinkResourcesActionTest.java b/src/tools/javatests/com/google/devtools/build/android/ValidateAndLinkResourcesActionTest.java index 8b66c67..99f66de 100644 --- a/src/tools/javatests/com/google/devtools/build/android/ValidateAndLinkResourcesActionTest.java +++ b/src/tools/javatests/com/google/devtools/build/android/ValidateAndLinkResourcesActionTest.java
@@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import com.android.aapt.Resources.Reference; import com.android.aapt.Resources.XmlAttribute; import com.android.aapt.Resources.XmlElement; import com.android.aapt.Resources.XmlNode; @@ -182,14 +183,16 @@ .build()) .build(); + ImmutableList<Reference> manifestReferences = + XmlUtils.getAllResourceReferences(manifestWithRef); + ImmutableList<CompiledResources> includes = ImmutableList.of(dep); + UserException expected = assertThrows( UserException.class, () -> ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences( - XmlUtils.getAllResourceReferences(manifestWithRef), - dummyCompiledResources, - ImmutableList.of(dep))); + manifestReferences, dummyCompiledResources, includes)); assertThat(expected) .hasMessageThat() @@ -218,14 +221,16 @@ CompiledResources lib = createCompiledResources("lib", libFiles, "<manifest package=\"com.lib\"/>"); + ImmutableList<Reference> manifestReferences = + XmlUtils.getAllResourceReferences(XmlNode.getDefaultInstance()); + ImmutableList<CompiledResources> includes = ImmutableList.of(dep); + UserException expected = assertThrows( UserException.class, () -> ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences( - XmlUtils.getAllResourceReferences(XmlNode.getDefaultInstance()), - lib, - ImmutableList.of(dep))); + manifestReferences, lib, includes)); assertThat(expected) .hasMessageThat() @@ -278,4 +283,28 @@ assertThrows(UserException.class, () -> ValidateAndLinkResourcesAction.main(args)); } + + @Test + public void testCheckVisibilityOfResourceReferences_directoryWithoutPublicNotPrivate() + throws Exception { + Map<String, String> depFiles = new HashMap<>(); + depFiles.put( + "values/strings.xml", + "<resources><string name=\"unrelated_string\">hello</string></resources>"); + + CompiledResources dep = + createCompiledResources("dep_no_pub", depFiles, "<manifest package=\"com.dep.no.pub\"/>"); + + Map<String, String> libFiles = new HashMap<>(); + libFiles.put( + "values/values.xml", + "<resources><string name=\"lib_string\">@string/unrelated_string</string></resources>"); + + CompiledResources lib = + createCompiledResources("lib_no_pub", libFiles, "<manifest package=\"com.lib.no.pub\"/>"); + + // Validating visibility against a dependency directory without <public> tags should NOT throw. + ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences( + /* manifestReferences= */ ImmutableList.of(), lib, ImmutableList.of(dep)); + } }
diff --git a/src/tools/javatests/com/google/devtools/build/android/aapt2/ResourceLinkerTest.java b/src/tools/javatests/com/google/devtools/build/android/aapt2/ResourceLinkerTest.java index 5c1b4c9..6d7b6f7 100644 --- a/src/tools/javatests/com/google/devtools/build/android/aapt2/ResourceLinkerTest.java +++ b/src/tools/javatests/com/google/devtools/build/android/aapt2/ResourceLinkerTest.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.android.aapt2; import static com.google.common.truth.Truth.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -66,14 +67,14 @@ // Add a flat file ZipEntry flatEntry = new ZipEntry("values_default.flat"); zos.putNextEntry(flatEntry); - zos.write("fake flat content".getBytes()); + zos.write("fake flat content".getBytes(UTF_8)); zos.closeEntry(); if (includeNonFlat) { // Add a non-flat file ZipEntry txtEntry = new ZipEntry("dummy.txt"); zos.putNextEntry(txtEntry); - zos.write("fake txt content".getBytes()); + zos.write("fake txt content".getBytes(UTF_8)); zos.closeEntry(); } } @@ -120,4 +121,25 @@ assertThat(Files.exists(expectedFilteredZip)).isTrue(); assertThat(paths).containsExactly(expectedFilteredZip.toString()); } + + @Test + public void testExtractPackages_concurrentParsing() throws Exception { + Path manifest1 = tempDir.resolve("AndroidManifest1.xml"); + Files.writeString(manifest1, "<manifest package=\"com.test.pkg1\"/>"); + Path manifest2 = tempDir.resolve("AndroidManifest2.xml"); + Files.writeString(manifest2, "<manifest package=\"com.test.pkg2\"/>"); + + Path zip1 = createFakeCompiledResourcesZip(false); + CompiledResources res1 = CompiledResources.from(zip1, manifest1); + Path zip2 = createFakeCompiledResourcesZip(false); + CompiledResources res2 = CompiledResources.from(zip2, manifest2); + + ResourceLinker linker = + ResourceLinker.create(AAPT2, executorService, workingDir).include(ImmutableList.of(res1)); + + Path packagesFile = linker.extractPackages(res2); + assertThat(Files.exists(packagesFile)).isTrue(); + List<String> packages = Files.readAllLines(packagesFile); + assertThat(packages).containsExactly("com.test.pkg1", "com.test.pkg2").inOrder(); + } }