Skip to content

Commit

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

This reverts commit 9f8c678.
  • Loading branch information
@larsrc-google
larsrc-google committed Jul 30, 2021
1 parent ce091ab commit 1b19cd3
Show file tree
Hide file tree
Showing 7 changed files with 20 additions and 255 deletions.
22 changes: 0 additions & 22 deletions src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -380,28 +380,6 @@ 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: 0 additions & 2 deletions src/main/java/com/google/devtools/build/lib/exec/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,14 +338,12 @@ 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: 11 additions & 147 deletions src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,22 +25,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.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 @@ -55,7 +50,6 @@
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 @@ -73,8 +67,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. */
Expand Down Expand Up @@ -143,7 +135,7 @@ public TestRunnerSpawn createTestRunnerSpawn(
action.getTestProperties().isPersistentTestRunner()
? action.getTools()
: NestedSetBuilder.emptySet(Order.STABLE_ORDER),
createSpawnOutputs(action),
ImmutableSet.copyOf(action.getSpawnOutputs()),
localResourceUsage);
Path execRoot = actionExecutionContext.getExecRoot();
ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
Expand All @@ -154,21 +146,6 @@ 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 @@ -317,84 +294,11 @@ private static Map<String, String> setupEnvironment(
relativeTmpDir = tmpDir.asFragment();
}
return DEFAULT_LOCAL_POLICY.computeTestEnvironment(
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);
}
action,
clientEnv,
getTimeout(action),
runfilesDir.relativeTo(execRoot),
relativeTmpDir);
}

private TestAttemptContinuation beginTestAttempt(
Expand All @@ -413,26 +317,12 @@ 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)
.withMetadataHandler(testMetadataHandler));
resolver.beginExecution(spawn, actionExecutionContext.withFileOutErr(testOutErr));
} catch (InterruptedException e) {
if (streamed != null) {
streamed.close();
Expand All @@ -442,7 +332,6 @@ private TestAttemptContinuation beginTestAttempt(
}
return new BazelTestAttemptContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -498,12 +387,6 @@ 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 @@ -540,7 +423,7 @@ private static Spawn createXmlGeneratingSpawn(
/*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())),
SpawnAction.DEFAULT_RESOURCE_SET);
}

Expand Down Expand Up @@ -693,7 +576,6 @@ 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 @@ -706,7 +588,6 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation

BazelTestAttemptContinuation(
TestRunnerAction testAction,
@Nullable TestMetadataHandler testMetadataHandler,
ActionExecutionContext actionExecutionContext,
Spawn spawn,
ResolvedPaths resolvedPaths,
Expand All @@ -717,7 +598,6 @@ 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 @@ -758,7 +638,6 @@ public TestAttemptContinuation execute()
if (!nextContinuation.isDone()) {
return new BazelTestAttemptContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -848,7 +727,6 @@ public TestAttemptContinuation execute()
appendCoverageLog(coverageOutErr, fileOutErr);
return new BazelCoveragePostProcessingContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -884,21 +762,15 @@ 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()
&& !testXmlGenerated) {
&& !xmlOutputPath.exists()) {
Spawn xmlGeneratingSpawn =
createXmlGeneratingSpawn(testAction, spawn.getEnvironment(), spawnResults.get(0));
SpawnStrategyResolver spawnStrategyResolver =
Expand All @@ -909,10 +781,7 @@ public TestAttemptContinuation execute()
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) {
Expand Down Expand Up @@ -1010,7 +879,6 @@ 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 @@ -1022,7 +890,6 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC

BazelCoveragePostProcessingContinuation(
TestRunnerAction testAction,
@Nullable TestMetadataHandler testMetadataHandler,
ActionExecutionContext actionExecutionContext,
Spawn spawn,
ResolvedPaths resolvedPaths,
Expand All @@ -1032,7 +899,6 @@ 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 @@ -1057,7 +923,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
if (!nextContinuation.isDone()) {
return new BazelCoveragePostProcessingContinuation(
testAction,
testMetadataHandler,
actionExecutionContext,
spawn,
resolvedPaths,
Expand Down Expand Up @@ -1093,7 +958,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept

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

0 comments on commit 1b19cd3

Please sign in to comment.