Skip to content

Commit 3ea9eb2

Browse files
ted-xieBencodes
ted-xie
and
Bencodes
authored
Merge ManifestMergerAction-related commits into release-5.3.0 (bazelbuild#15824)
* Support uses-permission merging in manifest merger Adding support for conditionally merging `uses-permissions`. bazelbuild#10628 bazelbuild#5411 Closes bazelbuild#13445. RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag. PiperOrigin-RevId: 439613035 * Make ManifestMergerAction worker compatible Calling `System#exit` kills the worker during the build. Passing the exception up to the worker should be enough for it to end up in the worker or local execution output. Closes bazelbuild#14427. PiperOrigin-RevId: 447808701 * Fix ManifestMergerAction test case on windows `manifest.toString().replaceFirst("^/", "")` silently fails on windows machines causing `removePermissions` to write to the original test file. This pull request creates a new temp file that `removePermissions` can write the modified manifest to. Pulling this change out of another PR so that it's easier to merge. Original PR here https://github.com/bazelbuild/bazel/pull/13445/files#r631575251 Closes bazelbuild#13760. PiperOrigin-RevId: 438643774 Co-authored-by: Ben Lee <[email protected]>
1 parent 024f552 commit 3ea9eb2

File tree

10 files changed

+377
-23
lines changed

10 files changed

+377
-23
lines changed

‎src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@
9393
import com.google.devtools.build.lib.rules.android.AndroidSdkProvider;
9494
import com.google.devtools.build.lib.rules.android.AndroidStarlarkCommon;
9595
import com.google.devtools.build.lib.rules.android.ApkInfo;
96+
import com.google.devtools.build.lib.rules.android.BazelAndroidConfiguration;
9697
import com.google.devtools.build.lib.rules.android.DexArchiveAspect;
9798
import com.google.devtools.build.lib.rules.android.ProguardMappingProvider;
9899
import com.google.devtools.build.lib.rules.android.databinding.DataBindingV2Provider;
@@ -346,6 +347,7 @@ public void init(ConfiguredRuleClassProvider.Builder builder) {
346347
String toolsRepository = checkNotNull(builder.getToolsRepository());
347348

348349
builder.addConfigurationFragment(AndroidConfiguration.class);
350+
builder.addConfigurationFragment(BazelAndroidConfiguration.class);
349351
builder.addConfigurationFragment(AndroidLocalTestConfiguration.class);
350352

351353
AndroidNeverlinkAspect androidNeverlinkAspect = new AndroidNeverlinkAspect();

‎src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import com.google.devtools.build.lib.packages.TriState;
3333
import com.google.devtools.build.lib.starlarkbuildapi.android.AndroidDataContextApi;
3434
import com.google.devtools.build.lib.vfs.PathFragment;
35+
import javax.annotation.Nullable;
3536

3637
/**
3738
* Wraps common tools and settings used for working with Android assets, resources, and manifests.
@@ -207,6 +208,11 @@ public AndroidConfiguration getAndroidConfig() {
207208
return ruleContext.getConfiguration().getFragment(AndroidConfiguration.class);
208209
}
209210

211+
@Nullable
212+
public BazelAndroidConfiguration getBazelAndroidConfig() {
213+
return ruleContext.getConfiguration().getFragment(BazelAndroidConfiguration.class);
214+
}
215+
210216
/** Indicates whether Busybox actions should be passed the "--debug" flag */
211217
public boolean useDebug() {
212218
return getActionConstructionContext().getConfiguration().getCompilationMode() != OPT;

‎src/main/java/com/google/devtools/build/lib/rules/android/AndroidManifest.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,18 @@ public AndroidManifest(Artifact manifest, @Nullable String pkg, boolean exported
188188
this.exported = exported;
189189
}
190190

191+
/** Checks if manifest permission merging is enabled. */
192+
private boolean getMergeManifestPermissionsEnabled(AndroidDataContext dataContext) {
193+
// Only enable manifest merging if BazelAndroidConfiguration exists. If the class does not
194+
// exist, then return false immediately. Otherwise, return the user-specified value of
195+
// mergeAndroidManifestPermissions.
196+
BazelAndroidConfiguration bazelAndroidConfig = dataContext.getBazelAndroidConfig();
197+
if (bazelAndroidConfig == null) {
198+
return false;
199+
}
200+
return bazelAndroidConfig.getMergeAndroidManifestPermissions();
201+
}
202+
191203
/** If needed, stamps the manifest with the correct Java package */
192204
public StampedAndroidManifest stamp(AndroidDataContext dataContext) {
193205
Artifact outputManifest = getManifest();
@@ -196,6 +208,7 @@ public StampedAndroidManifest stamp(AndroidDataContext dataContext) {
196208
new ManifestMergerActionBuilder()
197209
.setManifest(manifest)
198210
.setLibrary(true)
211+
.setMergeManifestPermissions(getMergeManifestPermissionsEnabled(dataContext))
199212
.setCustomPackage(pkg)
200213
.setManifestOutput(outputManifest)
201214
.build(dataContext);
@@ -240,6 +253,7 @@ public StampedAndroidManifest mergeWithDeps(
240253
.setManifest(manifest)
241254
.setMergeeManifests(mergeeManifests)
242255
.setLibrary(false)
256+
.setMergeManifestPermissions(getMergeManifestPermissionsEnabled(dataContext))
243257
.setManifestValues(manifestValues)
244258
.setCustomPackage(pkg)
245259
.setManifestOutput(newManifest)

‎src/main/java/com/google/devtools/build/lib/rules/android/BazelAndroidConfiguration.java‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
// Copyright 2022 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.google.devtools.build.lib.rules.android;
16+
17+
import com.google.devtools.build.lib.analysis.config.BuildOptions;
18+
import com.google.devtools.build.lib.analysis.config.Fragment;
19+
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
20+
import com.google.devtools.build.lib.analysis.config.RequiresOptions;
21+
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
22+
import com.google.devtools.common.options.Option;
23+
import com.google.devtools.common.options.OptionDocumentationCategory;
24+
import com.google.devtools.common.options.OptionEffectTag;
25+
import com.google.errorprone.annotations.CheckReturnValue;
26+
27+
/** Configuration fragment for Android rules that is specific to Bazel. */
28+
@Immutable
29+
@RequiresOptions(options = {BazelAndroidConfiguration.Options.class})
30+
@CheckReturnValue
31+
public class BazelAndroidConfiguration extends Fragment {
32+
33+
@Override
34+
public boolean isImmutable() {
35+
return true; // immutable and Starlark-hashable
36+
}
37+
38+
/** Android configuration options that are specific to Bazel. */
39+
public static class Options extends FragmentOptions {
40+
41+
@Option(
42+
name = "merge_android_manifest_permissions",
43+
defaultValue = "false",
44+
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
45+
effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
46+
help =
47+
"If enabled, the manifest merger will merge uses-permission and "
48+
+ "uses-permission-sdk-23 attributes.")
49+
public boolean mergeAndroidManifestPermissions;
50+
}
51+
52+
private final boolean mergeAndroidManifestPermissions;
53+
54+
public BazelAndroidConfiguration(BuildOptions buildOptions) {
55+
Options options = buildOptions.get(BazelAndroidConfiguration.Options.class);
56+
this.mergeAndroidManifestPermissions = options.mergeAndroidManifestPermissions;
57+
}
58+
59+
public boolean getMergeAndroidManifestPermissions() {
60+
return this.mergeAndroidManifestPermissions;
61+
}
62+
}

‎src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ public class ManifestMergerActionBuilder {
2727
private Artifact manifest;
2828
private Map<Artifact, Label> mergeeManifests;
2929
private boolean isLibrary;
30+
private boolean mergeManifestPermissions;
3031
private Map<String, String> manifestValues;
3132
private String customPackage;
3233
private Artifact manifestOutput;
@@ -47,6 +48,11 @@ public ManifestMergerActionBuilder setLibrary(boolean isLibrary) {
4748
return this;
4849
}
4950

51+
public ManifestMergerActionBuilder setMergeManifestPermissions(boolean mergeManifestPermissions) {
52+
this.mergeManifestPermissions = mergeManifestPermissions;
53+
return this;
54+
}
55+
5056
public ManifestMergerActionBuilder setManifestValues(Map<String, String> manifestValues) {
5157
this.manifestValues = manifestValues;
5258
return this;
@@ -80,6 +86,10 @@ public void build(AndroidDataContext dataContext) {
8086
mergeeManifests.keySet());
8187
}
8288

89+
if (mergeManifestPermissions) {
90+
builder.addFlag("--mergeManifestPermissions");
91+
}
92+
8393
builder
8494
.maybeAddFlag("--mergeType", isLibrary)
8595
.maybeAddFlag("LIBRARY", isLibrary)

‎src/test/java/com/google/devtools/build/android/ManifestMergerActionTest.java‎

Lines changed: 136 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import static com.google.common.truth.Truth.assertThat;
1717
import static java.nio.charset.StandardCharsets.UTF_8;
18+
import static org.junit.Assert.assertThrows;
1819

1920
import com.google.common.base.Joiner;
2021
import com.google.common.base.Splitter;
@@ -92,6 +93,42 @@ private static Path rlocation(String path) throws IOException {
9293
working.toFile().deleteOnExit();
9394
}
9495

96+
@Test
97+
public void testMergeManifestWithBrokenManifestSyntax() throws Exception {
98+
String dataDir =
99+
Paths.get(System.getenv("TEST_WORKSPACE"), System.getenv("TEST_BINARY"))
100+
.resolveSibling("testing/manifestmerge")
101+
.toString()
102+
.replace('\\', '/');
103+
Files.createDirectories(working.resolve("output"));
104+
final Path mergedManifest = working.resolve("output/mergedManifest.xml");
105+
final Path brokenMergerManifest = rlocation(dataDir + "/brokenManifest/AndroidManifest.xml");
106+
assertThat(brokenMergerManifest.toFile().exists()).isTrue();
107+
108+
AndroidManifestProcessor.ManifestProcessingException e =
109+
assertThrows(
110+
AndroidManifestProcessor.ManifestProcessingException.class,
111+
() -> {
112+
ManifestMergerAction.main(
113+
generateArgs(
114+
brokenMergerManifest,
115+
ImmutableMap.of(),
116+
false, /* isLibrary */
117+
ImmutableMap.of("applicationId", "com.google.android.apps.testapp"),
118+
"", /* custom_package */
119+
mergedManifest,
120+
false /* mergeManifestPermissions */)
121+
.toArray(new String[0]));
122+
});
123+
assertThat(e)
124+
.hasMessageThat()
125+
.contains(
126+
"com.android.manifmerger.ManifestMerger2$MergeFailureException: "
127+
+ "org.xml.sax.SAXParseException; lineNumber: 6; columnNumber: 6; "
128+
+ "The markup in the document following the root element must be well-formed.");
129+
assertThat(mergedManifest.toFile().exists()).isFalse();
130+
}
131+
95132
@Test
96133
public void testMerge_GenerateDummyManifest() throws Exception {
97134
Files.createDirectories(working.resolve("output"));
@@ -159,7 +196,8 @@ public void testMerge_GenerateDummyManifest() throws Exception {
159196
false, /* isLibrary */
160197
ImmutableMap.of("applicationId", "com.google.android.apps.testapp"),
161198
"", /* custom_package */
162-
mergedManifest);
199+
mergedManifest,
200+
/* mergeManifestPermissions */ false);
163201
ManifestMergerAction.main(args.toArray(new String[0]));
164202

165203
assertThat(
@@ -174,6 +212,64 @@ public void testMerge_GenerateDummyManifest() throws Exception {
174212
.trim());
175213
}
176214

215+
@Test
216+
public void testMergeWithMergePermissionsEnabled() throws Exception {
217+
// Largely copied from testMerge() above. Perhaps worth combining the two test methods into one
218+
// method in the future?
219+
String dataDir =
220+
Paths.get(System.getenv("TEST_WORKSPACE"), System.getenv("TEST_BINARY"))
221+
.resolveSibling("testing/manifestmerge")
222+
.toString()
223+
.replace("\\", "/");
224+
225+
final Path mergerManifest = rlocation(dataDir + "/merger/AndroidManifest.xml");
226+
final Path mergeeManifestOne = rlocation(dataDir + "/mergeeOne/AndroidManifest.xml");
227+
final Path mergeeManifestTwo = rlocation(dataDir + "/mergeeTwo/AndroidManifest.xml");
228+
assertThat(mergerManifest.toFile().exists()).isTrue();
229+
assertThat(mergeeManifestOne.toFile().exists()).isTrue();
230+
assertThat(mergeeManifestTwo.toFile().exists()).isTrue();
231+
232+
// The following code retrieves the path of the only AndroidManifest.xml in the
233+
// expected-merged-permission/manifests directory. Unfortunately, this test runs
234+
// internally and externally and the files have different names.
235+
final File expectedManifestDirectory =
236+
mergerManifest.getParent().resolveSibling("expected-merged-permissions").toFile();
237+
assertThat(expectedManifestDirectory.exists()).isTrue();
238+
final String[] debug =
239+
expectedManifestDirectory.list(new PatternFilenameFilter(".*AndroidManifest\\.xml$"));
240+
assertThat(debug).isNotNull();
241+
final File[] expectedManifestDirectoryManifests =
242+
expectedManifestDirectory.listFiles((File dir, String name) -> true);
243+
assertThat(expectedManifestDirectoryManifests).isNotNull();
244+
assertThat(expectedManifestDirectoryManifests).hasLength(1);
245+
final Path expectedManifest = expectedManifestDirectoryManifests[0].toPath();
246+
247+
Files.createDirectories(working.resolve("output"));
248+
final Path mergedManifest = working.resolve("output/mergedManifest.xml");
249+
250+
List<String> args =
251+
generateArgs(
252+
mergerManifest,
253+
ImmutableMap.of(mergeeManifestOne, "mergeeOne", mergeeManifestTwo, "mergeeTwo"),
254+
false, /* isLibrary */
255+
ImmutableMap.of("applicationId", "com.google.android.apps.testapp"),
256+
"", /* custom_package */
257+
mergedManifest,
258+
/* mergeManifestPermissions */ true);
259+
ManifestMergerAction.main(args.toArray(new String[0]));
260+
261+
assertThat(
262+
Joiner.on(" ")
263+
.join(Files.readAllLines(mergedManifest, UTF_8))
264+
.replaceAll("\\s+", " ")
265+
.trim())
266+
.isEqualTo(
267+
Joiner.on(" ")
268+
.join(Files.readAllLines(expectedManifest, UTF_8))
269+
.replaceAll("\\s+", " ")
270+
.trim());
271+
}
272+
177273
@Test public void fullIntegration() throws Exception {
178274
Files.createDirectories(working.resolve("output"));
179275
final Path binaryOutput = working.resolve("output/binaryManifest.xml");
@@ -198,8 +294,15 @@ public void testMerge_GenerateDummyManifest() throws Exception {
198294
.getManifest();
199295

200296
// libFoo manifest merging
201-
List<String> args = generateArgs(libFooManifest, ImmutableMap.<Path, String>of(), true,
202-
ImmutableMap.<String, String>of(), "", libFooOutput);
297+
List<String> args =
298+
generateArgs(
299+
libFooManifest,
300+
ImmutableMap.<Path, String>of(),
301+
true,
302+
ImmutableMap.<String, String>of(),
303+
"",
304+
libFooOutput,
305+
false);
203306
ManifestMergerAction.main(args.toArray(new String[0]));
204307
assertThat(Joiner.on(" ")
205308
.join(Files.readAllLines(libFooOutput, UTF_8))
@@ -211,8 +314,15 @@ public void testMerge_GenerateDummyManifest() throws Exception {
211314
+ "</manifest>");
212315

213316
// libBar manifest merging
214-
args = generateArgs(libBarManifest, ImmutableMap.<Path, String>of(), true,
215-
ImmutableMap.<String, String>of(), "com.google.libbar", libBarOutput);
317+
args =
318+
generateArgs(
319+
libBarManifest,
320+
ImmutableMap.<Path, String>of(),
321+
true,
322+
ImmutableMap.<String, String>of(),
323+
"com.google.libbar",
324+
libBarOutput,
325+
false);
216326
ManifestMergerAction.main(args.toArray(new String[0]));
217327
assertThat(Joiner.on(" ")
218328
.join(Files.readAllLines(libBarOutput, UTF_8))
@@ -235,7 +345,8 @@ public void testMerge_GenerateDummyManifest() throws Exception {
235345
"applicationId", "com.google.android.app",
236346
"foo", "this \\\\: is \"a, \"bad string"),
237347
/* customPackage= */ "",
238-
binaryOutput);
348+
binaryOutput,
349+
/* mergeManifestPermissions */ false);
239350
ManifestMergerAction.main(args.toArray(new String[0]));
240351
assertThat(Joiner.on(" ")
241352
.join(Files.readAllLines(binaryOutput, UTF_8))
@@ -255,14 +366,26 @@ private List<String> generateArgs(
255366
boolean library,
256367
Map<String, String> manifestValues,
257368
String customPackage,
258-
Path manifestOutput) {
259-
return ImmutableList.of(
369+
Path manifestOutput,
370+
boolean mergeManifestPermissions) {
371+
ImmutableList.Builder<String> builder = ImmutableList.builder();
372+
builder.add(
260373
"--manifest", manifest.toString(),
261-
"--mergeeManifests", mapToDictionaryString(mergeeManifests),
262-
"--mergeType", library ? "LIBRARY" : "APPLICATION",
263-
"--manifestValues", mapToDictionaryString(manifestValues),
264-
"--customPackage", customPackage,
265-
"--manifestOutput", manifestOutput.toString());
374+
"--mergeeManifests", mapToDictionaryString(mergeeManifests));
375+
if (mergeManifestPermissions) {
376+
builder.add("--mergeManifestPermissions");
377+
}
378+
379+
builder.add(
380+
"--mergeType",
381+
library ? "LIBRARY" : "APPLICATION",
382+
"--manifestValues",
383+
mapToDictionaryString(manifestValues),
384+
"--customPackage",
385+
customPackage,
386+
"--manifestOutput",
387+
manifestOutput.toString());
388+
return builder.build();
266389
}
267390

268391
private <K, V> String mapToDictionaryString(Map<K, V> map) {

‎src/test/java/com/google/devtools/build/android/testing/manifestmerge/BUILD‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ filegroup(
1313
filegroup(
1414
name = "test_data",
1515
srcs = [
16+
"brokenManifest/AndroidManifest.xml",
17+
"expected-merged-permissions/AndroidManifest.xml",
1618
"expected/AndroidManifest.xml",
1719
"mergeeOne/AndroidManifest.xml",
1820
"mergeeTwo/AndroidManifest.xml",

‎src/test/java/com/google/devtools/build/android/testing/manifestmerge/brokenManifest/AndroidManifest.xml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
2+
package="com.example.bazel"
3+
android:versionCode="1"
4+
android:versionName="1.0"/>
5+
6+
<uses-sdk
7+
android:minSdkVersion="21"
8+
android:targetSdkVersion="26"/>
9+
10+
</manifest>

0 commit comments

Comments
 (0)