Skip to content

Commit 6e134a1

Browse files
coeuvrelarsrc-google
coeuvre
authored and
larsrc-google
committed
Remote: Fix a bug that the XML generation is executed even if test.xml 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
1 parent 7e5cd52 commit 6e134a1

File tree

7 files changed

+255
-20
lines changed

7 files changed

+255
-20
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,28 @@ public ActionExecutionContext withFileOutErr(FileOutErr fileOutErr) {
380380
nestedSetExpander);
381381
}
382382

383+
/** Allows us to create a new context that overrides the MetadataHandler with another one. */
384+
public ActionExecutionContext withMetadataHandler(MetadataHandler metadataHandler) {
385+
return new ActionExecutionContext(
386+
executor,
387+
actionInputFileCache,
388+
actionInputPrefetcher,
389+
actionKeyContext,
390+
metadataHandler,
391+
rewindingEnabled,
392+
lostInputsCheck,
393+
fileOutErr,
394+
eventHandler,
395+
clientEnv,
396+
topLevelFilesets,
397+
artifactExpander,
398+
env,
399+
actionFileSystem,
400+
skyframeDepsResult,
401+
nestedSetExpander,
402+
syscalls);
403+
}
404+
383405
/**
384406
* A way of checking whether any lost inputs have been detected during the execution of this
385407
* action.

‎src/main/java/com/google/devtools/build/lib/exec/BUILD‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,12 +338,14 @@ java_library(
338338
"//src/main/java/com/google/devtools/build/lib/actions",
339339
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
340340
"//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
341+
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
341342
"//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
342343
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
343344
"//src/main/java/com/google/devtools/build/lib/buildeventstream",
344345
"//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
345346
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
346347
"//src/main/java/com/google/devtools/build/lib/events",
348+
"//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
347349
"//src/main/java/com/google/devtools/build/lib/util",
348350
"//src/main/java/com/google/devtools/build/lib/util/io",
349351
"//src/main/java/com/google/devtools/build/lib/vfs",

‎src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java‎

Lines changed: 147 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,17 +25,22 @@
2525
import com.google.devtools.build.lib.actions.ActionExecutionContext;
2626
import com.google.devtools.build.lib.actions.ActionInput;
2727
import com.google.devtools.build.lib.actions.ActionInputHelper;
28+
import com.google.devtools.build.lib.actions.Artifact;
29+
import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
2830
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
31+
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
2932
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
3033
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
3134
import com.google.devtools.build.lib.actions.ExecException;
3235
import com.google.devtools.build.lib.actions.ExecutionRequirements;
36+
import com.google.devtools.build.lib.actions.FileArtifactValue;
3337
import com.google.devtools.build.lib.actions.ResourceSet;
3438
import com.google.devtools.build.lib.actions.SimpleSpawn;
3539
import com.google.devtools.build.lib.actions.Spawn;
3640
import com.google.devtools.build.lib.actions.SpawnContinuation;
3741
import com.google.devtools.build.lib.actions.SpawnResult;
3842
import com.google.devtools.build.lib.actions.TestExecException;
43+
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
3944
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
4045
import com.google.devtools.build.lib.analysis.test.TestAttempt;
4146
import com.google.devtools.build.lib.analysis.test.TestResult;
@@ -50,6 +55,7 @@
5055
import com.google.devtools.build.lib.events.Reporter;
5156
import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
5257
import com.google.devtools.build.lib.server.FailureDetails.TestAction;
58+
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
5359
import com.google.devtools.build.lib.util.Pair;
5460
import com.google.devtools.build.lib.util.io.FileOutErr;
5561
import com.google.devtools.build.lib.vfs.FileStatus;
@@ -67,6 +73,8 @@
6773
import java.util.List;
6874
import java.util.Map;
6975
import java.util.TreeMap;
76+
import java.util.concurrent.ConcurrentHashMap;
77+
import java.util.concurrent.ConcurrentMap;
7078
import javax.annotation.Nullable;
7179

7280
/** Runs TestRunnerAction actions. */
@@ -135,7 +143,7 @@ public TestRunnerSpawn createTestRunnerSpawn(
135143
action.getTestProperties().isPersistentTestRunner()
136144
? action.getTools()
137145
: NestedSetBuilder.emptySet(Order.STABLE_ORDER),
138-
ImmutableSet.copyOf(action.getSpawnOutputs()),
146+
createSpawnOutputs(action),
139147
localResourceUsage);
140148
Path execRoot = actionExecutionContext.getExecRoot();
141149
ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
@@ -146,6 +154,21 @@ public TestRunnerSpawn createTestRunnerSpawn(
146154
action, actionExecutionContext, spawn, tmpDir, workingDirectory, execRoot);
147155
}
148156

