Save injected metadata in RemoteActionFileSystem
So that spawn outputs can be accessed among Spwans within the same action using the `FileSystem` API.
This allow us to revert the hack we introduced in #12590. Also fixes the issue described by #15711.
Closes #15711.
Closes #16123.
PiperOrigin-RevId: 469133936
Change-Id: Ide5bcfa0fe2c6a3806d333cd61270e411aa78f80
diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
index 09dbe44..1261a89 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -26,22 +26,17 @@
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.SimpleSpawn;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.actions.SpawnMetrics;
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;
@@ -57,7 +52,6 @@
import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
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;
@@ -76,8 +70,6 @@
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. */
@@ -147,7 +139,7 @@
ImmutableMap.of(),
/*inputs=*/ action.getInputs(),
NestedSetBuilder.emptySet(Order.STABLE_ORDER),
- createSpawnOutputs(action),
+ ImmutableSet.copyOf(action.getSpawnOutputs()),
/*mandatoryOutputs=*/ ImmutableSet.of(),
localResourcesSupplier);
Path execRoot = actionExecutionContext.getExecRoot();
@@ -159,21 +151,6 @@
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,
@@ -326,83 +303,6 @@
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(
TestRunnerAction testAction,
Spawn spawn,
@@ -420,25 +320,12 @@
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)
- .withMetadataHandler(testMetadataHandler));
+ resolver.beginExecution(spawn, actionExecutionContext.withFileOutErr(testOutErr));
} catch (InterruptedException e) {
if (streamed != null) {
streamed.close();
@@ -448,7 +335,6 @@
}
return new BazelTestAttemptContinuation(
testAction,
- testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
@@ -559,12 +445,6 @@
return Durations.fromNanos(d.toNanos());
}
- private static Artifact.DerivedArtifact createArtifactOutput(
- TestRunnerAction action, PathFragment outputPath) {
- Artifact.DerivedArtifact testLog = (Artifact.DerivedArtifact) action.getTestLog();
- return DerivedArtifact.create(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.
@@ -610,7 +490,7 @@
/*inputs=*/ NestedSetBuilder.create(
Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()),
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
- /*outputs=*/ ImmutableSet.of(createArtifactOutput(action, action.getXmlOutputPath())),
+ /*outputs=*/ ImmutableSet.of(ActionInputHelper.fromPath(action.getXmlOutputPath())),
/*mandatoryOutputs=*/ null,
SpawnAction.DEFAULT_RESOURCE_SET);
}
@@ -763,7 +643,6 @@
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;
@@ -776,7 +655,6 @@
BazelTestAttemptContinuation(
TestRunnerAction testAction,
- @Nullable TestMetadataHandler testMetadataHandler,
ActionExecutionContext actionExecutionContext,
Spawn spawn,
ResolvedPaths resolvedPaths,
@@ -787,7 +665,6 @@
TestResultData.Builder testResultDataBuilder,
ImmutableList<SpawnResult> spawnResults) {
this.testAction = testAction;
- this.testMetadataHandler = testMetadataHandler;
this.actionExecutionContext = actionExecutionContext;
this.spawn = spawn;
this.resolvedPaths = resolvedPaths;
@@ -827,7 +704,6 @@
if (!nextContinuation.isDone()) {
return new BazelTestAttemptContinuation(
testAction,
- testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
@@ -918,7 +794,6 @@
appendCoverageLog(coverageOutErr, fileOutErr);
return new BazelCoveragePostProcessingContinuation(
testAction,
- testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
@@ -955,12 +830,6 @@
}
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
@@ -968,7 +837,7 @@
// remote execution is enabled), and we do not want to have to download it.
if (executionOptions.splitXmlGeneration
&& fileOutErr.getOutputPath().exists()
- && !testXmlGenerated) {
+ && !xmlOutputPath.exists()) {
Spawn xmlGeneratingSpawn =
createXmlGeneratingSpawn(testAction, spawn.getEnvironment(), spawnResults.get(0));
SpawnStrategyResolver spawnStrategyResolver =
@@ -979,10 +848,7 @@
try {
SpawnContinuation xmlContinuation =
spawnStrategyResolver.beginExecution(
- xmlGeneratingSpawn,
- actionExecutionContext
- .withFileOutErr(xmlSpawnOutErr)
- .withMetadataHandler(testMetadataHandler));
+ xmlGeneratingSpawn, actionExecutionContext.withFileOutErr(xmlSpawnOutErr));
return new BazelXmlCreationContinuation(
resolvedPaths, xmlSpawnOutErr, testResultDataBuilder, spawnResults, xmlContinuation);
} catch (InterruptedException e) {
@@ -1080,7 +946,6 @@
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;
@@ -1092,7 +957,6 @@
BazelCoveragePostProcessingContinuation(
TestRunnerAction testAction,
- @Nullable TestMetadataHandler testMetadataHandler,
ActionExecutionContext actionExecutionContext,
Spawn spawn,
ResolvedPaths resolvedPaths,
@@ -1102,7 +966,6 @@
ImmutableList<SpawnResult> primarySpawnResults,
SpawnContinuation spawnContinuation) {
this.testAction = testAction;
- this.testMetadataHandler = testMetadataHandler;
this.actionExecutionContext = actionExecutionContext;
this.spawn = spawn;
this.resolvedPaths = resolvedPaths;
@@ -1127,7 +990,6 @@
if (!nextContinuation.isDone()) {
return new BazelCoveragePostProcessingContinuation(
testAction,
- testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
@@ -1164,7 +1026,6 @@
return new BazelTestAttemptContinuation(
testAction,
- testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
