Skip to content

Commit 00040bf

Browse files
Milezoefifcopybara-github
Milezoefif
authored and
copybara-github
committed
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
1 parent 2975c3b commit 00040bf

7 files changed

Lines changed: 130 additions & 56 deletions

File tree

‎src/tools/java/com/google/devtools/build/android/Aapt2ResourcePackagingAction.java‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import static com.google.common.collect.Streams.concat;
1818
import static java.util.stream.Collectors.toList;
1919

20+
import com.android.aapt.Resources.Reference;
2021
import com.android.builder.core.VariantTypeImpl;
2122
import com.android.utils.StdLogger;
2223
import com.beust.jcommander.JCommander;
@@ -38,6 +39,7 @@
3839
import com.google.devtools.build.android.aapt2.ResourceCompiler;
3940
import com.google.devtools.build.android.aapt2.ResourceLinker;
4041
import com.google.devtools.build.android.aapt2.StaticLibrary;
42+
import com.google.devtools.build.android.xml.XmlUtils;
4143
import java.io.IOException;
4244
import java.nio.file.Files;
4345
import java.nio.file.Path;
@@ -413,10 +415,17 @@ public static void main(String[] args) throws Exception {
413415
.link(compiled);
414416
profiler.recordEndOf("link").startTask("validate");
415417

418+
ImmutableList<Reference> manifestReferences;
419+
if (packagedResources.proto() != null) {
420+
manifestReferences =
421+
XmlUtils.getAllResourceReferences(
422+
ProtoApk.readFrom(packagedResources.proto()).getManifest());
423+
} else {
424+
manifestReferences = XmlUtils.getAllResourceReferences(compiled.getManifest());
425+
}
426+
416427
ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences(
417-
ProtoApk.readFrom(packagedResources.proto()).getManifest(),
418-
compiled,
419-
compiledResourceDeps);
428+
manifestReferences, compiled, compiledResourceDeps);
420429

421430
profiler.recordEndOf("validate");
422431

‎src/tools/java/com/google/devtools/build/android/DataValueFile.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
import com.google.devtools.build.android.AndroidResourceMerger.MergingException;
2222
import com.google.devtools.build.android.proto.SerializeFormat;
2323
import com.google.devtools.build.android.resources.Visibility;
24-
import com.google.devtools.build.android.xml.ProtoXmlUtils;
24+
import com.google.devtools.build.android.xml.XmlUtils;
2525
import com.google.protobuf.CodedOutputStream;
2626
import java.io.IOException;
2727
import java.io.OutputStream;
@@ -198,7 +198,7 @@ public ImmutableList<Reference> getReferencedResources() {
198198
if (rootXmlNode == null) {
199199
return ImmutableList.of();
200200
} else {
201-
return ProtoXmlUtils.getAllResourceReferences(rootXmlNode);
201+
return XmlUtils.getAllResourceReferences(rootXmlNode);
202202
}
203203
}
204204
}

‎src/tools/java/com/google/devtools/build/android/ValidateAndLinkResourcesAction.java‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
import com.google.devtools.build.android.aapt2.ResourceLinker;
3131
import com.google.devtools.build.android.aapt2.StaticLibrary;
3232
import com.google.devtools.build.android.resources.Visibility;
33-
import com.google.devtools.build.android.xml.ProtoXmlUtils;
33+
import com.google.devtools.build.android.xml.XmlUtils;
3434
import java.nio.file.Path;
3535
import java.util.List;
3636
import java.util.Map;
@@ -158,7 +158,9 @@ public static void main(String[] args) throws Exception {
158158
// TODO(b/146663858): distinguish direct/transitive deps for "strict deps".
159159
// TODO(b/128711690): validate AndroidManifest.xml
160160
checkVisibilityOfResourceReferences(
161-
/* androidManifest= */ XmlNode.getDefaultInstance(), resources, includes);
161+
/* manifestReferences= */ XmlUtils.getAllResourceReferences(XmlNode.getDefaultInstance()),
162+
resources,
163+
includes);
162164

