Skip to content

Commit

Permalink
Remote: Fix a bug that the XML generation is executed even if test.xm…
Browse files Browse the repository at this point in the history 
…l is generated when build with --remote_download_minimal.

Change test.xml from BasicActionInput to Artifact before executing the spawn so its metadata can be injected.

Use a custom MetadataHandler to allow metadata injections of undeclared outputs.

Fixes bazelbuild#12554.

Closes bazelbuild#12590.

PiperOrigin-RevId: 380741230
  • Loading branch information
@coeuvre
coeuvre committed Jul 16, 2021
1 parent 4d6e6db commit 9f8c678
Show file tree
Hide file tree
Showing 7 changed files with 255 additions and 20 deletions.
22 changes: 22 additions & 0 deletions src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
View file
Expand Up @@ -380,6 +380,28 @@ public ActionExecutionContext withFileOutErr(FileOutErr fileOutErr) {
nestedSetExpander);
}

/** Allows us to create a new context that overrides the MetadataHandler with another one. */
public ActionExecutionContext withMetadataHandler(MetadataHandler metadataHandler) {
return new ActionExecutionContext(
executor,
actionInputFileCache,
actionInputPrefetcher,
actionKeyContext,
metadataHandler,
rewindingEnabled,
lostInputsCheck,
fileOutErr,
eventHandler,
clientEnv,
topLevelFilesets,
artifactExpander,
env,
actionFileSystem,
skyframeDepsResult,
nestedSetExpander,
syscalls);
}

/**
* A way of checking whether any lost inputs have been detected during the execution of this
* action.
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/exec/BUILD
View file
Expand Up @@ -338,12 +338,14 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/buildeventstream",
"//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util/io",
"//src/main/java/com/google/devtools/build/lib/vfs",
Expand Down
158 changes: 147 additions & 11 deletions src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
View file
Expand Up @@ -25,17 +25,22 @@
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.SimpleSpawn;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.TestExecException;
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.test.TestAttempt;
import com.google.devtools.build.lib.analysis.test.TestResult;
Expand All @@ -50,6 +55,7 @@
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
import com.google.devtools.build.lib.server.FailureDetails.TestAction;
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileStatus;
Expand All @@ -67,6 +73,8 @@
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import javax.annotation.Nullable;

/** Runs TestRunnerAction actions. */
Expand Down Expand Up @@ -135,7 +143,7 @@ public TestRunnerSpawn createTestRunnerSpawn(
action.getTestProperties().isPersistentTestRunner()
? action.getTools()
: NestedSetBuilder.emptySet(Order.STABLE_ORDER),
ImmutableSet.copyOf(action.getSpawnOutputs()),
createSpawnOutputs(action),
localResourceUsage);
Path execRoot = actionExecutionContext.getExecRoot();
ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
Expand All @@ -146,6 +154,21 @@ public TestRunnerSpawn createTestRunnerSpawn(
action, actionExecutionContext, spawn, tmpDir, workingDirectory, execRoot);
}

private ImmutableSet<ActionInput> createSpawnOutputs(TestRunnerAction action) {
ImmutableSet.Builder<ActionInput> builder = ImmutableSet.builder();
for (ActionInput output : action.getSpawnOutputs()) {
if (output.getExecPath().equals(action.getXmlOutputPath())) {
// HACK: Convert type of test.xml from BasicActionInput to DerivedArtifact. We want to
// inject metadata of test.xml if it is generated remotely and it's currently only possible
// to inject Artifact.
builder.add(createArtifactOutput(action, output.getExecPath()));
} else {
builder.add(output);
}
}
return builder.build();
}

private static ImmutableList<Pair<String, Path>> renameOutputs(
ActionExecutionContext actionExecutionContext,
TestRunnerAction action,
Expand Down Expand Up @@ -294,11 +317,84 @@ private static Map<String, String> setupEnvironment(
relativeTmpDir = tmpDir.asFragment();
}
return DEFAULT_LOCAL_POLICY.computeTestEnvironment(
action,
clientEnv,
getTimeout(action),
runfilesDir.relativeTo(execRoot),
relativeTmpDir);
action, clientEnv, getTimeout(action), runfilesDir.relativeTo(execRoot), relativeTmpDir);
}

