From cc5492fde99f46d9aff472f8d9521b28a21bd979 Mon Sep 17 00:00:00 2001 From: Austin Schuh Date: Wed, 1 Feb 2023 17:47:26 -0800 Subject: [PATCH] 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 and updated by tristan.maat@codethink.co.uk Progress on #10799. --- .../build/lib/analysis/RuleContext.java | 29 +++++++++++++++++++ .../lib/analysis/test/TestActionBuilder.java | 7 ++++- .../lib/analysis/test/TestConfiguration.java | 14 +++++++++ 3 files changed, 49 insertions(+), 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 index 4009e630f15cd6..97b6401c45380b 100644 --- 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,34 @@ 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 executionPlatform; + ImmutableMap execProperties; + + // If we have a toolchain, pull the target platform out of it. + if (toolchainContexts != null) { + executionPlatform = toolchainContexts.getTargetPlatform(); + execProperties = executionPlatform.execProperties(); + } else { + executionPlatform = null; + execProperties = getExecGroups().getExecProperties(DEFAULT_TEST_RUNNER_EXEC_GROUP); + } + + ActionOwner actionOwner = + createActionOwner( + rule, aspectDescriptors, getConfiguration(), execProperties, executionPlatform); + + if (actionOwner == null) { + actionOwner = ruleContext.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 index d4e5d18d678dd7..566c9974d87554 100644 --- 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 @@ -386,6 +386,11 @@ private TestParams createTestAction(int shards) testRunfilesSupplier = SingleRunfilesSupplier.create(runfilesSupport); } + ActionOwner actionOwner = + testConfiguration.useTargetPlatformForTests() + ? ruleContext.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 +434,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 index 203c020d5e21f6..ff9258019b9284 100644 --- 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": *