163165
ImmutableList<StaticLibrary> resourceApks = ImmutableList.of();
164166
if (options.resourceApks != null) {
@@ -198,10 +200,10 @@ public static void main(String[] args) throws Exception {
198200
* @param deps resources from the transitive closure of the rule's "deps" attribute
199201
*/
200202
static void checkVisibilityOfResourceReferences(
201-
XmlNode androidManifest, CompiledResources compiled, List<CompiledResources> deps) {
203+
ImmutableList<Reference> manifestReferences,
204+
CompiledResources compiled,
205+
List<CompiledResources> deps) {
202206

203-
ImmutableList<Reference> manifestReferences =
204-
ProtoXmlUtils.getAllResourceReferences(androidManifest);
205207
// We only validate visibility against resources from Bazel library dependencies (deps), so we
206208
// ignore Android system resource references ("android:").
207209
boolean hasRelevantManifestReferences =

src/tools/java/com/google/devtools/build/android/xml/ProtoXmlUtils.java renamed to src/tools/java/com/google/devtools/build/android/xml/XmlUtils.java

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,22 @@
2020
import com.android.aapt.Resources.XmlNode;
2121
import com.google.common.annotations.VisibleForTesting;
2222
import com.google.common.collect.ImmutableList;
23+
import com.google.devtools.build.android.XmlResourceValues;
2324
import com.google.devtools.build.android.resources.ResourceTypeEnum;
25+
import java.io.IOException;
26+
import java.io.InputStream;
27+
import java.nio.file.Files;
28+
import java.nio.file.Path;
29+
import java.util.Iterator;
2430
import java.util.Optional;
31+
import javax.xml.namespace.QName;
32+
import javax.xml.stream.XMLEventReader;
33+
import javax.xml.stream.XMLStreamException;
34+
import javax.xml.stream.events.Attribute;
35+
import javax.xml.stream.events.XMLEvent;
2536

2637
/** Utilities for manipulating XML (as represented by aapt2's protobuf definitions). */
27-
public final class ProtoXmlUtils {
38+
public final class XmlUtils {
2839

2940
private static final String SCHEMA_AUTO = "http://schemas.android.com/apk/res-auto";
3041
private static final String SCHEMA_PUBLIC_PREFIX = "http://schemas.android.com/apk/res/";
@@ -59,6 +70,39 @@ private static void getAllResourceReferences(
5970
}
6071
}
6172

73+
/**
74+
* Returns all resources referenced from a plain text XML manifest file, including attribute
75+
* references.
76+
*
77+
* @param xmlManifest path to a plain text XML manifest file.
78+
*/
79+
public static ImmutableList<Reference> getAllResourceReferences(Path xmlManifest)
80+
throws IOException {
81+
ImmutableList.Builder<Reference> refs = ImmutableList.builder();
82+
try (InputStream input = Files.newInputStream(xmlManifest)) {
83+
XMLEventReader reader = XmlResourceValues.getXmlInputFactory().createXMLEventReader(input);
84+
while (reader.hasNext()) {
85+
XMLEvent event = reader.nextEvent();
86+
if (!event.isStartElement()) {
87+
continue;
88+
}
89+
Iterator<Attribute> attributes = event.asStartElement().getAttributes();
90+
while (attributes.hasNext()) {
91+
Attribute attr = attributes.next();
92+
QName qName = attr.getName();
93+
94+
parseAttributeNameReference(qName.getNamespaceURI(), qName.getLocalPart())
95+
.ifPresent(refs::add);
96+
97+
parseResourceReference(attr.getValue()).ifPresent(refs::add);
98+
}
99+
}
100+
} catch (XMLStreamException e) {
101+
throw new IOException("Failed to scan manifest XML for resource references", e);
102+
}
103+
return refs.build();
104+
}
105+
62106
@VisibleForTesting
63107
static Optional<Reference> parseAttributeNameReference(String uri, String name) {
64108
if (uri.isEmpty()) {
@@ -132,5 +176,5 @@ static Optional<Reference> parseResourceReference(String value) {
132176
return Optional.empty();
133177
}
134178

135-
private ProtoXmlUtils() {}
179+
private XmlUtils() {}
136180
}

‎src/tools/javatests/com/google/devtools/build/android/ValidateAndLinkResourcesActionTest.java‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.android.aapt.Resources.XmlNode;
2323
import com.google.common.collect.ImmutableList;
2424
import com.google.devtools.build.android.aapt2.CompiledResources;
25+
import com.google.devtools.build.android.xml.XmlUtils;
2526
import java.io.FileOutputStream;
2627
import java.nio.ByteBuffer;
2728
import java.nio.ByteOrder;
@@ -114,7 +115,9 @@ private CompiledResources createCompiledResources(
114115
@Test
115116
public void visibilityCheck_successNoReferences() throws Exception {
116117
ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences(
117-
XmlNode.getDefaultInstance(), dummyCompiledResources, ImmutableList.of());
118+
XmlUtils.getAllResourceReferences(XmlNode.getDefaultInstance()),
119+
dummyCompiledResources,
120+
ImmutableList.of());
118121
}
119122

120123
@Test
@@ -151,7 +154,7 @@ public void visibilityCheck_successNoPrivateResourcesUsed() throws Exception {
151154
createCompiledResources("lib", libFiles, "<manifest package=\"com.lib\"/>");
152155

153156
ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences(
154-
manifestWithRef, lib, ImmutableList.of(dep));
157+
XmlUtils.getAllResourceReferences(manifestWithRef), lib, ImmutableList.of(dep));
155158
}
156159

157160
@Test
@@ -184,7 +187,9 @@ public void visibilityCheck_failureManifestUsesPrivateResource() throws Exceptio
184187
UserException.class,
185188
() ->
186189
ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences(
187-
manifestWithRef, dummyCompiledResources, ImmutableList.of(dep)));
190+
XmlUtils.getAllResourceReferences(manifestWithRef),
191+
dummyCompiledResources,
192+
ImmutableList.of(dep)));
188193

189194
assertThat(expected)
190195
.hasMessageThat()
@@ -218,7 +223,9 @@ public void visibilityCheck_failureCompiledResourceValuesUsesPrivateResource() t
218223
UserException.class,
219224
() ->
220225
ValidateAndLinkResourcesAction.checkVisibilityOfResourceReferences(
221-
XmlNode.getDefaultInstance(), lib, ImmutableList.of(dep)));
226+
XmlUtils.getAllResourceReferences(XmlNode.getDefaultInstance()),
227+
lib,
228+
ImmutableList.of(dep)));
222229

