Add the ability to sort manifests in dependency order instead of path order.
Currently, mergee manifests are listed in path order. However, this may result
in erroneous conflicts. Dependency order, with the manifests of dependencies
listed after the manifests of the targets which depend on them, allows for
proper overriding.
Note that this just uses naive link order. Technically, this is incorrect...
But changing these nested sets to use proper link order is a change for another
day. And someone more familiar with the Android rules.
RELNOTES: None.
PiperOrigin-RevId: 227758377
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
index e71abda..e3d8d34 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
@@ -77,6 +77,14 @@
}
}
+ /** Converter for {@link ManifestMergerOrder} */
+ public static final class ManifestMergerOrderConverter
+ extends EnumConverter<ManifestMergerOrder> {
+ public ManifestMergerOrderConverter() {
+ super(ManifestMergerOrder.class, "android manifest merger order");
+ }
+ }
+
/** Converter for {@link AndroidAaptVersion} */
public static final class AndroidAaptConverter extends EnumConverter<AndroidAaptVersion> {
public AndroidAaptConverter() {
@@ -171,6 +179,14 @@
}
}
+ /** Orders for merging android manifests. */
+ public enum ManifestMergerOrder {
+ /** Manifests are sorted alphabetically by exec path. */
+ ALPHABETICAL,
+ /** Library manifests come before the manifests of their dependencies. */
+ DEPENDENCY;
+ }
+
/** Types of android manifest mergers. */
public enum AndroidAaptVersion {
AAPT,
@@ -668,6 +684,22 @@
public AndroidManifestMerger manifestMerger;
@Option(
+ name = "android_manifest_merger_order",
+ documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
+ effectTags = {
+ OptionEffectTag.ACTION_COMMAND_LINES,
+ OptionEffectTag.EXECUTION,
+ },
+ defaultValue = "alphabetical",
+ converter = ManifestMergerOrderConverter.class,
+ help =
+ "Sets the order of manifests passed to the manifest merger for Android binaries. "
+ + "ALPHABETICAL means manifests are sorted by path relative to the execroot. "
+ + "DEPENDENCY means manifests are ordered with each library's manifest coming "
+ + "before the manifests of its dependencies.")
+ public ManifestMergerOrder manifestMergerOrder;
+
+ @Option(
name = "android_aapt",
defaultValue = "auto",
documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
@@ -930,6 +962,7 @@
host.dexoptsSupportedInDexSharder = dexoptsSupportedInDexSharder;
host.useWorkersWithDexbuilder = useWorkersWithDexbuilder;
host.manifestMerger = manifestMerger;
+ host.manifestMergerOrder = manifestMergerOrder;
host.androidAaptVersion = androidAaptVersion;
host.allowAndroidLibraryDepsWithoutSrcs = allowAndroidLibraryDepsWithoutSrcs;
host.oneVersionEnforcementUseTransitiveJarsForBinaryUnderTest =
@@ -978,6 +1011,7 @@
private final boolean useAndroidResourceShrinking;
private final boolean useAndroidResourceCycleShrinking;
private final AndroidManifestMerger manifestMerger;
+ private final ManifestMergerOrder manifestMergerOrder;
private final ApkSigningMethod apkSigningMethod;
private final boolean useSingleJarApkBuilder;
private final boolean compressJavaResources;
@@ -1023,6 +1057,7 @@
options.useAndroidResourceShrinking || options.useExperimentalAndroidResourceShrinking;
this.useAndroidResourceCycleShrinking = options.useAndroidResourceCycleShrinking;
this.manifestMerger = options.manifestMerger;
+ this.manifestMergerOrder = options.manifestMergerOrder;
this.apkSigningMethod = options.apkSigningMethod;
this.useSingleJarApkBuilder = options.useSingleJarApkBuilder;
this.useRexToCompressDexFiles = options.useRexToCompressDexFiles;
@@ -1181,6 +1216,10 @@
return manifestMerger;
}
+ public ManifestMergerOrder getManifestMergerOrder() {
+ return manifestMergerOrder;
+ }
+
public ApkSigningMethod getApkSigningMethod() {
return apkSigningMethod;
}
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 031129e..1e41245 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
@@ -15,6 +15,7 @@
import static com.google.common.base.Strings.isNullOrEmpty;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedMap;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.RuleContext;
@@ -220,7 +221,10 @@
ResourceDependencies resourceDeps,
Map<String, String> manifestValues,
@Nullable String manifestMerger) {
- Map<Artifact, Label> mergeeManifests = getMergeeManifests(resourceDeps.getResourceContainers());
+ Map<Artifact, Label> mergeeManifests =
+ getMergeeManifests(
+ resourceDeps.getResourceContainers(),
+ dataContext.getAndroidConfig().getManifestMergerOrder());
Artifact newManifest;
if (useLegacyMerging(errorConsumer, dataContext.getAndroidConfig(), manifestMerger)) {
@@ -272,14 +276,17 @@
}
private static Map<Artifact, Label> getMergeeManifests(
- Iterable<ValidatedAndroidResources> transitiveData) {
- ImmutableSortedMap.Builder<Artifact, Label> builder =
- ImmutableSortedMap.orderedBy(Artifact.EXEC_PATH_COMPARATOR);
+ Iterable<ValidatedAndroidResources> transitiveData,
+ AndroidConfiguration.ManifestMergerOrder manifestMergerOrder) {
+ ImmutableMap.Builder<Artifact, Label> builder = new ImmutableMap.Builder<>();
for (ValidatedAndroidResources d : transitiveData) {
if (d.isManifestExported()) {
builder.put(d.getManifest(), d.getLabel());
}
}
+ if (AndroidConfiguration.ManifestMergerOrder.ALPHABETICAL.equals(manifestMergerOrder)) {
+ return ImmutableSortedMap.copyOf(builder.build(), Artifact.EXEC_PATH_COMPARATOR);
+ }
return builder.build();
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java
index e91732c..44188ee 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ResourceDependencies.java
@@ -250,8 +250,8 @@
newDirectResource.getProcessedManifest().toProvider(),
newDirectResource.getRTxt(),
NestedSetBuilder.<ValidatedAndroidResources>naiveLinkOrder()
- .addTransitive(transitiveResourceContainers)
.addTransitive(directResourceContainers)
+ .addTransitive(transitiveResourceContainers)
.build(),
NestedSetBuilder.<ValidatedAndroidResources>naiveLinkOrder()
.add(newDirectResource.export())
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
index 9cfc91d..4a2dfb1 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
@@ -27,8 +27,10 @@
import com.google.common.base.Joiner;
import com.google.common.base.Predicate;
import com.google.common.base.Predicates;
+import com.google.common.base.Splitter;
import com.google.common.collect.Collections2;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
@@ -62,6 +64,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
+import java.util.Map;
import java.util.Set;
import org.junit.Before;
import org.junit.Test;
@@ -4492,4 +4495,173 @@
"libal_bottom_for_deps-src.jar",
"libfoo_app-src.jar");
}
+
+ /** Gets the map of mergee manifests in the order specified on the command line. */
+ private Map<String, String> getBinaryMergeeManifests(ConfiguredTarget target) throws Exception {
+ Artifact processedManifest = target.get(ApkInfo.PROVIDER).getMergedManifest();
+ List<String> processingActionArgs = getGeneratingSpawnActionArgs(processedManifest);
+ assertThat(processingActionArgs).contains("--primaryData");
+ String primaryData =
+ processingActionArgs.get(processingActionArgs.indexOf("--primaryData") + 1);
+ String mergedManifestExecPathString = Splitter.on(":").splitToList(primaryData).get(2);
+ SpawnAction processingAction = getGeneratingSpawnAction(processedManifest);
+ Artifact mergedManifest =
+ Iterables.find(
+ processingAction.getInputs(),
+ (artifact) -> artifact.getExecPath().toString().equals(mergedManifestExecPathString));
+ List<String> mergeArgs = getGeneratingSpawnActionArgs(mergedManifest);
+ assertThat(mergeArgs).contains("--mergeeManifests");
+ Map<String, String> splitData =
+ Splitter.on(",")
+ .withKeyValueSeparator(Splitter.onPattern("(?<!\\\\):"))
+ .split(mergeArgs.get(mergeArgs.indexOf("--mergeeManifests") + 1));
+ ImmutableMap.Builder<String, String> results = new ImmutableMap.Builder<>();
+ for (Map.Entry<String, String> manifestAndLabel : splitData.entrySet()) {
+ results.put(manifestAndLabel.getKey(), manifestAndLabel.getValue().replace("\\:", ":"));
+ }
+ return results.build();
+ }
+
+ private Artifact getLibraryManifest(ConfiguredTarget target) throws Exception {
+ if (target.get(AndroidManifestInfo.PROVIDER) != null) {
+ return target.get(AndroidManifestInfo.PROVIDER).getManifest();
+ }
+ return null;
+ }
+
+ @Test
+ public void androidManifestMergerOrderAlphabetical_MergeesSortedByExecPath() throws Exception {
+ // Hack: Avoid the Android split transition by turning off fat_apk_cpu/android_cpu.
+ useConfiguration(
+ "--fat_apk_cpu=", "--android_cpu=", "--android_manifest_merger_order=alphabetical");
+ scratch.overwriteFile(
+ "java/android/BUILD",
+ "android_library(",
+ " name = 'core',",
+ " manifest = 'core/AndroidManifest.xml',",
+ " exports_manifest = 1,",
+ " resource_files = ['core/res/values/strings.xml'],",
+ ")",
+ "android_library(",
+ " name = 'utility',",
+ " manifest = 'utility/AndroidManifest.xml',",
+ " exports_manifest = 1,",
+ " resource_files = ['utility/res/values/values.xml'],",
+ " deps = ['//java/common:common'],",
+ ")");
+ scratch.file(
+ "java/binary/BUILD",
+ "android_binary(",
+ " name = 'application',",
+ " srcs = ['App.java'],",
+ " manifest = 'app/AndroidManifest.xml',",
+ " deps = [':library'],",
+ ")",
+ "android_library(",
+ " name = 'library',",
+ " manifest = 'library/AndroidManifest.xml',",
+ " exports_manifest = 1,",
+ " deps = ['//java/common:theme', '//java/android:utility'],",
+ ")");
+ scratch.file(
+ "java/common/BUILD",
+ "android_library(",
+ " name = 'common',",
+ " manifest = 'common/AndroidManifest.xml',",
+ " exports_manifest = 1,",
+ " resource_files = ['common/res/values/common.xml'],",
+ " deps = ['//java/android:core'],",
+ ")",
+ "android_library(",
+ " name = 'theme',",
+ " manifest = 'theme/AndroidManifest.xml',",
+ " exports_manifest = 1,",
+ " resource_files = ['theme/res/values/values.xml'],",
+ ")");
+ Artifact androidCoreManifest = getLibraryManifest(getConfiguredTarget("//java/android:core"));
+ Artifact androidUtilityManifest =
+ getLibraryManifest(getConfiguredTarget("//java/android:utility"));
+ Artifact binaryLibraryManifest =
+ getLibraryManifest(getConfiguredTarget("//java/binary:library"));
+ Artifact commonManifest = getLibraryManifest(getConfiguredTarget("//java/common:common"));
+ Artifact commonThemeManifest = getLibraryManifest(getConfiguredTarget("//java/common:theme"));
+
+ assertThat(getBinaryMergeeManifests(getConfiguredTarget("//java/binary:application")))
+ .containsExactlyEntriesIn(
+ ImmutableMap.of(
+ androidCoreManifest.getExecPath().toString(), "//java/android:core",
+ androidUtilityManifest.getExecPath().toString(), "//java/android:utility",
+ binaryLibraryManifest.getExecPath().toString(), "//java/binary:library",
+ commonManifest.getExecPath().toString(), "//java/common:common",
+ commonThemeManifest.getExecPath().toString(), "//java/common:theme"))
+ .inOrder();
+ }
+
+ @Test
+ public void androidManifestMergerOrderDependencies_MergeesSortedByDepOrder() throws Exception {
+ // Hack: Avoid the Android split transition by turning off fat_apk_cpu/android_cpu.
+ useConfiguration(
+ "--fat_apk_cpu=", "--android_cpu=", "--android_manifest_merger_order=dependency");
+ scratch.overwriteFile(
+ "java/android/BUILD",
+ "android_library(",
+ " name = 'core',",
+ " manifest = 'core/AndroidManifest.xml',",
+ " exports_manifest = 1,",
+ " resource_files = ['core/res/values/strings.xml'],",
+ ")",
+ "android_library(",
+ " name = 'utility',",
+ " manifest = 'utility/AndroidManifest.xml',",
+ " exports_manifest = 1,",
+ " resource_files = ['utility/res/values/values.xml'],",
+ " deps = ['//java/common:common'],",
+ ")");
+ scratch.file(
+ "java/binary/BUILD",
+ "android_binary(",
+ " name = 'application',",
+ " srcs = ['App.java'],",
+ " manifest = 'app/AndroidManifest.xml',",
+ " deps = [':library'],",
+ ")",
+ "android_library(",
+ " name = 'library',",
+ " manifest = 'library/AndroidManifest.xml',",
+ " exports_manifest = 1,",
+ " deps = ['//java/common:theme', '//java/android:utility'],",
+ ")");
+ scratch.file(
+ "java/common/BUILD",
+ "android_library(",
+ " name = 'common',",
+ " manifest = 'common/AndroidManifest.xml',",
+ " exports_manifest = 1,",
+ " resource_files = ['common/res/values/common.xml'],",
+ " deps = ['//java/android:core'],",
+ ")",
+ "android_library(",
+ " name = 'theme',",
+ " manifest = 'theme/AndroidManifest.xml',",
+ " exports_manifest = 1,",
+ " resource_files = ['theme/res/values/values.xml'],",
+ ")");
+ Artifact androidCoreManifest = getLibraryManifest(getConfiguredTarget("//java/android:core"));
+ Artifact androidUtilityManifest =
+ getLibraryManifest(getConfiguredTarget("//java/android:utility"));
+ Artifact binaryLibraryManifest =
+ getLibraryManifest(getConfiguredTarget("//java/binary:library"));
+ Artifact commonManifest = getLibraryManifest(getConfiguredTarget("//java/common:common"));
+ Artifact commonThemeManifest = getLibraryManifest(getConfiguredTarget("//java/common:theme"));
+
+ assertThat(getBinaryMergeeManifests(getConfiguredTarget("//java/binary:application")))
+ .containsExactlyEntriesIn(
+ ImmutableMap.of(
+ binaryLibraryManifest.getExecPath().toString(), "//java/binary:library",
+ commonThemeManifest.getExecPath().toString(), "//java/common:theme",
+ androidUtilityManifest.getExecPath().toString(), "//java/android:utility",
+ commonManifest.getExecPath().toString(), "//java/common:common",
+ androidCoreManifest.getExecPath().toString(), "//java/android:core"))
+ .inOrder();
+ }
}
