Skip to content

Commit

Permalink
Support running tests on the target platform
Browse files Browse the repository at this point in the history
This change *includes* another change by @juliexxia that was previously
merged and rolled back:
bazelbuild@c266ac9

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 bazelbuild#10799.

Change-Id: I774bd4442044d6725e78f496b9991368e73ffa00
  • Loading branch information
ulfjack committed Dec 17, 2020
1 parent f5b6abc commit 342543f
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,16 @@ public static LabelLateBoundDefault<BuildConfiguration> getCoverageOutputGenerat
return runUnder != null ? runUnder.getLabel() : null;
});

public static final String TEST_RUNNER_EXEC_GROUP = "test";

/**
* A base rule for all test rules.
*/
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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -329,6 +331,12 @@ private TestParams createTestAction(int shards) throws InterruptedException {
ImmutableList.Builder<Artifact> coverageArtifacts = ImmutableList.builder();
ImmutableList.Builder<ActionInput> testOutputs = ImmutableList.builder();

ActionOwner actionOwner = testConfiguration.useTargetPlatformForTests()
? ruleContext.getTestActionOwner()
: ruleContext.getActionOwner(TEST_RUNNER_EXEC_GROUP);

This comment has been minimized.

Copy link
@quval

quval Dec 22, 2020

At least with the configuration I tried, this didn't seem to resolve the bug that you spotted: the action got assigned to the first platform configured on extra_execution_platforms rather than the platform chosen by exec_constraints (or the host platform), as if the test exec group didn't inherit any constraints. (See also my comment bazelbuild#10799 (comment))

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;
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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":
*
Expand Down

0 comments on commit 342543f

Please sign in to comment.