Skip to content

Commit 1b19cd3

Browse files
larsrc-googlelarsrc-google
committed
Revert "Remote: Fix a bug that the XML generation is executed even if test.xml is generated when build with --remote_download_minimal."
This reverts commit 9f8c678.
1 parent ce091ab commit 1b19cd3

File tree

7 files changed

+20
-255
lines changed

7 files changed

+20
-255
lines changed

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

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -380,28 +380,6 @@ 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-
405383
/**
406384
* A way of checking whether any lost inputs have been detected during the execution of this
407385
* action.

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -338,14 +338,12 @@ 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",
342341
"//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
343342
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
344343
"//src/main/java/com/google/devtools/build/lib/buildeventstream",
345344
"//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
346345
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
347346
"//src/main/java/com/google/devtools/build/lib/events",
348-
"//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
349347
"//src/main/java/com/google/devtools/build/lib/util",
350348
"//src/main/java/com/google/devtools/build/lib/util/io",
351349
"//src/main/java/com/google/devtools/build/lib/vfs",

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

Lines changed: 11 additions & 147 deletions
Original file line numberDiff line numberDiff line change
@@ -25,22 +25,17 @@
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;
3028
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
31-
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
3229
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
3330
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
3431
import com.google.devtools.build.lib.actions.ExecException;
3532
import com.google.devtools.build.lib.actions.ExecutionRequirements;
36-
import com.google.devtools.build.lib.actions.FileArtifactValue;
3733
import com.google.devtools.build.lib.actions.ResourceSet;
3834
import com.google.devtools.build.lib.actions.SimpleSpawn;
3935
import com.google.devtools.build.lib.actions.Spawn;
4036
import com.google.devtools.build.lib.actions.SpawnContinuation;
4137
import com.google.devtools.build.lib.actions.SpawnResult;
4238
import com.google.devtools.build.lib.actions.TestExecException;
43-
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
4439
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
4540
import com.google.devtools.build.lib.analysis.test.TestAttempt;
4641
import com.google.devtools.build.lib.analysis.test.TestResult;
@@ -55,7 +50,6 @@
5550
import com.google.devtools.build.lib.events.Reporter;
5651
import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
5752
import com.google.devtools.build.lib.server.FailureDetails.TestAction;
58-
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
5953
import com.google.devtools.build.lib.util.Pair;
6054
import com.google.devtools.build.lib.util.io.FileOutErr;
6155
import com.google.devtools.build.lib.vfs.FileStatus;
@@ -73,8 +67,6 @@
7367
import java.util.List;
7468
import java.util.Map;
7569
import java.util.TreeMap;
76-
import java.util.concurrent.ConcurrentHashMap;
77-
import java.util.concurrent.ConcurrentMap;
7870
import javax.annotation.Nullable;
7971

8072
/** Runs TestRunnerAction actions. */
@@ -143,7 +135,7 @@ public TestRunnerSpawn createTestRunnerSpawn(
143135
action.getTestProperties().isPersistentTestRunner()
144136
? action.getTools()
145137
: NestedSetBuilder.emptySet(Order.STABLE_ORDER),
146-
createSpawnOutputs(action),
138+
ImmutableSet.copyOf(action.getSpawnOutputs()),
147139
localResourceUsage);
148140
Path execRoot = actionExecutionContext.getExecRoot();
149141
ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
@@ -154,21 +146,6 @@ public TestRunnerSpawn createTestRunnerSpawn(
154146
action, actionExecutionContext, spawn, tmpDir, workingDirectory, execRoot);
155147
}
156148

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-
172149
private static ImmutableList<Pair<String, Path>> renameOutputs(
173150
ActionExecutionContext actionExecutionContext,
174151
TestRunnerAction action,
@@ -317,84 +294,11 @@ private static Map<String, String> setupEnvironment(
317294
relativeTmpDir = tmpDir.asFragment();
318295
}
319296
return DEFAULT_LOCAL_POLICY.computeTestEnvironment(
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-
}
297+
action,
298+
clientEnv,
299+
getTimeout(action),
300+
runfilesDir.relativeTo(execRoot),
301+
relativeTmpDir);
398302
}
399303

