From 342543fac3f3e2dae48dd1daf7fbd51e0f103261 Mon Sep 17 00:00:00 2001 From: Ulf Adams Date: Thu, 17 Dec 2020 12:53:42 +0100 Subject: [PATCH] Support running tests on the target platform This change *includes* another change by @juliexxia that was previously merged and rolled back: https://github.com/bazelbuild/bazel/commit/c266ac966761c4b3d8a408a03e407505c93effdd 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. Progress on #10799. Change-Id: I774bd4442044d6725e78f496b9991368e73ffa00 --- .../build/lib/analysis/BaseRuleClasses.java | 3 +++ .../build/lib/analysis/RuleContext.java | 18 ++++++++++++++++++ .../lib/analysis/test/TestActionBuilder.java | 10 +++++++++- .../lib/analysis/test/TestConfiguration.java | 12 ++++++++++++ 4 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java index 2268936e817988..22fe779a9a5bdb 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java @@ -148,6 +148,8 @@ public static LabelLateBoundDefault getCoverageOutputGenerat return runUnder != null ? runUnder.getLabel() : null; }); + public static final String TEST_RUNNER_EXEC_GROUP = "test"; + /** * A base rule for all test rules. */ @@ -155,6 +157,7 @@ public static final class TestBaseRule implements RuleDefinition { @Override public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) { return builder + .addExecGroup(TEST_RUNNER_EXEC_GROUP) .requiresConfigurationFragments(TestConfiguration.class) // TestConfiguration only needed to create TestAction and TestProvider // Only necessary at top-level and can be skipped if trimmed. 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 8c8251b501b499..bcc7dae69d84ba 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 @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.analysis; + +import static com.google.devtools.build.lib.analysis.BaseRuleClasses.TEST_RUNNER_EXEC_GROUP; import static com.google.devtools.build.lib.analysis.ToolchainCollection.DEFAULT_EXEC_GROUP_NAME; import com.google.common.annotations.VisibleForTesting; @@ -468,6 +470,22 @@ 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() { + ActionOwner actionOwner = + createActionOwner( + rule, + aspectDescriptors, + getConfiguration(), + getExecProperties(TEST_RUNNER_EXEC_GROUP, execProperties), + toolchainContexts == null ? null : toolchainContexts.getTargetPlatform()); + 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 b9cf03173f05a3..83385b729cf3a2 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 @@ -14,12 +14,14 @@ package com.google.devtools.build.lib.analysis.test; +import static com.google.devtools.build.lib.analysis.BaseRuleClasses.TEST_RUNNER_EXEC_GROUP; import static com.google.devtools.build.lib.packages.BuildType.LABEL; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.RunfilesSupplier; @@ -329,6 +331,12 @@ private TestParams createTestAction(int shards) throws InterruptedException { ImmutableList.Builder coverageArtifacts = ImmutableList.builder(); ImmutableList.Builder testOutputs = ImmutableList.builder(); + ActionOwner actionOwner = testConfiguration.useTargetPlatformForTests() + ? ruleContext.getTestActionOwner() + : ruleContext.getActionOwner(TEST_RUNNER_EXEC_GROUP); + if (actionOwner == null) { + actionOwner = ruleContext.getActionOwner(); + } // 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; @@ -390,7 +398,7 @@ private TestParams createTestAction(int shards) throws InterruptedException { boolean splitCoveragePostProcessing = testConfiguration.splitCoveragePostProcessing(); TestRunnerAction testRunnerAction = new TestRunnerAction( - ruleContext.getActionOwner(), + 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 f24cde317ecf35..5e2ebcf7d74c3e 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,14 @@ public static class TestOptions extends FragmentOptions { help = "If true, then Bazel will run coverage postprocessing for test in a new spawn.") public boolean splitCoveragePostProcessing; + @Option( + name = "use_target_platform_for_tests", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.EXECUTION}, + help = "If true, then Bazel will run coverage postprocessing for test in a new spawn.") + public boolean useTargetPlatformForTests; + @Override public FragmentOptions getHost() { TestOptions hostOptions = (TestOptions) getDefault(); @@ -391,6 +399,10 @@ public boolean splitCoveragePostProcessing() { return options.splitCoveragePostProcessing; } + public boolean useTargetPlatformForTests() { + return options.useTargetPlatformForTests; + } + /** * Option converter that han handle two styles of value for "--runs_per_test": *