Update private resource validation to also handle non-Proto XML manifests. Previously, a call to aapt2 convert was required to produce a Proto XML manifest that could be parsed into an XmlNode for visibility validation. An new method overload of getAllResrouceReferences within ProtoXmlUtils has been added which uses XMLEventReader to scan a non-proto manifest for attributes and uses preexisting helper methods to aggregate a resource reference list. This change allows for future optimizations which skip aapt2 convert in PackageAndroidResources, potentially resulting in significant reductions in runtime for android_local_test builds. Finally, ProtoXmlUtils has been renamed to XmlUtils to reflect that its functionality is now format agnostic. PiperOrigin-RevId: 945342960 Change-Id: I70fa42ae9aac3ccf1d3bc5b47d2d8f190e6c4d89
diff --git a/src/tools/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java b/src/tools/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java index 8212ed7..95afbb4 100644 --- a/src/tools/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java +++ b/src/tools/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java
@@ -17,6 +17,7 @@ import static com.google.common.collect.Streams.concat; import static java.util.stream.Collectors.toList; +import com.android.aapt.Resources.Reference; import com.android.builder.core.VariantTypeImpl; import com.android.utils.StdLogger; import com.beust.jcommander.JCommander; @@ -38,6 +39,7 @@ import com.google.devtools.build.android.aapt2.ResourceCompiler; import com.google.devtools.build.android.aapt2.ResourceLinker; import com.google.devtools.build.android.aapt2.StaticLibrary; +import com.google.devtools.build.android.xml.XmlUtils; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; @@ -413,10 +415,17 @@ .link(compiled); profiler.recordEndOf("link").startTask("validate"); + ImmutableList<Reference> manifestReferences; + if (packagedResources.proto() != null) { + manifestReferences = + XmlUtils.getAllResourceReferences( + ProtoApk.readFrom(packagedResources.proto()).getManifest()); + } else { + manifestReferences = XmlUtils.getAllResourceReferences(compiled.getManifest()); + } + ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences( - ProtoApk.readFrom(packagedResources.proto()).getManifest(), - compiled, - compiledResourceDeps); + manifestReferences, compiled, compiledResourceDeps); profiler.recordEndOf("validate");
diff --git a/src/tools/java/com/google/devtools/build/android/DataValueFile.java b/src/tools/java/com/google/devtools/build/android/DataValueFile.java index 74949b6..e06874d 100644 --- a/src/tools/java/com/google/devtools/build/android/DataValueFile.java +++ b/src/tools/java/com/google/devtools/build/android/DataValueFile.java
@@ -21,7 +21,7 @@ import com.google.devtools.build.android.AndroidResourceMerger.MergingException; import com.google.devtools.build.android.proto.SerializeFormat; import com.google.devtools.build.android.resources.Visibility; -import com.google.devtools.build.android.xml.ProtoXmlUtils; +import com.google.devtools.build.android.xml.XmlUtils; import com.google.protobuf.CodedOutputStream; import java.io.IOException; import java.io.OutputStream; @@ -198,7 +198,7 @@ if (rootXmlNode == null) { return ImmutableList.of(); } else { - return ProtoXmlUtils.getAllResourceReferences(rootXmlNode); + return XmlUtils.getAllResourceReferences(rootXmlNode); } } }
diff --git a/src/tools/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java b/src/tools/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java index 46980fc..a8b2f84 100644 --- a/src/tools/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java +++ b/src/tools/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java
@@ -30,7 +30,7 @@ import com.google.devtools.build.android.aapt2.ResourceLinker; import com.google.devtools.build.android.aapt2.StaticLibrary; import com.google.devtools.build.android.resources.Visibility; -import com.google.devtools.build.android.xml.ProtoXmlUtils; +import com.google.devtools.build.android.xml.XmlUtils; import java.nio.file.Path; import java.util.List; import java.util.Map; @@ -158,7 +158,9 @@ // TODO(b/146663858): distinguish direct/transitive deps for "strict deps". // TODO(b/128711690): validate AndroidManifest.xml checkVisibilityOfResourceReferences( - /* androidManifest= */ XmlNode.getDefaultInstance(), resources, includes); + /* manifestReferences= */ XmlUtils.getAllResourceReferences(XmlNode.getDefaultInstance()), + resources, + includes); ImmutableList<StaticLibrary> resourceApks = ImmutableList.of(); if (options.resourceApks != null) { @@ -198,10 +200,10 @@ * @param deps resources from the transitive closure of the rule's "deps" attribute */ static void checkVisibilityOfResourceReferences( - XmlNode androidManifest, CompiledResources compiled, List<CompiledResources> deps) { + ImmutableList<Reference> manifestReferences, + CompiledResources compiled, + List<CompiledResources> deps) { - ImmutableList<Reference> manifestReferences = - ProtoXmlUtils.getAllResourceReferences(androidManifest); // We only validate visibility against resources from Bazel library dependencies (deps), so we // ignore Android system resource references ("android:"). boolean hasRelevantManifestReferences =
diff --git a/src/tools/java/com/google/devtools/build/android/xml/ProtoXmlUtils.java b/src/tools/java/com/google/devtools/build/android/xml/XmlUtils.java similarity index 75% rename from src/tools/java/com/google/devtools/build/android/xml/ProtoXmlUtils.java rename to src/tools/java/com/google/devtools/build/android/xml/XmlUtils.java index e40f4ca..a6e060d 100644 --- a/src/tools/java/com/google/devtools/build/android/xml/ProtoXmlUtils.java +++ b/src/tools/java/com/google/devtools/build/android/xml/XmlUtils.java
@@ -20,11 +20,22 @@ import com.android.aapt.Resources.XmlNode; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.android.XmlResourceValues; import com.google.devtools.build.android.resources.ResourceTypeEnum; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Iterator; import java.util.Optional; +import javax.xml.namespace.QName; +import javax.xml.stream.XMLEventReader; +import javax.xml.stream.XMLStreamException; +import javax.xml.stream.events.Attribute; +import javax.xml.stream.events.XMLEvent; /** Utilities for manipulating XML (as represented by aapt2's protobuf definitions). */ -public final class ProtoXmlUtils { +public final class XmlUtils { private static final String SCHEMA_AUTO = "http://schemas.android.com/apk/res-auto"; private static final String SCHEMA_PUBLIC_PREFIX = "http://schemas.android.com/apk/res/"; @@ -59,6 +70,39 @@ } } + /** + * Returns all resources referenced from a plain text XML manifest file, including attribute + * references. + * + * @param xmlManifest path to a plain text XML manifest file. + */ + public static ImmutableList<Reference> getAllResourceReferences(Path xmlManifest) + throws IOException { + ImmutableList.Builder<Reference> refs = ImmutableList.builder(); + try (InputStream input = Files.newInputStream(xmlManifest)) { + XMLEventReader reader = XmlResourceValues.getXmlInputFactory().createXMLEventReader(input); + while (reader.hasNext()) { + XMLEvent event = reader.nextEvent(); + if (!event.isStartElement()) { + continue; + } + Iterator<Attribute> attributes = event.asStartElement().getAttributes(); + while (attributes.hasNext()) { + Attribute attr = attributes.next(); + QName qName = attr.getName(); + + parseAttributeNameReference(qName.getNamespaceURI(), qName.getLocalPart()) + .ifPresent(refs::add); + + parseResourceReference(attr.getValue()).ifPresent(refs::add); + } + } + } catch (XMLStreamException e) { + throw new IOException("Failed to scan manifest XML for resource references", e); + } + return refs.build(); + } + @VisibleForTesting static Optional<Reference> parseAttributeNameReference(String uri, String name) { if (uri.isEmpty()) { @@ -132,5 +176,5 @@ return Optional.empty(); } - private ProtoXmlUtils() {} + private XmlUtils() {} }
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 08ee554..60d37b1 100644 --- a/src/tools/javatests/com/google/devtools/build/android/ValidateAndLinkResourcesActionTest.java +++ b/src/tools/javatests/com/google/devtools/build/android/ValidateAndLinkResourcesActionTest.java
@@ -22,6 +22,7 @@ import com.android.aapt.Resources.XmlNode; import com.google.common.collect.ImmutableList; import com.google.devtools.build.android.aapt2.CompiledResources; +import com.google.devtools.build.android.xml.XmlUtils; import java.io.FileOutputStream; import java.nio.ByteBuffer; import java.nio.ByteOrder; @@ -114,7 +115,9 @@ @Test public void visibilityCheck_successNoReferences() throws Exception { ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences( - XmlNode.getDefaultInstance(), dummyCompiledResources, ImmutableList.of()); + XmlUtils.getAllResourceReferences(XmlNode.getDefaultInstance()), + dummyCompiledResources, + ImmutableList.of()); } @Test @@ -151,7 +154,7 @@ createCompiledResources("lib", libFiles, "<manifest package=\"com.lib\"/>"); ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences( - manifestWithRef, lib, ImmutableList.of(dep)); + XmlUtils.getAllResourceReferences(manifestWithRef), lib, ImmutableList.of(dep)); } @Test @@ -184,7 +187,9 @@ UserException.class, () -> ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences( - manifestWithRef, dummyCompiledResources, ImmutableList.of(dep))); + XmlUtils.getAllResourceReferences(manifestWithRef), + dummyCompiledResources, + ImmutableList.of(dep))); assertThat(expected) .hasMessageThat() @@ -218,7 +223,9 @@ UserException.class, () -> ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences( - XmlNode.getDefaultInstance(), lib, ImmutableList.of(dep))); + XmlUtils.getAllResourceReferences(XmlNode.getDefaultInstance()), + lib, + ImmutableList.of(dep))); assertThat(expected) .hasMessageThat()
diff --git a/src/tools/javatests/com/google/devtools/build/android/xml/BUILD b/src/tools/javatests/com/google/devtools/build/android/xml/BUILD index 669bc1e..2e9e7c8 100644 --- a/src/tools/javatests/com/google/devtools/build/android/xml/BUILD +++ b/src/tools/javatests/com/google/devtools/build/android/xml/BUILD
@@ -22,18 +22,18 @@ ) java_test( - name = "ProtoXmlUtilsTest", + name = "XmlUtilsTest", size = "small", - srcs = ["ProtoXmlUtilsTest.java"], + srcs = ["XmlUtilsTest.java"], + runtime_deps = [ + "@rules_android_maven//:com_google_guava_guava", + ], deps = [ "//src/tools/java/com/google/devtools/build/android:android_builder_lib", "//src/tools/java/com/google/devtools/build/android/proto:resources_java_proto", "@rules_android_maven//:com_google_truth_truth", "@rules_android_maven//:junit_junit", ], - runtime_deps = [ - "@rules_android_maven//:com_google_guava_guava", - ], ) java_test(
diff --git a/src/tools/javatests/com/google/devtools/build/android/xml/ProtoXmlUtilsTest.java b/src/tools/javatests/com/google/devtools/build/android/xml/ProtoXmlUtilsTest.java deleted file mode 100644 index 3325b6c..0000000 --- a/src/tools/javatests/com/google/devtools/build/android/xml/ProtoXmlUtilsTest.java +++ /dev/null
@@ -1,103 +0,0 @@ -// Copyright 2020 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -package com.google.devtools.build.android.xml; - -import static com.google.common.truth.Truth.assertThat; - -import com.android.aapt.Resources.Reference; -import com.android.aapt.Resources.XmlAttribute; -import com.android.aapt.Resources.XmlElement; -import com.android.aapt.Resources.XmlNode; -import java.util.Optional; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Unit tests for {@link ProtoXmlUtils}. */ -@RunWith(JUnit4.class) -public final class ProtoXmlUtilsTest { - - @Test - public void parseAttributeNameReference() { - assertThat( - ProtoXmlUtils.parseAttributeNameReference( - "http://schemas.android.com/apk/res-auto", "foo")) - .isEqualTo(Optional.of(Reference.newBuilder().setName("attr/foo").build())); - assertThat( - ProtoXmlUtils.parseAttributeNameReference( - "http://schemas.android.com/apk/res/android", "foo")) - .isEqualTo(Optional.of(Reference.newBuilder().setName("android:attr/foo").build())); - assertThat( - ProtoXmlUtils.parseAttributeNameReference( - "http://schemas.android.com/apk/prv/res/android", "foo")) - .isEqualTo( - Optional.of( - Reference.newBuilder().setPrivate(true).setName("android:attr/foo").build())); - - assertThat(ProtoXmlUtils.parseAttributeNameReference("", "foo")).isEqualTo(Optional.empty()); - assertThat(ProtoXmlUtils.parseAttributeNameReference("http://asdf", "foo")) - .isEqualTo(Optional.empty()); - } - - @Test - public void parseResourceReference() { - assertThat(ProtoXmlUtils.parseResourceReference("@string/foo")) - .isEqualTo( - Optional.of( - Reference.newBuilder() - .setType(Reference.Type.REFERENCE) - .setName("string/foo") - .build())); - assertThat(ProtoXmlUtils.parseResourceReference("?*android:attr/foo")) - .isEqualTo( - Optional.of( - Reference.newBuilder() - .setType(Reference.Type.ATTRIBUTE) - .setPrivate(true) - .setName("android:attr/foo") - .build())); - - assertThat(ProtoXmlUtils.parseResourceReference("x")).isEqualTo(Optional.empty()); - assertThat(ProtoXmlUtils.parseResourceReference("@")).isEqualTo(Optional.empty()); - assertThat(ProtoXmlUtils.parseResourceReference("@x")).isEqualTo(Optional.empty()); - assertThat(ProtoXmlUtils.parseResourceReference("@x/foo")).isEqualTo(Optional.empty()); - } - - @Test - public void getAllResourceReferences() { - XmlNode root = - XmlNode.newBuilder() - .setElement( - XmlElement.newBuilder() - .addAttribute( - XmlAttribute.newBuilder() - .setNamespaceUri("http://schemas.android.com/apk/res/android") - .setName("text") - .setValue("asdf")) - .addChild( - XmlNode.newBuilder() - .setElement( - XmlElement.newBuilder() - .addAttribute( - XmlAttribute.newBuilder() - .setName("id") - .setValue("@string/foo"))))) - .build(); - - assertThat(ProtoXmlUtils.getAllResourceReferences(root)) - .containsExactly( - Reference.newBuilder().setName("android:attr/text").build(), - Reference.newBuilder().setName("string/foo").build()); - } -}
diff --git a/src/tools/javatests/com/google/devtools/build/android/xml/XmlUtilsTest.java b/src/tools/javatests/com/google/devtools/build/android/xml/XmlUtilsTest.java new file mode 100644 index 0000000..5618617 --- /dev/null +++ b/src/tools/javatests/com/google/devtools/build/android/xml/XmlUtilsTest.java
@@ -0,0 +1,115 @@ +// Copyright 2020 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.android.xml; + +import static com.google.common.truth.Truth.assertThat; + +import com.android.aapt.Resources.Reference; +import com.android.aapt.Resources.XmlAttribute; +import com.android.aapt.Resources.XmlElement; +import com.android.aapt.Resources.XmlNode; +import java.nio.file.Files; +import java.nio.file.Path; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for {@link XmlUtils}. */ +@RunWith(JUnit4.class) +public final class XmlUtilsTest { + + @Rule public final TemporaryFolder tempFolder = new TemporaryFolder(); + + @Test + public void parseAttributeNameReference() { + assertThat( + XmlUtils.parseAttributeNameReference("http://schemas.android.com/apk/res-auto", "foo")) + .hasValue(Reference.newBuilder().setName("attr/foo").build()); + assertThat( + XmlUtils.parseAttributeNameReference( + "http://schemas.android.com/apk/res/android", "foo")) + .hasValue(Reference.newBuilder().setName("android:attr/foo").build()); + assertThat( + XmlUtils.parseAttributeNameReference( + "http://schemas.android.com/apk/prv/res/android", "foo")) + .hasValue(Reference.newBuilder().setPrivate(true).setName("android:attr/foo").build()); + + assertThat(XmlUtils.parseAttributeNameReference("", "foo")).isEmpty(); + assertThat(XmlUtils.parseAttributeNameReference("http://asdf", "foo")).isEmpty(); + } + + @Test + public void parseResourceReference() { + assertThat(XmlUtils.parseResourceReference("@string/foo")) + .hasValue( + Reference.newBuilder().setType(Reference.Type.REFERENCE).setName("string/foo").build()); + assertThat(XmlUtils.parseResourceReference("?*android:attr/foo")) + .hasValue( + Reference.newBuilder() + .setType(Reference.Type.ATTRIBUTE) + .setPrivate(true) + .setName("android:attr/foo") + .build()); + + assertThat(XmlUtils.parseResourceReference("x")).isEmpty(); + assertThat(XmlUtils.parseResourceReference("@")).isEmpty(); + assertThat(XmlUtils.parseResourceReference("@x")).isEmpty(); + assertThat(XmlUtils.parseResourceReference("@x/foo")).isEmpty(); + } + + @Test + public void getAllResourceReferences() { + XmlNode root = + XmlNode.newBuilder() + .setElement( + XmlElement.newBuilder() + .addAttribute( + XmlAttribute.newBuilder() + .setNamespaceUri("http://schemas.android.com/apk/res/android") + .setName("text") + .setValue("asdf")) + .addChild( + XmlNode.newBuilder() + .setElement( + XmlElement.newBuilder() + .addAttribute( + XmlAttribute.newBuilder() + .setName("id") + .setValue("@string/foo"))))) + .build(); + + assertThat(XmlUtils.getAllResourceReferences(root)) + .containsExactly( + Reference.newBuilder().setName("android:attr/text").build(), + Reference.newBuilder().setName("string/foo").build()); + } + + @Test + public void getAllResourceReferences_manifestPath() throws Exception { + Path manifestPath = tempFolder.newFile("AndroidManifest.xml").toPath(); + Files.writeString( + manifestPath, + "<manifest xmlns:android=\"http://schemas.android.com/apk/res/android\"" + + " android:text=\"asdf\">\n" + + " <application id=\"@string/foo\"/>\n" + + "</manifest>"); + + assertThat(XmlUtils.getAllResourceReferences(manifestPath)) + .containsExactly( + Reference.newBuilder().setName("android:attr/text").build(), + Reference.newBuilder().setName("string/foo").build()); + } +}