Skip to content

Commit

Permalink
Merge ManifestMergerAction-related commits into release-5.3.0 (bazelb…
Browse files Browse the repository at this point in the history
…uild#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 <ben@ben.cm>
  • Loading branch information
ted-xie and Bencodes authored Jul 6, 2022
1 parent 024f552 commit 3ea9eb2
Show file tree
Hide file tree
Showing 10 changed files with 377 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
import com.google.devtools.build.lib.rules.android.AndroidSdkProvider;
import com.google.devtools.build.lib.rules.android.AndroidStarlarkCommon;
import com.google.devtools.build.lib.rules.android.ApkInfo;
import com.google.devtools.build.lib.rules.android.BazelAndroidConfiguration;
import com.google.devtools.build.lib.rules.android.DexArchiveAspect;
import com.google.devtools.build.lib.rules.android.ProguardMappingProvider;
import com.google.devtools.build.lib.rules.android.databinding.DataBindingV2Provider;
Expand Down Expand Up @@ -346,6 +347,7 @@ public void init(ConfiguredRuleClassProvider.Builder builder) {
String toolsRepository = checkNotNull(builder.getToolsRepository());

builder.addConfigurationFragment(AndroidConfiguration.class);
builder.addConfigurationFragment(BazelAndroidConfiguration.class);
builder.addConfigurationFragment(AndroidLocalTestConfiguration.class);

AndroidNeverlinkAspect androidNeverlinkAspect = new AndroidNeverlinkAspect();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.devtools.build.lib.packages.TriState;
import com.google.devtools.build.lib.starlarkbuildapi.android.AndroidDataContextApi;
import com.google.devtools.build.lib.vfs.PathFragment;
import javax.annotation.Nullable;

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

@Nullable
public BazelAndroidConfiguration getBazelAndroidConfig() {
return ruleContext.getConfiguration().getFragment(BazelAndroidConfiguration.class);
}

/** Indicates whether Busybox actions should be passed the "--debug" flag */
public boolean useDebug() {
return getActionConstructionContext().getConfiguration().getCompilationMode() != OPT;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,18 @@ public AndroidManifest(Artifact manifest, @Nullable String pkg, boolean exported
this.exported = exported;
}

/** Checks if manifest permission merging is enabled. */
private boolean getMergeManifestPermissionsEnabled(AndroidDataContext dataContext) {
// Only enable manifest merging if BazelAndroidConfiguration exists. If the class does not
// exist, then return false immediately. Otherwise, return the user-specified value of
// mergeAndroidManifestPermissions.
BazelAndroidConfiguration bazelAndroidConfig = dataContext.getBazelAndroidConfig();
if (bazelAndroidConfig == null) {
return false;
}
return bazelAndroidConfig.getMergeAndroidManifestPermissions();
}

/** If needed, stamps the manifest with the correct Java package */
public StampedAndroidManifest stamp(AndroidDataContext dataContext) {
Artifact outputManifest = getManifest();
Expand All @@ -196,6 +208,7 @@ public StampedAndroidManifest stamp(AndroidDataContext dataContext) {
new ManifestMergerActionBuilder()
.setManifest(manifest)
.setLibrary(true)
.setMergeManifestPermissions(getMergeManifestPermissionsEnabled(dataContext))
.setCustomPackage(pkg)
.setManifestOutput(outputManifest)
.build(dataContext);
Expand Down Expand Up @@ -240,6 +253,7 @@ public StampedAndroidManifest mergeWithDeps(
.setManifest(manifest)
.setMergeeManifests(mergeeManifests)
.setLibrary(false)
.setMergeManifestPermissions(getMergeManifestPermissionsEnabled(dataContext))
.setManifestValues(manifestValues)
.setCustomPackage(pkg)
.setManifestOutput(newManifest)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Copyright 2022 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.lib.rules.android;

import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.RequiresOptions;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
import com.google.devtools.common.options.OptionEffectTag;
import com.google.errorprone.annotations.CheckReturnValue;

/** Configuration fragment for Android rules that is specific to Bazel. */
@Immutable
@RequiresOptions(options = {BazelAndroidConfiguration.Options.class})
@CheckReturnValue
public class BazelAndroidConfiguration extends Fragment {

@Override
public boolean isImmutable() {
return true; // immutable and Starlark-hashable
}

/** Android configuration options that are specific to Bazel. */
public static class Options extends FragmentOptions {

@Option(
name = "merge_android_manifest_permissions",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
help =
"If enabled, the manifest merger will merge uses-permission and "
+ "uses-permission-sdk-23 attributes.")
public boolean mergeAndroidManifestPermissions;
}

private final boolean mergeAndroidManifestPermissions;

public BazelAndroidConfiguration(BuildOptions buildOptions) {
Options options = buildOptions.get(BazelAndroidConfiguration.Options.class);
this.mergeAndroidManifestPermissions = options.mergeAndroidManifestPermissions;
}

public boolean getMergeAndroidManifestPermissions() {
return this.mergeAndroidManifestPermissions;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public class ManifestMergerActionBuilder {
private Artifact manifest;
private Map<Artifact, Label> mergeeManifests;
private boolean isLibrary;
private boolean mergeManifestPermissions;
private Map<String, String> manifestValues;
private String customPackage;
private Artifact manifestOutput;
Expand All @@ -47,6 +48,11 @@ public ManifestMergerActionBuilder setLibrary(boolean isLibrary) {
return this;
}

public ManifestMergerActionBuilder setMergeManifestPermissions(boolean mergeManifestPermissions) {
this.mergeManifestPermissions = mergeManifestPermissions;
return this;
}

public ManifestMergerActionBuilder setManifestValues(Map<String, String> manifestValues) {
this.manifestValues = manifestValues;
return this;
Expand Down Expand Up @@ -80,6 +86,10 @@ public void build(AndroidDataContext dataContext) {
mergeeManifests.keySet());
}

if (mergeManifestPermissions) {
builder.addFlag("--mergeManifestPermissions");
}

builder
.maybeAddFlag("--mergeType", isLibrary)
.maybeAddFlag("LIBRARY", isLibrary)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import static com.google.common.truth.Truth.assertThat;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.Assert.assertThrows;

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

@Test
public void testMergeManifestWithBrokenManifestSyntax() throws Exception {
String dataDir =
Paths.get(System.getenv("TEST_WORKSPACE"), System.getenv("TEST_BINARY"))
.resolveSibling("testing/manifestmerge")
.toString()
.replace('\\', '/');
Files.createDirectories(working.resolve("output"));
final Path mergedManifest = working.resolve("output/mergedManifest.xml");
final Path brokenMergerManifest = rlocation(dataDir + "/brokenManifest/AndroidManifest.xml");
assertThat(brokenMergerManifest.toFile().exists()).isTrue();

AndroidManifestProcessor.ManifestProcessingException e =
assertThrows(
AndroidManifestProcessor.ManifestProcessingException.class,
() -> {
ManifestMergerAction.main(
generateArgs(
brokenMergerManifest,
ImmutableMap.of(),
false, /* isLibrary */
ImmutableMap.of("applicationId", "com.google.android.apps.testapp"),
"", /* custom_package */
mergedManifest,
false /* mergeManifestPermissions */)
.toArray(new String[0]));
});
assertThat(e)
.hasMessageThat()
.contains(
"com.android.manifmerger.ManifestMerger2$MergeFailureException: "
+ "org.xml.sax.SAXParseException; lineNumber: 6; columnNumber: 6; "
+ "The markup in the document following the root element must be well-formed.");
assertThat(mergedManifest.toFile().exists()).isFalse();
}

@Test
public void testMerge_GenerateDummyManifest() throws Exception {
Files.createDirectories(working.resolve("output"));
Expand Down Expand Up @@ -159,7 +196,8 @@ public void testMerge_GenerateDummyManifest() throws Exception {
false, /* isLibrary */
ImmutableMap.of("applicationId", "com.google.android.apps.testapp"),
"", /* custom_package */
mergedManifest);
mergedManifest,
/* mergeManifestPermissions */ false);
ManifestMergerAction.main(args.toArray(new String[0]));

assertThat(
Expand All @@ -174,6 +212,64 @@ public void testMerge_GenerateDummyManifest() throws Exception {
.trim());
}

@Test
public void testMergeWithMergePermissionsEnabled() throws Exception {
// Largely copied from testMerge() above. Perhaps worth combining the two test methods into one
// method in the future?
String dataDir =
Paths.get(System.getenv("TEST_WORKSPACE"), System.getenv("TEST_BINARY"))
.resolveSibling("testing/manifestmerge")
.toString()
.replace("\\", "/");

final Path mergerManifest = rlocation(dataDir + "/merger/AndroidManifest.xml");
final Path mergeeManifestOne = rlocation(dataDir + "/mergeeOne/AndroidManifest.xml");
final Path mergeeManifestTwo = rlocation(dataDir + "/mergeeTwo/AndroidManifest.xml");
assertThat(mergerManifest.toFile().exists()).isTrue();
assertThat(mergeeManifestOne.toFile().exists()).isTrue();
assertThat(mergeeManifestTwo.toFile().exists()).isTrue();

// The following code retrieves the path of the only AndroidManifest.xml in the
// expected-merged-permission/manifests directory. Unfortunately, this test runs
// internally and externally and the files have different names.
final File expectedManifestDirectory =
mergerManifest.getParent().resolveSibling("expected-merged-permissions").toFile();
assertThat(expectedManifestDirectory.exists()).isTrue();
final String[] debug =
expectedManifestDirectory.list(new PatternFilenameFilter(".*AndroidManifest\\.xml$"));
assertThat(debug).isNotNull();
final File[] expectedManifestDirectoryManifests =
expectedManifestDirectory.listFiles((File dir, String name) -> true);
assertThat(expectedManifestDirectoryManifests).isNotNull();
assertThat(expectedManifestDirectoryManifests).hasLength(1);
final Path expectedManifest = expectedManifestDirectoryManifests[0].toPath();

Files.createDirectories(working.resolve("output"));
final Path mergedManifest = working.resolve("output/mergedManifest.xml");

List<String> args =
generateArgs(
mergerManifest,
ImmutableMap.of(mergeeManifestOne, "mergeeOne", mergeeManifestTwo, "mergeeTwo"),
false, /* isLibrary */
ImmutableMap.of("applicationId", "com.google.android.apps.testapp"),
"", /* custom_package */
mergedManifest,
/* mergeManifestPermissions */ true);
ManifestMergerAction.main(args.toArray(new String[0]));

assertThat(
Joiner.on(" ")
.join(Files.readAllLines(mergedManifest, UTF_8))
.replaceAll("\\s+", " ")
.trim())
.isEqualTo(
Joiner.on(" ")
.join(Files.readAllLines(expectedManifest, UTF_8))
.replaceAll("\\s+", " ")
.trim());
}

@Test public void fullIntegration() throws Exception {
Files.createDirectories(working.resolve("output"));
final Path binaryOutput = working.resolve("output/binaryManifest.xml");
Expand All @@ -198,8 +294,15 @@ public void testMerge_GenerateDummyManifest() throws Exception {
.getManifest();

// libFoo manifest merging
List<String> args = generateArgs(libFooManifest, ImmutableMap.<Path, String>of(), true,
ImmutableMap.<String, String>of(), "", libFooOutput);
List<String> args =
generateArgs(
libFooManifest,
ImmutableMap.<Path, String>of(),
true,
ImmutableMap.<String, String>of(),
"",
libFooOutput,
false);
ManifestMergerAction.main(args.toArray(new String[0]));
assertThat(Joiner.on(" ")
.join(Files.readAllLines(libFooOutput, UTF_8))
Expand All @@ -211,8 +314,15 @@ public void testMerge_GenerateDummyManifest() throws Exception {
+ "</manifest>");

// libBar manifest merging
args = generateArgs(libBarManifest, ImmutableMap.<Path, String>of(), true,
ImmutableMap.<String, String>of(), "com.google.libbar", libBarOutput);
args =
generateArgs(
libBarManifest,
ImmutableMap.<Path, String>of(),
true,
ImmutableMap.<String, String>of(),
"com.google.libbar",
libBarOutput,
false);
ManifestMergerAction.main(args.toArray(new String[0]));
assertThat(Joiner.on(" ")
.join(Files.readAllLines(libBarOutput, UTF_8))
Expand All @@ -235,7 +345,8 @@ public void testMerge_GenerateDummyManifest() throws Exception {
"applicationId", "com.google.android.app",
"foo", "this \\\\: is \"a, \"bad string"),
/* customPackage= */ "",
binaryOutput);
binaryOutput,
/* mergeManifestPermissions */ false);
ManifestMergerAction.main(args.toArray(new String[0]));
assertThat(Joiner.on(" ")
.join(Files.readAllLines(binaryOutput, UTF_8))
Expand All @@ -255,14 +366,26 @@ private List<String> generateArgs(
boolean library,
Map<String, String> manifestValues,
String customPackage,
Path manifestOutput) {
return ImmutableList.of(
Path manifestOutput,
boolean mergeManifestPermissions) {
ImmutableList.Builder<String> builder = ImmutableList.builder();
builder.add(
"--manifest", manifest.toString(),
"--mergeeManifests", mapToDictionaryString(mergeeManifests),
"--mergeType", library ? "LIBRARY" : "APPLICATION",
"--manifestValues", mapToDictionaryString(manifestValues),
"--customPackage", customPackage,
"--manifestOutput", manifestOutput.toString());
"--mergeeManifests", mapToDictionaryString(mergeeManifests));
if (mergeManifestPermissions) {
builder.add("--mergeManifestPermissions");
}

builder.add(
"--mergeType",
library ? "LIBRARY" : "APPLICATION",
"--manifestValues",
mapToDictionaryString(manifestValues),
"--customPackage",
customPackage,
"--manifestOutput",
manifestOutput.toString());
return builder.build();
}

private <K, V> String mapToDictionaryString(Map<K, V> map) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ filegroup(
filegroup(
name = "test_data",
srcs = [
"brokenManifest/AndroidManifest.xml",
"expected-merged-permissions/AndroidManifest.xml",
"expected/AndroidManifest.xml",
"mergeeOne/AndroidManifest.xml",
"mergeeTwo/AndroidManifest.xml",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="com.example.bazel"
android:versionCode="1"
android:versionName="1.0"/>

<uses-sdk
android:minSdkVersion="21"
android:targetSdkVersion="26"/>

</manifest>
Loading

0 comments on commit 3ea9eb2

Please sign in to comment.