static class TestMetadataHandler implements MetadataHandler {
private final MetadataHandler metadataHandler;
private final ImmutableSet<Artifact> outputs;
private final ConcurrentMap<Artifact, FileArtifactValue> fileMetadataMap =
new ConcurrentHashMap<>();

TestMetadataHandler(MetadataHandler metadataHandler, ImmutableSet<Artifact> outputs) {
this.metadataHandler = metadataHandler;
this.outputs = outputs;
}

@Nullable
@Override
public ActionInput getInput(String execPath) {
return metadataHandler.getInput(execPath);
}

@Nullable
@Override
public FileArtifactValue getMetadata(ActionInput input) throws IOException {
return metadataHandler.getMetadata(input);
}

@Override
public void setDigestForVirtualArtifact(Artifact artifact, byte[] digest) {
metadataHandler.setDigestForVirtualArtifact(artifact, digest);
}

@Override
public FileArtifactValue constructMetadataForDigest(
Artifact output, FileStatus statNoFollow, byte[] injectedDigest) throws IOException {
return metadataHandler.constructMetadataForDigest(output, statNoFollow, injectedDigest);
}

@Override
public ImmutableSet<TreeFileArtifact> getTreeArtifactChildren(SpecialArtifact treeArtifact) {
return metadataHandler.getTreeArtifactChildren(treeArtifact);
}

@Override
public TreeArtifactValue getTreeArtifactValue(SpecialArtifact treeArtifact) throws IOException {
return metadataHandler.getTreeArtifactValue(treeArtifact);
}

@Override
public void markOmitted(Artifact output) {
metadataHandler.markOmitted(output);
}

@Override
public boolean artifactOmitted(Artifact artifact) {
return metadataHandler.artifactOmitted(artifact);
}

@Override
public void resetOutputs(Iterable<? extends Artifact> outputs) {
metadataHandler.resetOutputs(outputs);
}

@Override
public void injectFile(Artifact output, FileArtifactValue metadata) {
if (outputs.contains(output)) {
metadataHandler.injectFile(output, metadata);
}
fileMetadataMap.put(output, metadata);
}

@Override
public void injectTree(SpecialArtifact output, TreeArtifactValue tree) {
metadataHandler.injectTree(output, tree);
}

public boolean fileInjected(Artifact output) {
return fileMetadataMap.containsKey(output);
}
}

private TestAttemptContinuation beginTestAttempt(
Expand All @@ -317,12 +413,26 @@ private TestAttemptContinuation beginTestAttempt(
createStreamedTestOutput(
Reporter.outErrForReporter(actionExecutionContext.getEventHandler()), out);
}

// We use TestMetadataHandler here mainly because the one provided by actionExecutionContext
// doesn't allow to inject undeclared outputs and test.xml is undeclared by the test action.
TestMetadataHandler testMetadataHandler = null;
if (actionExecutionContext.getMetadataHandler() != null) {
testMetadataHandler =
new TestMetadataHandler(
actionExecutionContext.getMetadataHandler(), testAction.getOutputs());
}

long startTimeMillis = actionExecutionContext.getClock().currentTimeMillis();
SpawnStrategyResolver resolver = actionExecutionContext.getContext(SpawnStrategyResolver.class);
SpawnContinuation spawnContinuation;
try {
spawnContinuation =
resolver.beginExecution(spawn, actionExecutionContext.withFileOutErr(testOutErr));
resolver.beginExecution(
spawn,
actionExecutionContext
.withFileOutErr(testOutErr)
.withMetadataHandler(testMetadataHandler));
} catch (InterruptedException e) {
if (streamed != null) {
streamed.close();
Expand All @@ -332,6 +442,7 @@ private TestAttemptContinuation beginTestAttempt(
}
return new BazelTestAttemptContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -387,6 +498,12 @@ private static BuildEventStreamProtos.TestResult.ExecutionInfo extractExecutionI
return executionInfo.build();
}

private static Artifact.DerivedArtifact createArtifactOutput(
TestRunnerAction action, PathFragment outputPath) {
Artifact.DerivedArtifact testLog = (Artifact.DerivedArtifact) action.getTestLog();
return new DerivedArtifact(testLog.getRoot(), outputPath, testLog.getArtifactOwner());
}

/**
* A spawn to generate a test.xml file from the test log. This is only used if the test does not
* generate a test.xml file itself.
Expand Down Expand Up @@ -423,7 +540,7 @@ private static Spawn createXmlGeneratingSpawn(
/*inputs=*/ NestedSetBuilder.create(
Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()),
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/*outputs=*/ ImmutableSet.of(ActionInputHelper.fromPath(action.getXmlOutputPath())),
/*outputs=*/ ImmutableSet.of(createArtifactOutput(action, action.getXmlOutputPath())),
SpawnAction.DEFAULT_RESOURCE_SET);
}

Expand Down Expand Up @@ -576,6 +693,7 @@ public void finalizeCancelledTest(List<FailedAttemptResult> failedAttempts) thro

