Use exec path as a fallback when sorting by root-relative path.
ImmutableSortedMap uses the comparator it's given to check for uniqueness;
in android_local_test scenarios where the same manifest is picked up both
through the normal configuration and the Android configuration, this can
result in duplicates.
Instead of considering the two artifacts equal in this case, fall back to
comparing their configuration directories.
RELNOTES: None.
PiperOrigin-RevId: 234879527
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index d3113ec..03fd7b2 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -140,7 +140,14 @@
} else if (b == null) {
return 1;
} else {
- return a.rootRelativePath.compareTo(b.rootRelativePath);
+ int result = a.rootRelativePath.compareTo(b.rootRelativePath);
+ if (result == 0) {
+ // Use the full exec path as a fallback if the root-relative paths are the same, thus
+ // avoiding problems when ImmutableSortedMaps are switched from EXEC_PATH_COMPARATOR.
+ return a.execPath.compareTo(b.execPath);
+ } else {
+ return result;
+ }
}
};
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AbstractAndroidLocalTestTestBase.java b/src/test/java/com/google/devtools/build/lib/rules/android/AbstractAndroidLocalTestTestBase.java
index 9492eab..7c449e2 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AbstractAndroidLocalTestTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AbstractAndroidLocalTestTestBase.java
@@ -21,7 +21,6 @@
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
-import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import java.util.List;
import org.junit.Before;
import org.junit.Test;
@@ -34,7 +33,7 @@
* scratch.writeFile() and scratch.overwriteFile()).
*/
@RunWith(JUnit4.class)
-public abstract class AbstractAndroidLocalTestTestBase extends BuildViewTestCase {
+public abstract class AbstractAndroidLocalTestTestBase extends AndroidBuildViewTestCase {
@Before
public void setUp() throws Exception {
@@ -141,8 +140,8 @@
")");
ConfiguredTarget binary = getConfiguredTarget("//java/com/foo");
List<String> inputs =
- actionsTestUtil()
- .prettyArtifactNames(actionsTestUtil().artifactClosureOf(getFilesToBuild(binary)));
+ ActionsTestUtil.prettyArtifactNames(
+ actionsTestUtil().artifactClosureOf(getFilesToBuild(binary)));
assertThat(inputs).containsAllOf("java/com/foo/Flag1On.java", "java/com/foo/Flag2Off.java");
assertThat(inputs).containsNoneOf("java/com/foo/Flag1Off.java", "java/com/foo/Flag2On.java");
@@ -323,7 +322,7 @@
" transitive_configs = [':flag1', ':flag2'],",
")");
Artifact flagList =
- actionsTestUtil().getFirstArtifactEndingWith(
+ ActionsTestUtil.getFirstArtifactEndingWith(
actionsTestUtil()
.artifactClosureOf(getFilesToBuild(getConfiguredTarget("//java/com/foo"))),
"/FooFlags.java");
@@ -460,6 +459,86 @@
}
@Test
+ public void androidManifestMergerOrderAlphabeticalByConfiguration_MergeesSortedByPathInBinOrGen()
+ throws Exception {
+ useConfiguration(
+ "--fat_apk_cpu=k8", "--android_manifest_merger_order=alphabetical_by_configuration");
+ 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/test/BUILD",
+ "android_local_test(",
+ " name = 'test',",
+ " srcs = ['Test.java'],",
+ " manifest = 'testing/AndroidManifest.xml',",
+ " deps = ['//java/binary:application', '//java/binary:library'],",
+ ")");
+ 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'],",
+ ")");
+
+ ConfiguredTarget test = getConfiguredTarget("//java/test:test");
+ assertNoEvents();
+ assertThat(test).isNotNull();
+
+ // each entry appears twice because the application duplicates them all into another config;
+ // this is partially testing that doing so does not cause a crash because of matching
+ // root-relative paths.
+ assertThat(getLocalTestMergeeManifests(test).values())
+ .containsExactly(
+ "//java/android:core",
+ "//java/android:core",
+ "//java/android:utility",
+ "//java/android:utility",
+ "//java/binary:library",
+ "//java/binary:library",
+ "//java/common:common",
+ "//java/common:common",
+ "//java/common:theme",
+ "//java/common:theme")
+ .inOrder();
+ }
+
+ @Test
public void testDeployJar() throws Exception {
writeFile(
"java/com/google/android/foo/BUILD",
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 0d28e01..6f39a89 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,7 +27,6 @@
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;
@@ -65,7 +64,6 @@
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;
@@ -4497,39 +4495,6 @@
"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.
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBuildViewTestCase.java
index 28dd2dc..2e2cdbd 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBuildViewTestCase.java
@@ -25,6 +25,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
+import com.google.common.collect.MoreCollectors;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.Artifact;
@@ -48,6 +49,7 @@
import java.util.Collection;
import java.util.Iterator;
import java.util.List;
+import java.util.Map;
import java.util.Set;
import javax.annotation.Nullable;
@@ -235,6 +237,54 @@
.getJavaClassJar();
}
+ protected Map<String, String> getBinaryMergeeManifests(ConfiguredTarget target) throws Exception {
+ return getMergeeManifests(target.get(ApkInfo.PROVIDER).getMergedManifest());
+ }
+
+ protected Map<String, String> getLocalTestMergeeManifests(ConfiguredTarget target)
+ throws Exception {
+ return getMergeeManifests(
+ ImmutableList.copyOf(collectRunfiles(target)).stream()
+ .filter(
+ (artifact) ->
+ artifact.getFilename().equals("AndroidManifest.xml")
+ && artifact.getOwnerLabel().equals(target.getLabel()))
+ .collect(MoreCollectors.onlyElement()));
+ }
+
+ /** Gets the map of mergee manifests in the order specified on the command line. */
+ protected Map<String, String> getMergeeManifests(Artifact processedManifest) throws Exception {
+ 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();
+ }
+
+ /** Gets the processed manifest exported by the given library. */
+ protected Artifact getLibraryManifest(ConfiguredTarget target) throws Exception {
+ if (target.get(AndroidManifestInfo.PROVIDER) != null) {
+ return target.get(AndroidManifestInfo.PROVIDER).getManifest();
+ }
+ return null;
+ }
+
// Returns an artifact that will be generated when a rule has assets that are processed seperately
static Artifact getDecoupledAssetArtifact(ConfiguredTarget target) {
return target.get(AndroidAssetsInfo.PROVIDER).getValidationResult();
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/BUILD b/src/test/java/com/google/devtools/build/lib/rules/android/BUILD
index 32936e2..d9c9e7f 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/BUILD
@@ -326,6 +326,7 @@
name = "AbstractAndroidLocalTestTestBase",
srcs = ["AbstractAndroidLocalTestTestBase.java"],
deps = [
+ ":AndroidBuildViewTestCase",
"//src/main/java/com/google/devtools/build/lib:build-base",
"//src/main/java/com/google/devtools/build/lib:util",
"//src/main/java/com/google/devtools/build/lib/actions",