157+
private ImmutableSet<ActionInput> createSpawnOutputs(TestRunnerAction action) {
158+
ImmutableSet.Builder<ActionInput> builder = ImmutableSet.builder();
159+
for (ActionInput output : action.getSpawnOutputs()) {
160+
if (output.getExecPath().equals(action.getXmlOutputPath())) {
161+
// HACK: Convert type of test.xml from BasicActionInput to DerivedArtifact. We want to
162+
// inject metadata of test.xml if it is generated remotely and it's currently only possible
163+
// to inject Artifact.
164+
builder.add(createArtifactOutput(action, output.getExecPath()));
165+
} else {
166+
builder.add(output);
167+
}
168+
}
169+
return builder.build();
170+
}
171+
149172
private static ImmutableList<Pair<String, Path>> renameOutputs(
150173
ActionExecutionContext actionExecutionContext,
151174
TestRunnerAction action,
@@ -294,11 +317,84 @@ private static Map<String, String> setupEnvironment(
294317
relativeTmpDir = tmpDir.asFragment();
295318
}
296319
return DEFAULT_LOCAL_POLICY.computeTestEnvironment(
297-
action,
298-
clientEnv,
299-
getTimeout(action),
300-
runfilesDir.relativeTo(execRoot),
301-
relativeTmpDir);
320+
action, clientEnv, getTimeout(action), runfilesDir.relativeTo(execRoot), relativeTmpDir);
321+
}
322+
323+
static class TestMetadataHandler implements MetadataHandler {
324+
private final MetadataHandler metadataHandler;
325+
private final ImmutableSet<Artifact> outputs;
326+
private final ConcurrentMap<Artifact, FileArtifactValue> fileMetadataMap =
327+
new ConcurrentHashMap<>();
328+
329+
TestMetadataHandler(MetadataHandler metadataHandler, ImmutableSet<Artifact> outputs) {
330+
this.metadataHandler = metadataHandler;
331+
this.outputs = outputs;
332+
}
333+
334+
@Nullable
335+
@Override
336+
public ActionInput getInput(String execPath) {
337+
return metadataHandler.getInput(execPath);
338+
}
339+
340+
@Nullable
341+
@Override
342+
public FileArtifactValue getMetadata(ActionInput input) throws IOException {
343+
return metadataHandler.getMetadata(input);
344+
}
345+
346+
@Override
347+
public void setDigestForVirtualArtifact(Artifact artifact, byte[] digest) {
348+
metadataHandler.setDigestForVirtualArtifact(artifact, digest);
349+
}
350+
351+
@Override
352+
public FileArtifactValue constructMetadataForDigest(
353+
Artifact output, FileStatus statNoFollow, byte[] injectedDigest) throws IOException {
354+
return metadataHandler.constructMetadataForDigest(output, statNoFollow, injectedDigest);
355+
}
356+
357+
@Override
358+
public ImmutableSet<TreeFileArtifact> getTreeArtifactChildren(SpecialArtifact treeArtifact) {
359+
return metadataHandler.getTreeArtifactChildren(treeArtifact);
360+
}
361+
362+
@Override
363+
public TreeArtifactValue getTreeArtifactValue(SpecialArtifact treeArtifact) throws IOException {
364+
return metadataHandler.getTreeArtifactValue(treeArtifact);
365+
}
366+
367+
@Override
368+
public void markOmitted(Artifact output) {
369+
metadataHandler.markOmitted(output);
370+
}
371+
372+
@Override
373+
public boolean artifactOmitted(Artifact artifact) {
374+
return metadataHandler.artifactOmitted(artifact);
375+
}
376+
377+
@Override
378+
public void resetOutputs(Iterable<? extends Artifact> outputs) {
379+
metadataHandler.resetOutputs(outputs);
380+
}
381+
382+
@Override
383+
public void injectFile(Artifact output, FileArtifactValue metadata) {
384+
if (outputs.contains(output)) {
385+
metadataHandler.injectFile(output, metadata);
386+
}
387+
fileMetadataMap.put(output, metadata);
388+
}
389+
390+
@Override
391+
public void injectTree(SpecialArtifact output, TreeArtifactValue tree) {
392+
metadataHandler.injectTree(output, tree);
393+
}
394+
395+
public boolean fileInjected(Artifact output) {
396+
return fileMetadataMap.containsKey(output);
397+
}
302398
}
303399

