Support running tests on the target platform

This adds an --use_target_platform_for_tests option that changes tests to use the execution properties from the target platform rather than the host platform. I believe that the code is currently incorrect - if the host and target platforms differ, then tests are cross-compiled for the target platform, but use the execution properties for the host platform. This matters for remote execution, where host and target platform may be different CPU architectures and operating systems (e.g., compiling on x64 Linux and running on ARM64 Mac). Currently, such tests will typically fail to run if they contain architecture or OS-specific code. Based on work by Ulf Adams <[email protected]> and updated by [email protected] Progress on bazelbuild#10799. RELNOTES: Add --use_target_platform_for_tests which uses the target platform for executing tests instead of the execution platform.
AustinSchuh · Feb 10, 2023 · ddd04cb · ddd04cb
1 parent cc4af3e
commit ddd04cb
Show file tree

Hide file tree

Showing 4 changed files with 200 additions and 1 deletion.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.analysis;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.devtools.build.lib.analysis.test.ExecutionInfo.DEFAULT_TEST_RUNNER_EXEC_GROUP;
 import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -409,6 +410,38 @@ public ActionOwner getActionOwner() {
     return getActionOwner(DEFAULT_EXEC_GROUP_NAME);
   }
 
+  /**
+   * Returns a special action owner for test actions. Test actions should run on the target platform
+   * rather than the host platform. Note that the value is not cached (on the assumption that this
+   * method is only called once).
+   */
+  public ActionOwner getTestActionOwner() {
+    PlatformInfo testExecutionPlatform;
+    ImmutableMap<String, String> testExecProperties;
+
+    // If we have a toolchain, pull the target platform out of it.
+    if (toolchainContexts != null) {
+      // TODO(https://github.com/bazelbuild/bazel/issues/17466): This doesn't respect execution
+      // properties coming from the target's `exec_properties` attribute.
+      // src/test/java/com/google/devtools/build/lib/analysis/test/TestActionBuilderTest.java has a
+      // test to test for it when it gets figured out.
+      testExecutionPlatform = toolchainContexts.getTargetPlatform();
+      testExecProperties = testExecutionPlatform.execProperties();
+    } else {
+      testExecutionPlatform = null;
+      testExecProperties = getExecGroups().getExecProperties(DEFAULT_TEST_RUNNER_EXEC_GROUP);
+    }
+
+    ActionOwner actionOwner =
+        createActionOwner(
+            rule, aspectDescriptors, getConfiguration(), testExecProperties, testExecutionPlatform);
+
+    if (actionOwner == null) {
+      actionOwner = getActionOwner();
+    }
+    return actionOwner;
+  }
+
   @Override
   @Nullable
   public ActionOwner getActionOwner(String execGroup) {

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
@@ -187,6 +187,16 @@ public TestActionBuilder setShardCount(int explicitShardCount) {
     return this;
   }
 
+  private ActionOwner getTestActionOwner() {
+    if (this.executionRequirements != null) {
+      ActionOwner owner = ruleContext.getActionOwner(this.executionRequirements.getExecGroup());
+      if (owner != null) {
+        return owner;
+      }
+    }
+    return ruleContext.getTestActionOwner();
+  }
+
   private ActionOwner getOwner() {
     ActionOwner owner =
         ruleContext.getActionOwner(
@@ -386,6 +396,9 @@ private TestParams createTestAction(int shards)
       testRunfilesSupplier = SingleRunfilesSupplier.create(runfilesSupport);
     }
 
+    ActionOwner actionOwner =
+        testConfiguration.useTargetPlatformForTests() ? getTestActionOwner() : getOwner();
+
     // Use 1-based indices for user friendliness.
     for (int shard = 0; shard < shardRuns; shard++) {
       String shardDir = shardRuns > 1 ? String.format("shard_%d_of_%d", shard + 1, shards) : null;
@@ -429,7 +442,7 @@ private TestParams createTestAction(int shards)
         // TODO(b/234923262): Take exec_group into consideration when selecting sh tools
         TestRunnerAction testRunnerAction =
             new TestRunnerAction(
-                getOwner(),
+                actionOwner,
                 inputs,
                 testRunfilesSupplier,
                 testActionExecutable,

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java
@@ -281,6 +281,16 @@ public static class TestOptions extends FragmentOptions {
         help = "If true, undeclared test outputs will be archived in a zip file.")
     public boolean zipUndeclaredTestOutputs;
 
+    @Option(
+        name = "use_target_platform_for_tests",
+        defaultValue = "false",
+        documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+        effectTags = {OptionEffectTag.EXECUTION},
+        help =
+            "If true, then Bazel will use the target platform for running tests rather than "
+                + "the test exec group.")
+    public boolean useTargetPlatformForTests;
+
     @Override
     public FragmentOptions getExec() {
       // Options here are either:
@@ -381,6 +391,10 @@ public boolean getZipUndeclaredTestOutputs() {
     return options.zipUndeclaredTestOutputs;
   }
 
+  public boolean useTargetPlatformForTests() {
+    return options.useTargetPlatformForTests;
+  }
+
   /**
    * Option converter that han handle two styles of value for "--runs_per_test":
    *

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/test/TestActionBuilderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/test/TestActionBuilderTest.java
@@ -37,6 +37,7 @@
 import java.util.List;
 import java.util.Set;
 import org.junit.Before;
+import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -473,6 +474,144 @@ public void testOverrideExecGroup() throws Exception {
     assertThat(executionInfo).containsExactly("key", "good");
   }
 
+  /**
+   * Overriding the exec group from within the test with --use_target_platform_for_tests.
+   *
+   * <p>This is the same test as testOverrideExecGroup with --use_target_platform_for_tests and a
+   * target platform.
+   */
+  @Test
+  public void testOverrideTestExecGroup() throws Exception {
+    useConfiguration("--use_target_platform_for_tests=true", "--platforms=//:linux_aarch64");
+    scratch.file(
+        "some_test.bzl",
+        "def _some_test_impl(ctx):",
+        "    script = ctx.actions.declare_file(ctx.attr.name + '.sh')",
+        "    ctx.actions.write(script, 'shell script goes here', is_executable = True)",
+        "    return [",
+        "        DefaultInfo(executable = script),",
+        "        testing.ExecutionInfo({}, exec_group = 'custom_group'),",
+        "    ]",
+        "",
+        "some_test = rule(",
+        "    implementation = _some_test_impl,",
+        "    exec_groups = {'custom_group': exec_group()},",
+        "    test = True,",
+        ")");
+    scratch.file(
+        "BUILD",
+        "load(':some_test.bzl', 'some_test')",
+        "platform(",
+        "    name = 'linux_aarch64',",
+        "    constraint_values = ['@platforms//os:linux', '@platforms//cpu:aarch64'],",
+        ")",
+        "some_test(",
+        "    name = 'custom_exec_group_test',",
+        "    exec_properties = {'test.key': 'bad', 'custom_group.key': 'good'},",
+        ")");
+    ImmutableList<Artifact.DerivedArtifact> testStatusList =
+        getTestStatusArtifacts("//:custom_exec_group_test");
+    TestRunnerAction testAction = (TestRunnerAction) getGeneratingAction(testStatusList.get(0));
+    ImmutableMap<String, String> executionInfo = testAction.getExecutionInfo();
+    assertThat(executionInfo).containsExactly("key", "good");
+  }
+
+  /**
+   * Adding exec_properties from the platform with --use_target_platform_for_tests.
+   */
+  @Test
+  public void testTargetTestExecGroup() throws Exception {
+    useConfiguration(
+        "--use_target_platform_for_tests=true",
+        "--platforms=//:linux_aarch64",
+        "--host_platform=//:linux_x86");
+    scratch.file(
+        "some_test.bzl",
+        "def _some_test_impl(ctx):",
+        "    script = ctx.actions.declare_file(ctx.attr.name + '.sh')",
+        "    ctx.actions.write(script, 'shell script goes here', is_executable = True)",
+        "    return [",
+        "        DefaultInfo(executable = script),",
+        "    ]",
+        "",
+        "some_test = rule(",
+        "    implementation = _some_test_impl,",
+        "    test = True,",
+        ")");
+    scratch.file(
+        "BUILD",
+        "load(':some_test.bzl', 'some_test')",
+        "platform(",
+        "    name = 'linux_x86',",
+        "    constraint_values = ['@platforms//os:linux', '@platforms//cpu:x86_64'],",
+        "    exec_properties = {'keyhost': 'bad'},",
+        ")",
+        "platform(",
+        "    name = 'linux_aarch64',",
+        "    constraint_values = ['@platforms//os:linux', '@platforms//cpu:aarch64'],",
+        "    exec_properties = {'key2': 'good'},",
+        ")",
+        "some_test(",
+        "    name = 'exec_group_test',",
+        "    exec_properties = {'key': 'bad'},",
+        ")");
+    ImmutableList<Artifact.DerivedArtifact> testStatusList =
+        getTestStatusArtifacts("//:exec_group_test");
+    TestRunnerAction testAction = (TestRunnerAction) getGeneratingAction(testStatusList.get(0));
+    assertThat(testAction.getExecutionPlatform().label().getName()).isEqualTo("linux_aarch64");
+
+    ImmutableMap<String, String> executionInfo = testAction.getExecutionInfo();
+    assertThat(executionInfo).containsExactly("key2", "good");
+  }
+
+  /**
+   * Adding test specific exec_properties with --use_target_platform_for_tests.
+   */
+  @Test @Ignore("https://github.com/bazelbuild/bazel/issues/17466")
+  public void testTargetTestExecGroupInheritance() throws Exception {
+    useConfiguration(
+        "--use_target_platform_for_tests=true",
+        "--platforms=//:linux_aarch64",
+        "--host_platform=//:linux_x86");
+    scratch.file(
+        "some_test.bzl",
+        "def _some_test_impl(ctx):",
+        "    script = ctx.actions.declare_file(ctx.attr.name + '.sh')",
+        "    ctx.actions.write(script, 'shell script goes here', is_executable = True)",
+        "    return [",
+        "        DefaultInfo(executable = script),",
+        "    ]",
+        "",
+        "some_test = rule(",
+        "    implementation = _some_test_impl,",
+        "    test = True,",
+        ")");
+    scratch.file(
+        "BUILD",
+        "load(':some_test.bzl', 'some_test')",
+        "platform(",
+        "    name = 'linux_x86',",
+        "    constraint_values = ['@platforms//os:linux', '@platforms//cpu:x86_64'],",
+        "    exec_properties = {'keyhost': 'bad'},",
+        ")",
+        "platform(",
+        "    name = 'linux_aarch64',",
+        "    constraint_values = ['@platforms//os:linux', '@platforms//cpu:aarch64'],",
+        "    exec_properties = {'key2': 'good'},",
+        ")",
+        "some_test(",
+        "    name = 'exec_group_test',",
+        "    exec_properties = {'test.key': 'good', 'key': 'bad'},",
+        ")");
+    ImmutableList<Artifact.DerivedArtifact> testStatusList =
+        getTestStatusArtifacts("//:exec_group_test");
+    TestRunnerAction testAction = (TestRunnerAction) getGeneratingAction(testStatusList.get(0));
+    assertThat(testAction.getExecutionPlatform().label().getName()).isEqualTo("linux_aarch64");
+
+    ImmutableMap<String, String> executionInfo = testAction.getExecutionInfo();
+    assertThat(executionInfo).containsExactly("key2", "good", "key", "good");
+  }
+
   @Test
   public void testNonExecutableCoverageReportGenerator() throws Exception {
     useConfiguration(