400304
private TestAttemptContinuation beginTestAttempt(
@@ -413,26 +317,12 @@ private TestAttemptContinuation beginTestAttempt(
413317
createStreamedTestOutput(
414318
Reporter.outErrForReporter(actionExecutionContext.getEventHandler()), out);
415319
}
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-
426320
long startTimeMillis = actionExecutionContext.getClock().currentTimeMillis();
427321
SpawnStrategyResolver resolver = actionExecutionContext.getContext(SpawnStrategyResolver.class);
428322
SpawnContinuation spawnContinuation;
429323
try {
430324
spawnContinuation =
431-
resolver.beginExecution(
432-
spawn,
433-
actionExecutionContext
434-
.withFileOutErr(testOutErr)
435-
.withMetadataHandler(testMetadataHandler));
325+
resolver.beginExecution(spawn, actionExecutionContext.withFileOutErr(testOutErr));
436326
} catch (InterruptedException e) {
437327
if (streamed != null) {
438328
streamed.close();
@@ -442,7 +332,6 @@ private TestAttemptContinuation beginTestAttempt(
442332
}
443333
return new BazelTestAttemptContinuation(
444334
testAction,
445-
testMetadataHandler,
446335
actionExecutionContext,
447336
spawn,
448337
resolvedPaths,
@@ -498,12 +387,6 @@ private static BuildEventStreamProtos.TestResult.ExecutionInfo extractExecutionI
498387
return executionInfo.build();
499388
}
500389

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-
507390
/**
508391
* A spawn to generate a test.xml file from the test log. This is only used if the test does not
509392
* generate a test.xml file itself.
@@ -540,7 +423,7 @@ private static Spawn createXmlGeneratingSpawn(
540423
/*inputs=*/ NestedSetBuilder.create(
541424
Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()),
542425
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
543-
/*outputs=*/ ImmutableSet.of(createArtifactOutput(action, action.getXmlOutputPath())),
426+
/*outputs=*/ ImmutableSet.of(ActionInputHelper.fromPath(action.getXmlOutputPath())),
544427
SpawnAction.DEFAULT_RESOURCE_SET);
545428
}
546429

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

694577
private final class BazelTestAttemptContinuation extends TestAttemptContinuation {
695578
private final TestRunnerAction testAction;
696-
@Nullable private final TestMetadataHandler testMetadataHandler;
697579
private final ActionExecutionContext actionExecutionContext;
698580
private final Spawn spawn;
699581
private final ResolvedPaths resolvedPaths;
@@ -706,7 +588,6 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation
706588

707589
BazelTestAttemptContinuation(
708590
TestRunnerAction testAction,
709-
@Nullable TestMetadataHandler testMetadataHandler,
710591
ActionExecutionContext actionExecutionContext,
711592
Spawn spawn,
712593
ResolvedPaths resolvedPaths,
@@ -717,7 +598,6 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation
717598
TestResultData.Builder testResultDataBuilder,
718599
ImmutableList<SpawnResult> spawnResults) {
719600
this.testAction = testAction;
720-
this.testMetadataHandler = testMetadataHandler;
721601
this.actionExecutionContext = actionExecutionContext;
722602
this.spawn = spawn;
723603
this.resolvedPaths = resolvedPaths;
@@ -758,7 +638,6 @@ public TestAttemptContinuation execute()
758638
if (!nextContinuation.isDone()) {
759639
return new BazelTestAttemptContinuation(
760640
testAction,
761-
testMetadataHandler,
762641
actionExecutionContext,
763642
spawn,
764643
resolvedPaths,
@@ -848,7 +727,6 @@ public TestAttemptContinuation execute()
848727
appendCoverageLog(coverageOutErr, fileOutErr);
849728
return new BazelCoveragePostProcessingContinuation(
850729
testAction,
851-
testMetadataHandler,
852730
actionExecutionContext,
853731
spawn,
854732
resolvedPaths,
@@ -884,21 +762,15 @@ public TestAttemptContinuation execute()
884762
throw new EnvironmentalExecException(e, Code.TEST_OUT_ERR_IO_EXCEPTION);
885763
}
886764

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-
}
894765

895766
// If the test did not create a test.xml, and --experimental_split_xml_generation is enabled,
896767
// then we run a separate action to create a test.xml from test.log. We do this as a spawn
897768
// rather than doing it locally in-process, as the test.log file may only exist remotely (when
898769
// remote execution is enabled), and we do not want to have to download it.
770+
Path xmlOutputPath = resolvedPaths.getXmlOutputPath();
899771
if (executionOptions.splitXmlGeneration
900772
&& fileOutErr.getOutputPath().exists()
901-
&& !testXmlGenerated) {
773+
&& !xmlOutputPath.exists()) {
902774
Spawn xmlGeneratingSpawn =
903775
createXmlGeneratingSpawn(testAction, spawn.getEnvironment(), spawnResults.get(0));
904776
SpawnStrategyResolver spawnStrategyResolver =
@@ -909,10 +781,7 @@ public TestAttemptContinuation execute()
909781
try {
910782
SpawnContinuation xmlContinuation =
911783
spawnStrategyResolver.beginExecution(
912-
xmlGeneratingSpawn,
913-
actionExecutionContext
914-
.withFileOutErr(xmlSpawnOutErr)
915-
.withMetadataHandler(testMetadataHandler));
784+
xmlGeneratingSpawn, actionExecutionContext.withFileOutErr(xmlSpawnOutErr));
916785
return new BazelXmlCreationContinuation(
917786
resolvedPaths, xmlSpawnOutErr, testResultDataBuilder, spawnResults, xmlContinuation);
918787
} catch (InterruptedException e) {
@@ -1010,7 +879,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
1010879

1011880
private final class BazelCoveragePostProcessingContinuation extends TestAttemptContinuation {
1012881
private final ResolvedPaths resolvedPaths;
1013-
@Nullable private final TestMetadataHandler testMetadataHandler;
1014882
private final FileOutErr fileOutErr;
1015883
private final Closeable streamed;
1016884
private final TestResultData.Builder testResultDataBuilder;
@@ -1022,7 +890,6 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC
1022890

1023891
BazelCoveragePostProcessingContinuation(
1024892
TestRunnerAction testAction,
1025-
@Nullable TestMetadataHandler testMetadataHandler,
1026893
ActionExecutionContext actionExecutionContext,
1027894
Spawn spawn,
1028895
ResolvedPaths resolvedPaths,
@@ -1032,7 +899,6 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC
1032899
ImmutableList<SpawnResult> primarySpawnResults,
1033900
SpawnContinuation spawnContinuation) {
1034901
this.testAction = testAction;
1035-
this.testMetadataHandler = testMetadataHandler;
1036902
this.actionExecutionContext = actionExecutionContext;
1037903
this.spawn = spawn;
1038904
this.resolvedPaths = resolvedPaths;
@@ -1057,7 +923,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
1057923
if (!nextContinuation.isDone()) {
1058924
return new BazelCoveragePostProcessingContinuation(
1059925
testAction,
1060-
testMetadataHandler,
1061926
actionExecutionContext,
1062927
spawn,
1063928
resolvedPaths,
@@ -1093,7 +958,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
1093958

1094959
return new BazelTestAttemptContinuation(
1095960
testAction,
1096-
testMetadataHandler,
1097961
actionExecutionContext,
1098962
spawn,
1099963
resolvedPaths,

0 commit comments

Comments
 (0)