304400
private TestAttemptContinuation beginTestAttempt(
@@ -317,12 +413,26 @@ private TestAttemptContinuation beginTestAttempt(
317413
createStreamedTestOutput(
318414
Reporter.outErrForReporter(actionExecutionContext.getEventHandler()), out);
319415
}
416+
417+
// We use TestMetadataHandler here mainly because the one provided by actionExecutionContext
418+
// doesn't allow to inject undeclared outputs and test.xml is undeclared by the test action.
419+
TestMetadataHandler testMetadataHandler = null;
420+
if (actionExecutionContext.getMetadataHandler() != null) {
421+
testMetadataHandler =
422+
new TestMetadataHandler(
423+
actionExecutionContext.getMetadataHandler(), testAction.getOutputs());
424+
}
425+
320426
long startTimeMillis = actionExecutionContext.getClock().currentTimeMillis();
321427
SpawnStrategyResolver resolver = actionExecutionContext.getContext(SpawnStrategyResolver.class);
322428
SpawnContinuation spawnContinuation;
323429
try {
324430
spawnContinuation =
325-
resolver.beginExecution(spawn, actionExecutionContext.withFileOutErr(testOutErr));
431+
resolver.beginExecution(
432+
spawn,
433+
actionExecutionContext
434+
.withFileOutErr(testOutErr)
435+
.withMetadataHandler(testMetadataHandler));
326436
} catch (InterruptedException e) {
327437
if (streamed != null) {
328438
streamed.close();
@@ -332,6 +442,7 @@ private TestAttemptContinuation beginTestAttempt(
332442
}
333443
return new BazelTestAttemptContinuation(
334444
testAction,
445+
testMetadataHandler,
335446
actionExecutionContext,
336447
spawn,
337448
resolvedPaths,
@@ -387,6 +498,12 @@ private static BuildEventStreamProtos.TestResult.ExecutionInfo extractExecutionI
387498
return executionInfo.build();
388499
}
389500

501+
private static Artifact.DerivedArtifact createArtifactOutput(
502+
TestRunnerAction action, PathFragment outputPath) {
503+
Artifact.DerivedArtifact testLog = (Artifact.DerivedArtifact) action.getTestLog();
504+
return new DerivedArtifact(testLog.getRoot(), outputPath, testLog.getArtifactOwner());
505+
}
506+
390507
/**
391508
* A spawn to generate a test.xml file from the test log. This is only used if the test does not
392509
* generate a test.xml file itself.
@@ -423,7 +540,7 @@ private static Spawn createXmlGeneratingSpawn(
423540
/*inputs=*/ NestedSetBuilder.create(
424541
Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()),
425542
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
426-
/*outputs=*/ ImmutableSet.of(ActionInputHelper.fromPath(action.getXmlOutputPath())),
543+
/*outputs=*/ ImmutableSet.of(createArtifactOutput(action, action.getXmlOutputPath())),
427544
SpawnAction.DEFAULT_RESOURCE_SET);
428545
}
429546

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

577694
private final class BazelTestAttemptContinuation extends TestAttemptContinuation {
578695
private final TestRunnerAction testAction;
696+
@Nullable private final TestMetadataHandler testMetadataHandler;
579697
private final ActionExecutionContext actionExecutionContext;
580698
private final Spawn spawn;
581699
private final ResolvedPaths resolvedPaths;
@@ -588,6 +706,7 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation
588706

589707
BazelTestAttemptContinuation(
590708
TestRunnerAction testAction,
709+
@Nullable TestMetadataHandler testMetadataHandler,
591710
ActionExecutionContext actionExecutionContext,
592711
Spawn spawn,
593712
ResolvedPaths resolvedPaths,
@@ -598,6 +717,7 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation
598717
TestResultData.Builder testResultDataBuilder,
599718
ImmutableList<SpawnResult> spawnResults) {
600719
this.testAction = testAction;
720+
this.testMetadataHandler = testMetadataHandler;
601721
this.actionExecutionContext = actionExecutionContext;
602722
this.spawn = spawn;
603723
this.resolvedPaths = resolvedPaths;
@@ -638,6 +758,7 @@ public TestAttemptContinuation execute()
638758
if (!nextContinuation.isDone()) {
639759
return new BazelTestAttemptContinuation(
640760
testAction,
761+
testMetadataHandler,
641762
actionExecutionContext,
642763
spawn,
643764
resolvedPaths,
@@ -727,6 +848,7 @@ public TestAttemptContinuation execute()
727848
appendCoverageLog(coverageOutErr, fileOutErr);
728849
return new BazelCoveragePostProcessingContinuation(
729850
testAction,
851+
testMetadataHandler,
730852
actionExecutionContext,
731853
spawn,
732854
resolvedPaths,
@@ -762,15 +884,21 @@ public TestAttemptContinuation execute()
762884
throw new EnvironmentalExecException(e, Code.TEST_OUT_ERR_IO_EXCEPTION);
763885
}
764886

887+
Path xmlOutputPath = resolvedPaths.getXmlOutputPath();
888+
boolean testXmlGenerated = xmlOutputPath.exists();
889+
if (!testXmlGenerated && testMetadataHandler != null) {
890+
testXmlGenerated =
891+
testMetadataHandler.fileInjected(
892+
createArtifactOutput(testAction, testAction.getXmlOutputPath()));
893+
}
765894

766895
// If the test did not create a test.xml, and --experimental_split_xml_generation is enabled,
767896
// then we run a separate action to create a test.xml from test.log. We do this as a spawn
768897
// rather than doing it locally in-process, as the test.log file may only exist remotely (when
769898
// remote execution is enabled), and we do not want to have to download it.
770-
Path xmlOutputPath = resolvedPaths.getXmlOutputPath();
771899
if (executionOptions.splitXmlGeneration
772900
&& fileOutErr.getOutputPath().exists()
773-
&& !xmlOutputPath.exists()) {
901+
&& !testXmlGenerated) {
774902
Spawn xmlGeneratingSpawn =
775903
createXmlGeneratingSpawn(testAction, spawn.getEnvironment(), spawnResults.get(0));
776904
SpawnStrategyResolver spawnStrategyResolver =
@@ -781,7 +909,10 @@ public TestAttemptContinuation execute()
781909
try {
782910
SpawnContinuation xmlContinuation =
783911
spawnStrategyResolver.beginExecution(
784-
xmlGeneratingSpawn, actionExecutionContext.withFileOutErr(xmlSpawnOutErr));
912+
xmlGeneratingSpawn,
913+
actionExecutionContext
914+
.withFileOutErr(xmlSpawnOutErr)
915+
.withMetadataHandler(testMetadataHandler));
785916
return new BazelXmlCreationContinuation(
786917
resolvedPaths, xmlSpawnOutErr, testResultDataBuilder, spawnResults, xmlContinuation);
787918
} catch (InterruptedException e) {
@@ -879,6 +1010,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
8791010

8801011
private final class BazelCoveragePostProcessingContinuation extends TestAttemptContinuation {
8811012
private final ResolvedPaths resolvedPaths;
1013+
@Nullable private final TestMetadataHandler testMetadataHandler;
8821014
private final FileOutErr fileOutErr;
8831015
private final Closeable streamed;
8841016
private final TestResultData.Builder testResultDataBuilder;
@@ -890,6 +1022,7 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC
8901022

8911023
BazelCoveragePostProcessingContinuation(
8921024
TestRunnerAction testAction,
1025+
@Nullable TestMetadataHandler testMetadataHandler,
8931026
ActionExecutionContext actionExecutionContext,
8941027
Spawn spawn,
8951028
ResolvedPaths resolvedPaths,
@@ -899,6 +1032,7 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC
8991032
ImmutableList<SpawnResult> primarySpawnResults,
9001033
SpawnContinuation spawnContinuation) {
9011034
this.testAction = testAction;
1035+
this.testMetadataHandler = testMetadataHandler;
9021036
this.actionExecutionContext = actionExecutionContext;
9031037
this.spawn = spawn;
9041038
this.resolvedPaths = resolvedPaths;
@@ -923,6 +1057,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
9231057
if (!nextContinuation.isDone()) {
9241058
return new BazelCoveragePostProcessingContinuation(
9251059
testAction,
1060+
testMetadataHandler,
9261061
actionExecutionContext,
9271062
spawn,
9281063
resolvedPaths,
@@ -958,6 +1093,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
9581093

9591094
return new BazelTestAttemptContinuation(
9601095
testAction,
1096+
testMetadataHandler,
9611097
actionExecutionContext,
9621098
spawn,
9631099
resolvedPaths,

0 commit comments

Comments
 (0)