diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java index 5fae569920e950..e03583e0768a57 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java @@ -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; @@ -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(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java index 630c1fdb7d169f..ef6036539dcfa6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java @@ -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. @@ -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; diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidManifest.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidManifest.java index ed9d097fa7a56d..b0ebd5224ec930 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidManifest.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidManifest.java @@ -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(); @@ -196,6 +208,7 @@ public StampedAndroidManifest stamp(AndroidDataContext dataContext) { new ManifestMergerActionBuilder() .setManifest(manifest) .setLibrary(true) + .setMergeManifestPermissions(getMergeManifestPermissionsEnabled(dataContext)) .setCustomPackage(pkg) .setManifestOutput(outputManifest) .build(dataContext); @@ -240,6 +253,7 @@ public StampedAndroidManifest mergeWithDeps( .setManifest(manifest) .setMergeeManifests(mergeeManifests) .setLibrary(false) + .setMergeManifestPermissions(getMergeManifestPermissionsEnabled(dataContext)) .setManifestValues(manifestValues) .setCustomPackage(pkg) .setManifestOutput(newManifest) diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/BazelAndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/BazelAndroidConfiguration.java new file mode 100644 index 00000000000000..7b7559b4e6b5db --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/android/BazelAndroidConfiguration.java @@ -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; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java index 29a92f1e4234d5..e8ef4a7bfcdb99 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java @@ -27,6 +27,7 @@ public class ManifestMergerActionBuilder { private Artifact manifest; private Map mergeeManifests; private boolean isLibrary; + private boolean mergeManifestPermissions; private Map manifestValues; private String customPackage; private Artifact manifestOutput; @@ -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 manifestValues) { this.manifestValues = manifestValues; return this; @@ -80,6 +86,10 @@ public void build(AndroidDataContext dataContext) { mergeeManifests.keySet()); } + if (mergeManifestPermissions) { + builder.addFlag("--mergeManifestPermissions"); + } + builder .maybeAddFlag("--mergeType", isLibrary) .maybeAddFlag("LIBRARY", isLibrary) diff --git a/src/test/java/com/google/devtools/build/android/ManifestMergerActionTest.java b/src/test/java/com/google/devtools/build/android/ManifestMergerActionTest.java index fda1cd88e16341..ee8bb7b53683de 100644 --- a/src/test/java/com/google/devtools/build/android/ManifestMergerActionTest.java +++ b/src/test/java/com/google/devtools/build/android/ManifestMergerActionTest.java @@ -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; @@ -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")); @@ -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( @@ -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 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"); @@ -198,8 +294,15 @@ public void testMerge_GenerateDummyManifest() throws Exception { .getManifest(); // libFoo manifest merging - List args = generateArgs(libFooManifest, ImmutableMap.of(), true, - ImmutableMap.of(), "", libFooOutput); + List args = + generateArgs( + libFooManifest, + ImmutableMap.of(), + true, + ImmutableMap.of(), + "", + libFooOutput, + false); ManifestMergerAction.main(args.toArray(new String[0])); assertThat(Joiner.on(" ") .join(Files.readAllLines(libFooOutput, UTF_8)) @@ -211,8 +314,15 @@ public void testMerge_GenerateDummyManifest() throws Exception { + ""); // libBar manifest merging - args = generateArgs(libBarManifest, ImmutableMap.of(), true, - ImmutableMap.of(), "com.google.libbar", libBarOutput); + args = + generateArgs( + libBarManifest, + ImmutableMap.of(), + true, + ImmutableMap.of(), + "com.google.libbar", + libBarOutput, + false); ManifestMergerAction.main(args.toArray(new String[0])); assertThat(Joiner.on(" ") .join(Files.readAllLines(libBarOutput, UTF_8)) @@ -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)) @@ -255,14 +366,26 @@ private List generateArgs( boolean library, Map manifestValues, String customPackage, - Path manifestOutput) { - return ImmutableList.of( + Path manifestOutput, + boolean mergeManifestPermissions) { + ImmutableList.Builder 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 String mapToDictionaryString(Map map) { diff --git a/src/test/java/com/google/devtools/build/android/testing/manifestmerge/BUILD b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/BUILD index 7d7dcc4c112a5a..79cd5c4f2bdb2a 100644 --- a/src/test/java/com/google/devtools/build/android/testing/manifestmerge/BUILD +++ b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/BUILD @@ -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", diff --git a/src/test/java/com/google/devtools/build/android/testing/manifestmerge/brokenManifest/AndroidManifest.xml b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/brokenManifest/AndroidManifest.xml new file mode 100644 index 00000000000000..fe650964e05333 --- /dev/null +++ b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/brokenManifest/AndroidManifest.xml @@ -0,0 +1,10 @@ + + + + + \ No newline at end of file diff --git a/src/test/java/com/google/devtools/build/android/testing/manifestmerge/expected-merged-permissions/AndroidManifest.xml b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/expected-merged-permissions/AndroidManifest.xml new file mode 100644 index 00000000000000..1cbf1982188b05 --- /dev/null +++ b/src/test/java/com/google/devtools/build/android/testing/manifestmerge/expected-merged-permissions/AndroidManifest.xml @@ -0,0 +1,107 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java b/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java index 64832bc6b9472d..29bab51628b110 100644 --- a/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java +++ b/src/tools/android/java/com/google/devtools/build/android/ManifestMergerAction.java @@ -62,6 +62,7 @@ * --manifestValues key value pairs of manifest overrides * --customPackage package to write for library manifest * --manifestOutput path to write output manifest + * --mergeManifestPermissions merge manifest uses-permissions * */ public class ManifestMergerAction { @@ -152,6 +153,15 @@ public static final class Options extends OptionsBase { help = "Path to where the merger log should be written." ) public Path log; + + @Option( + name = "mergeManifestPermissions", + defaultValue = "false", + category = "output", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, + help = "If enabled, manifest permissions will be merged.") + public boolean mergeManifestPermissions; } private static final String[] PERMISSION_TAGS = @@ -173,9 +183,8 @@ private static Path removePermissions(Path manifest, Path outputDir) } } } - // Write resulting manifest to the output directory, maintaining full path to prevent collisions - Path output = outputDir.resolve(manifest.toString().replaceFirst("^/", "")); - Files.createDirectories(output.getParent()); + // Write resulting manifest to a tmp file to prevent collisions + Path output = Files.createTempFile(outputDir, "AndroidManifest", ".xml"); TransformerFactory.newInstance() .newTransformer() .transform(new DOMSource(doc), new StreamResult(output.toFile())); @@ -195,13 +204,17 @@ public static void main(String[] args) throws Exception { Path mergedManifest; AndroidManifestProcessor manifestProcessor = AndroidManifestProcessor.with(stdLogger); - // Remove uses-permission tags from mergees before the merge. Path tmp = Files.createTempDirectory("manifest_merge_tmp"); tmp.toFile().deleteOnExit(); ImmutableMap.Builder mergeeManifests = ImmutableMap.builder(); for (Map.Entry mergeeManifest : options.mergeeManifests.entrySet()) { - mergeeManifests.put( - removePermissions(mergeeManifest.getKey(), tmp), mergeeManifest.getValue()); + if (!options.mergeManifestPermissions) { + // Remove uses-permission tags from mergees before the merge. + mergeeManifests.put( + removePermissions(mergeeManifest.getKey(), tmp), mergeeManifest.getValue()); + } else { + mergeeManifests.put(mergeeManifest); + } } Path manifest = options.manifest; @@ -229,10 +242,15 @@ public static void main(String[] args) throws Exception { Files.copy(manifest, options.manifestOutput, StandardCopyOption.REPLACE_EXISTING); } } catch (AndroidManifestProcessor.ManifestProcessingException e) { - // We special case ManifestProcessingExceptions here to indicate that this is - // caused by a build error, not an Bazel-internal error. - logger.log(SEVERE, "Error during merging manifests", e); - System.exit(1); // Don't duplicate the error to the user or bubble up the exception. + // ManifestProcessingExceptions represent build errors that should be delivered directly to + // ResourceProcessorBusyBox where the exception can be delivered with a non-zero status code + // to the worker/process + // Note that this exception handler is nearly identical to the generic case, except that it + // does not have a log print associated with it. This is because the exception will bubble up + // to ResourceProcessorBusyBox, which will print an identical error message. It is preferable + // to slightly convolute this try/catch block, rather than pollute the user's console with + // extra repeated error messages. + throw e; } catch (Exception e) { logger.log(SEVERE, "Error during merging manifests", e); throw e; // This is a proper internal exception, so we bubble it up.