Clean up Android rules and tests for Android Platforms.

In some cases, this means adding missing dependencies. In many others, it means test cleanups. Part of bazelbuild#16285. PiperOrigin-RevId: 570765426 Change-Id: I36763a45699a37e54b3823be8f70f59d54269ca6
katre · Oct 10, 2023 · 040dcc0 · 040dcc0
1 parent 79df679
commit 040dcc0
Show file tree

Hide file tree

Showing 12 changed files with 49 additions and 81 deletions.
diff --git a/...ain/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidLocalTestRule.java b/...ain/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidLocalTestRule.java
@@ -93,14 +93,15 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
   }
 
   @Override
-  public Metadata getMetadata() {
+  public RuleDefinition.Metadata getMetadata() {
     return RuleDefinition.Metadata.builder()
         .name("android_local_test")
         .type(RuleClassType.TEST)
         .ancestors(
             AndroidLocalTestBaseRule.class,
             BaseJavaBinaryRule.class,
-            BaseRuleClasses.TestBaseRule.class)
+            BaseRuleClasses.TestBaseRule.class,
+            BazelSdkToolchainRule.class)
         .factoryClass(BazelAndroidLocalTest.class)
         .build();
   }

diff --git a/...est/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidLocalTestTest.java b/...est/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidLocalTestTest.java
@@ -34,13 +34,13 @@ public static class WithoutPlatforms extends BazelAndroidLocalTestTest {}
   // TODO(b/161709111): With platforms, all tests fail with
   // "no attribute `$android_sdk_toolchain_type`" on AspectAwareAttributeMapper.
   /** Use platform-based toolchain resolution. */
-  /*  @RunWith(JUnit4.class)
-  public static class WithPlatforms extends GoogleAndroidLocalTestTest {
+  @RunWith(JUnit4.class)
+  public static class WithPlatforms extends BazelAndroidLocalTestTest {
     @Override
     protected boolean platformBasedToolchains() {
       return true;
     }
-  } */
+  }
 
   @Before
   @Override

diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AarImportTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AarImportTest.java
@@ -607,7 +607,7 @@ public void testFailsWithoutAndroidSdk() throws Exception {
     checkError(
         "aar",
         "aar",
-        platformBasedToolchains()
+        platformBasedToolchains() // Error changes based on the flag used
             ? "resolved to target //sdk:sdk, but that target does not provide ToolchainInfo"
             : "No Android SDK found. Use the --android_sdk command line option to specify one.",
         "aar_import(",

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
@@ -214,39 +214,36 @@ public void setup() throws Exception {
         "java/android/res/values/strings.xml",
         "<resources><string name = 'hello'>Hello Android!</string></resources>");
     scratch.file("java/android/A.java", "package android; public class A {};");
-    if (platformBasedToolchains()) {
-      scratch.file(
-          "java/android/platforms/BUILD",
-          "platform(",
-          "    name = 'x86',",
-          "    parents = ['" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "android:armeabi-v7a'],",
-          "    constraint_values = ['" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "cpu:x86_32'],",
-          ")",
-          "platform(",
-          "    name = 'armeabi-v7a',",
-          "    parents = ['" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "android:armeabi-v7a'],",
-          "    constraint_values = ['" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "cpu:armv7'],",
-          ")");
-      scratch.file(
-          "/workspace/platform_mappings",
-          "platforms:",
-          "  //java/android/platforms:armeabi-v7a",
-          "    --cpu=armeabi-v7a",
-          "    --android_cpu=armeabi-v7a",
-          "    --crosstool_top=//android/crosstool:everything",
-          "  //java/android/platforms:x86",
-          "    --cpu=x86",
-          "    --android_cpu=x86",
-          "    --crosstool_top=//android/crosstool:everything",
-          "flags:",
-          "  --crosstool_top=//android/crosstool:everything",
-          "  --cpu=armeabi-v7a",
-          "    //java/android/platforms:armv7",
-          "  --crosstool_top=//android/crosstool:everything",
-          "  --cpu=x86",
-          "    //java/android/platforms:x86");
-      invalidatePackages(false);
-    }
+    scratch.file(
+        "java/android/platforms/BUILD",
+        "platform(",
+        "    name = 'x86',",
+        "    parents = ['" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "android:armeabi-v7a'],",
+        "    constraint_values = ['" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "cpu:x86_32'],",
+        ")",
+        "platform(",
+        "    name = 'armeabi-v7a',",
+        "    parents = ['" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "android:armeabi-v7a'],",
+        "    constraint_values = ['" + TestConstants.CONSTRAINTS_PACKAGE_ROOT + "cpu:armv7'],",
+        ")");
+    scratch.file(
+        "/workspace/platform_mappings",
+        "platforms:",
+        "  //java/android/platforms:armeabi-v7a",
+        "    --cpu=armeabi-v7a",
+        "    --android_cpu=armeabi-v7a",
+        "    --crosstool_top=//android/crosstool:everything",
+        "  //java/android/platforms:x86",
+        "    --cpu=x86",
+        "    --android_cpu=x86",
+        "    --crosstool_top=//android/crosstool:everything",
+        "flags:",
+        "  --crosstool_top=//android/crosstool:everything",
+        "  --cpu=armeabi-v7a",
+        "    //java/android/platforms:armv7",
+        "  --crosstool_top=//android/crosstool:everything",
+        "  --cpu=x86",
+        "    //java/android/platforms:x86");
     setBuildLanguageOptions("--experimental_google_legacy_api");
   }
 

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
@@ -86,6 +86,7 @@ protected void useConfiguration(ImmutableMap<String, Object> starlarkOptions, St
           starlarkOptions,
           ImmutableList.builder()
               .add((Object[]) args)
+              .add("--noincompatible_enable_android_toolchain_resolution")
               .add("--noincompatible_enable_cc_toolchain_resolution")
               .build()
               .toArray(new String[0]));
@@ -113,6 +114,7 @@ protected void useConfiguration(ImmutableMap<String, Object> starlarkOptions, St
             String.format("    toolchain = '%s',", sdkLabel),
             ")");
         fullArgs.add("--extra_toolchains=//legacy_to_platform_sdk:custom_sdk_toolchain");
+        fullArgs.add(arg);
       } else {
         fullArgs.add(arg);
       }

diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidDataBindingV2Test.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidDataBindingV2Test.java
@@ -1163,12 +1163,6 @@ public void dataBinding_androidLocalTest_dataBindingDisabled_doesNotUseDataBindi
     writeDataBindingFiles();
     writeNonDataBindingLocalTestFiles();
 
-    if (platformBasedToolchains()) {
-      // TODO(b/161709111): With platforms, the below fails with
-      // "no attribute `$android_sdk_toolchain_type`" on AspectAwareAttributeMapper.
-      return;
-    }
-
     ConfiguredTarget testTarget =
         getConfiguredTarget("//javatests/android/test:databinding_enabled_test");
     Set<Artifact> allArtifacts = actionsTestUtil().artifactClosureOf(getFilesToBuild(testTarget));
@@ -1200,12 +1194,6 @@ public void dataBinding_androidLocalTest_dataBindingEnabled_usesDataBindingFlags
     writeDataBindingFiles();
     writeDataBindingLocalTestFiles();
 
-    if (platformBasedToolchains()) {
-      // TODO(b/161709111): With platforms, the below fails with
-      // "no attribute `$android_sdk_toolchain_type`" on AspectAwareAttributeMapper.
-      return;
-    }
-
     ConfiguredTarget testTarget =
         getConfiguredTarget("//javatests/android/test:databinding_enabled_test");
     Set<Artifact> allArtifacts = actionsTestUtil().artifactClosureOf(getFilesToBuild(testTarget));

diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLibraryTest.java
@@ -1935,11 +1935,6 @@ public void aapt2ArtifactGenerationWhenSdkIsDefined() throws Exception {
             getImplicitOutputArtifact(a, AndroidRuleClasses.ANDROID_LIBRARY_APK));
     assertThat(linkAction).isNotNull();
 