223230
assertThat(expected)
224231
.hasMessageThat()

‎src/tools/javatests/com/google/devtools/build/android/xml/BUILD‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,18 +22,18 @@ java_test(
2222
)
2323

2424
java_test(
25-
name = "ProtoXmlUtilsTest",
25+
name = "XmlUtilsTest",
2626
size = "small",
27-
srcs = ["ProtoXmlUtilsTest.java"],
27+
srcs = ["XmlUtilsTest.java"],
28+
runtime_deps = [
29+
"@rules_android_maven//:com_google_guava_guava",
30+
],
2831
deps = [
2932
"//src/tools/java/com/google/devtools/build/android:android_builder_lib",
3033
"//src/tools/java/com/google/devtools/build/android/proto:resources_java_proto",
3134
"@rules_android_maven//:com_google_truth_truth",
3235
"@rules_android_maven//:junit_junit",
3336
],
34-
runtime_deps = [
35-
"@rules_android_maven//:com_google_guava_guava",
36-
],
3737
)
3838

3939
java_test(

src/tools/javatests/com/google/devtools/build/android/xml/ProtoXmlUtilsTest.java renamed to src/tools/javatests/com/google/devtools/build/android/xml/XmlUtilsTest.java

Lines changed: 47 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -19,59 +19,55 @@
1919
import com.android.aapt.Resources.XmlAttribute;
2020
import com.android.aapt.Resources.XmlElement;
2121
import com.android.aapt.Resources.XmlNode;
22-
import java.util.Optional;
22+
import java.nio.file.Files;
23+
import java.nio.file.Path;
24+
import org.junit.Rule;
2325
import org.junit.Test;
26+
import org.junit.rules.TemporaryFolder;
2427
import org.junit.runner.RunWith;
2528
import org.junit.runners.JUnit4;
2629

27-
/** Unit tests for {@link ProtoXmlUtils}. */
30+
/** Unit tests for {@link XmlUtils}. */
2831
@RunWith(JUnit4.class)
29-
public final class ProtoXmlUtilsTest {
32+
public final class XmlUtilsTest {
33+
34+
@Rule public final TemporaryFolder tempFolder = new TemporaryFolder();
3035

3136
@Test
3237
public void parseAttributeNameReference() {
3338
assertThat(
34-
ProtoXmlUtils.parseAttributeNameReference(
35-
"http://schemas.android.com/apk/res-auto", "foo"))
36-
.isEqualTo(Optional.of(Reference.newBuilder().setName("attr/foo").build()));
39+
XmlUtils.parseAttributeNameReference("http://schemas.android.com/apk/res-auto", "foo"))
40+
.hasValue(Reference.newBuilder().setName("attr/foo").build());
3741
assertThat(
38-
ProtoXmlUtils.parseAttributeNameReference(
42+
XmlUtils.parseAttributeNameReference(
3943
"http://schemas.android.com/apk/res/android", "foo"))
40-
.isEqualTo(Optional.of(Reference.newBuilder().setName("android:attr/foo").build()));
44+
.hasValue(Reference.newBuilder().setName("android:attr/foo").build());
4145
assertThat(
42-
ProtoXmlUtils.parseAttributeNameReference(
46+
XmlUtils.parseAttributeNameReference(
4347
"http://schemas.android.com/apk/prv/res/android", "foo"))
44-
.isEqualTo(
45-
Optional.of(
46-
Reference.newBuilder().setPrivate(true).setName("android:attr/foo").build()));
48+
.hasValue(Reference.newBuilder().setPrivate(true).setName("android:attr/foo").build());
4749

48-
assertThat(ProtoXmlUtils.parseAttributeNameReference("", "foo")).isEqualTo(Optional.empty());
49-
assertThat(ProtoXmlUtils.parseAttributeNameReference("http://asdf", "foo"))
50-
.isEqualTo(Optional.empty());
50+
assertThat(XmlUtils.parseAttributeNameReference("", "foo")).isEmpty();
51+
assertThat(XmlUtils.parseAttributeNameReference("http://asdf", "foo")).isEmpty();
5152
}
5253

5354
@Test
5455
public void parseResourceReference() {
55-
assertThat(ProtoXmlUtils.parseResourceReference("@string/foo"))
56-
.isEqualTo(
57-
Optional.of(
58-
Reference.newBuilder()
59-
.setType(Reference.Type.REFERENCE)
60-
.setName("string/foo")
61-
.build()));
62-
assertThat(ProtoXmlUtils.parseResourceReference("?*android:attr/foo"))
63-
.isEqualTo(
64-
Optional.of(
65-
Reference.newBuilder()
66-
.setType(Reference.Type.ATTRIBUTE)
67-
.setPrivate(true)
68-
.setName("android:attr/foo")
69-
.build()));
56+
assertThat(XmlUtils.parseResourceReference("@string/foo"))
57+
.hasValue(
58+
Reference.newBuilder().setType(Reference.Type.REFERENCE).setName("string/foo").build());
59+
assertThat(XmlUtils.parseResourceReference("?*android:attr/foo"))
60+
.hasValue(
61+
Reference.newBuilder()
62+
.setType(Reference.Type.ATTRIBUTE)
63+
.setPrivate(true)
64+
.setName("android:attr/foo")
65+
.build());
7066

71-
assertThat(ProtoXmlUtils.parseResourceReference("x")).isEqualTo(Optional.empty());
72-
assertThat(ProtoXmlUtils.parseResourceReference("@")).isEqualTo(Optional.empty());
73-
assertThat(ProtoXmlUtils.parseResourceReference("@x")).isEqualTo(Optional.empty());
74-
assertThat(ProtoXmlUtils.parseResourceReference("@x/foo")).isEqualTo(Optional.empty());
67+
assertThat(XmlUtils.parseResourceReference("x")).isEmpty();
68+
assertThat(XmlUtils.parseResourceReference("@")).isEmpty();
69+
assertThat(XmlUtils.parseResourceReference("@x")).isEmpty();
70+
assertThat(XmlUtils.parseResourceReference("@x/foo")).isEmpty();
7571
}
7672

7773
@Test
@@ -95,7 +91,23 @@ public void getAllResourceReferences() {
9591
.setValue("@string/foo")))))
9692
.build();
9793

98-
assertThat(ProtoXmlUtils.getAllResourceReferences(root))
94+
assertThat(XmlUtils.getAllResourceReferences(root))
95+
.containsExactly(
96+
Reference.newBuilder().setName("android:attr/text").build(),
97+
Reference.newBuilder().setName("string/foo").build());
98+
}
99+
100+
@Test
101+
public void getAllResourceReferences_manifestPath() throws Exception {
102+
Path manifestPath = tempFolder.newFile("AndroidManifest.xml").toPath();
103+
Files.writeString(
104+
manifestPath,
105+
"<manifest xmlns:android=\"http://schemas.android.com/apk/res/android\""
106+
+ " android:text=\"asdf\">\n"
107+
+ " <application id=\"@string/foo\"/>\n"
108+
+ "</manifest>");
109+
110+
assertThat(XmlUtils.getAllResourceReferences(manifestPath))
99111
.containsExactly(
100112
Reference.newBuilder().setName("android:attr/text").build(),
101113
Reference.newBuilder().setName("string/foo").build());

0 commit comments

Comments
 (0)