private final class BazelTestAttemptContinuation extends TestAttemptContinuation {
private final TestRunnerAction testAction;
@Nullable private final TestMetadataHandler testMetadataHandler;
private final ActionExecutionContext actionExecutionContext;
private final Spawn spawn;
private final ResolvedPaths resolvedPaths;
Expand All @@ -588,6 +706,7 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation

BazelTestAttemptContinuation(
TestRunnerAction testAction,
@Nullable TestMetadataHandler testMetadataHandler,
ActionExecutionContext actionExecutionContext,
Spawn spawn,
ResolvedPaths resolvedPaths,
Expand All @@ -598,6 +717,7 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation
TestResultData.Builder testResultDataBuilder,
ImmutableList<SpawnResult> spawnResults) {
this.testAction = testAction;
this.testMetadataHandler = testMetadataHandler;
this.actionExecutionContext = actionExecutionContext;
this.spawn = spawn;
this.resolvedPaths = resolvedPaths;
Expand Down Expand Up @@ -638,6 +758,7 @@ public TestAttemptContinuation execute()
if (!nextContinuation.isDone()) {
return new BazelTestAttemptContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -727,6 +848,7 @@ public TestAttemptContinuation execute()
appendCoverageLog(coverageOutErr, fileOutErr);
return new BazelCoveragePostProcessingContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -762,15 +884,21 @@ public TestAttemptContinuation execute()
throw new EnvironmentalExecException(e, Code.TEST_OUT_ERR_IO_EXCEPTION);
}

Path xmlOutputPath = resolvedPaths.getXmlOutputPath();
boolean testXmlGenerated = xmlOutputPath.exists();
if (!testXmlGenerated && testMetadataHandler != null) {
testXmlGenerated =
testMetadataHandler.fileInjected(
createArtifactOutput(testAction, testAction.getXmlOutputPath()));
}

// If the test did not create a test.xml, and --experimental_split_xml_generation is enabled,
// then we run a separate action to create a test.xml from test.log. We do this as a spawn
// rather than doing it locally in-process, as the test.log file may only exist remotely (when
// remote execution is enabled), and we do not want to have to download it.
Path xmlOutputPath = resolvedPaths.getXmlOutputPath();
if (executionOptions.splitXmlGeneration
&& fileOutErr.getOutputPath().exists()
&& !xmlOutputPath.exists()) {
&& !testXmlGenerated) {
Spawn xmlGeneratingSpawn =
createXmlGeneratingSpawn(testAction, spawn.getEnvironment(), spawnResults.get(0));
SpawnStrategyResolver spawnStrategyResolver =
Expand All @@ -781,7 +909,10 @@ public TestAttemptContinuation execute()
try {
SpawnContinuation xmlContinuation =
spawnStrategyResolver.beginExecution(
xmlGeneratingSpawn, actionExecutionContext.withFileOutErr(xmlSpawnOutErr));
xmlGeneratingSpawn,
actionExecutionContext
.withFileOutErr(xmlSpawnOutErr)
.withMetadataHandler(testMetadataHandler));
return new BazelXmlCreationContinuation(
resolvedPaths, xmlSpawnOutErr, testResultDataBuilder, spawnResults, xmlContinuation);
} catch (InterruptedException e) {
Expand Down Expand Up @@ -879,6 +1010,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept

private final class BazelCoveragePostProcessingContinuation extends TestAttemptContinuation {
private final ResolvedPaths resolvedPaths;
@Nullable private final TestMetadataHandler testMetadataHandler;
private final FileOutErr fileOutErr;
private final Closeable streamed;
private final TestResultData.Builder testResultDataBuilder;
Expand All @@ -890,6 +1022,7 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC

BazelCoveragePostProcessingContinuation(
TestRunnerAction testAction,
@Nullable TestMetadataHandler testMetadataHandler,
ActionExecutionContext actionExecutionContext,
Spawn spawn,
ResolvedPaths resolvedPaths,
Expand All @@ -899,6 +1032,7 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC
ImmutableList<SpawnResult> primarySpawnResults,
SpawnContinuation spawnContinuation) {
this.testAction = testAction;
this.testMetadataHandler = testMetadataHandler;
this.actionExecutionContext = actionExecutionContext;
this.spawn = spawn;
this.resolvedPaths = resolvedPaths;
Expand All @@ -923,6 +1057,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
if (!nextContinuation.isDone()) {
return new BazelCoveragePostProcessingContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -958,6 +1093,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept

return new BazelTestAttemptContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down

0 comments on commit 9f8c678

Please sign in to comment.