-    if (platformBasedToolchains()) {
-      // TODO(b/161709111): With platform, the call to sdk below produces a NullPointerException.
-      return;
-    }
-
     assertThat(linkAction.getInputs().toList())
         .containsAtLeast(
             sdk.get(AndroidSdkProvider.PROVIDER).getAndroidJar(),

diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkTest.java
@@ -338,24 +338,9 @@ public void testAndroidSdkConfigurationField() throws Exception {
         "load('//:foo_library.bzl', 'foo_library')",
         "filegroup(name = 'new_sdk')",
         "foo_library(name = 'lib')");
-    if (platformBasedToolchains()) {
-      // TODO(b/161709111): fails to find a matching Android toolchain.
-      if (true) {
-        return;
-      }
-      scratch.file(
-          "platform_toolchain_defs/BUILD",
-          "toolchain(",
-          "    name = 'new_sdk_toolchain',",
-          String.format("    toolchain_type = '%s',", TestConstants.ANDROID_TOOLCHAIN_TYPE_LABEL),
-          "toolchain = '//:new_sdk',",
-          ")");
-      useConfiguration(
-          "--extra_toolchains=//platform_toolchain_defs:new_sdk_toolchain",
-          "--android_sdk=//:new_sdk");
-    } else {
-      useConfiguration("--android_sdk=//:new_sdk");
-    }
+
+    // This test doesn't touch platforms, it directly reads the --android_sdk flag value.
+    useConfiguration("--android_sdk=//:new_sdk");
 
     ConfiguredTarget ct = getConfiguredTarget("//:lib");
     assertThat(getMyInfoFromTarget(ct).getValue("foo"))

diff --git a/src/test/shell/bazel/android/android_helper.sh b/src/test/shell/bazel/android/android_helper.sh
@@ -115,7 +115,6 @@ function resolve_android_toolchains() {
     echo "This test uses platform-based Android toolchain resolution."
     add_to_bazelrc "build --incompatible_enable_android_toolchain_resolution"
     add_to_bazelrc "build --incompatible_enable_cc_toolchain_resolution"
-    add_to_bazelrc "build --platform_mappings=test_android_platforms/mappings"
     add_to_bazelrc "build --platforms=//test_android_platforms:simple"
   else
     echo "This test uses legacy Android toolchains."

diff --git a/src/test/shell/bazel/android/android_local_test_integration_test.sh b/src/test/shell/bazel/android/android_local_test_integration_test.sh
@@ -34,12 +34,7 @@ fail_if_no_android_sdk
 source "${CURRENT_DIR}/../../integration_test_setup.sh" \
   || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
 
-if [[ "$1" = '--with_platforms' ]]; then
-  # TODO(b/161709111): With platforms, the below fails with
-  # "no attribute `$android_sdk_toolchain_type`" on AspectAwareAttributeMapper.
-  echo "android_local_test_integration_test.sh does not support --with_platforms!" >&2
-  exit 0
-fi
+resolve_android_toolchains "$1"
 
 function setup_android_local_test_env() {
   mkdir -p java/com/bin/res/values

diff --git a/src/test/shell/bazel/android/android_sdk_integration_test.sh b/src/test/shell/bazel/android/android_sdk_integration_test.sh
@@ -33,6 +33,8 @@ fail_if_no_android_sdk
 source "${CURRENT_DIR}/../../integration_test_setup.sh" \
   || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
 
+resolve_android_toolchains "$1"
+
 function test_android_sdk_repository_path_from_environment() {
   create_new_workspace
   setup_android_sdk_support
@@ -51,6 +53,7 @@ EOF
 
 function test_android_sdk_repository_no_path_or_android_home() {
   create_new_workspace
+  setup_android_platforms
   cat >> $(create_workspace_with_default_repos WORKSPACE) <<EOF
 android_sdk_repository(
     name = "androidsdk",
@@ -63,6 +66,7 @@ EOF
 
 function test_android_sdk_repository_wrong_path() {
   create_new_workspace
+  setup_android_platforms
   mkdir "$TEST_SRCDIR/some_dir"
   cat >> $(create_workspace_with_default_repos WORKSPACE) <<EOF
 android_sdk_repository(
@@ -105,6 +109,7 @@ function test_android_sdk_repository_returns_null_if_env_vars_missing() {
 # Regression test for https://github.com/bazelbuild/bazel/issues/12069
 function test_android_sdk_repository_present_not_set() {
   create_new_workspace
+  setup_android_platforms
   cat >> WORKSPACE <<EOF
 android_sdk_repository(
     name = "androidsdk",

diff --git a/src/test/shell/testenv.sh.tmpl b/src/test/shell/testenv.sh.tmpl
@@ -381,6 +381,7 @@ platform(
 )
 EOF
 
+  add_to_bazelrc "build --platform_mappings=test_android_platforms/mappings"
   cat > test_android_platforms/mappings <<EOF
 platforms:
   //test_android_platforms:simple