From 9f8c678d7054548865f56f3464f778c751657074 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Mon, 21 Jun 2021 23:23:19 -0700 Subject: [PATCH] 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 #12554. Closes #12590. PiperOrigin-RevId: 380741230 --- .../lib/actions/ActionExecutionContext.java | 22 +++ .../com/google/devtools/build/lib/exec/BUILD | 2 + .../lib/exec/StandaloneTestStrategy.java | 158 ++++++++++++++++-- .../lib/remote/RemoteExecutionService.java | 9 +- .../devtools/build/lib/remote/util/Utils.java | 12 +- .../lib/exec/StandaloneTestStrategyTest.java | 18 +- .../bazel/remote/remote_execution_test.sh | 54 ++++++ 7 files changed, 255 insertions(+), 20 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java index fffb6a32dd1e23..218f732e32e56c 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java @@ -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. diff --git a/src/main/java/com/google/devtools/build/lib/exec/BUILD b/src/main/java/com/google/devtools/build/lib/exec/BUILD index 11606c087752de..bc21d403468eda 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -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", 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 d8a8030b03f9b1..1bad7dcb8471c8 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 @@ -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; @@ -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; @@ -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. */ @@ -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(); @@ -146,6 +154,21 @@ public TestRunnerSpawn createTestRunnerSpawn( action, actionExecutionContext, spawn, tmpDir, workingDirectory, execRoot); } + private ImmutableSet createSpawnOutputs(TestRunnerAction action) { + ImmutableSet.Builder 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> renameOutputs( ActionExecutionContext actionExecutionContext, TestRunnerAction action, @@ -294,11 +317,84 @@ private static Map 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 outputs; + private final ConcurrentMap fileMetadataMap = + new ConcurrentHashMap<>(); + + TestMetadataHandler(MetadataHandler metadataHandler, ImmutableSet 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 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 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( @@ -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(); @@ -332,6 +442,7 @@ private TestAttemptContinuation beginTestAttempt( } return new BazelTestAttemptContinuation( testAction, + testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -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. @@ -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); } @@ -576,6 +693,7 @@ public void finalizeCancelledTest(List 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; @@ -588,6 +706,7 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation BazelTestAttemptContinuation( TestRunnerAction testAction, + @Nullable TestMetadataHandler testMetadataHandler, ActionExecutionContext actionExecutionContext, Spawn spawn, ResolvedPaths resolvedPaths, @@ -598,6 +717,7 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation TestResultData.Builder testResultDataBuilder, ImmutableList spawnResults) { this.testAction = testAction; + this.testMetadataHandler = testMetadataHandler; this.actionExecutionContext = actionExecutionContext; this.spawn = spawn; this.resolvedPaths = resolvedPaths; @@ -638,6 +758,7 @@ public TestAttemptContinuation execute() if (!nextContinuation.isDone()) { return new BazelTestAttemptContinuation( testAction, + testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -727,6 +848,7 @@ public TestAttemptContinuation execute() appendCoverageLog(coverageOutErr, fileOutErr); return new BazelCoveragePostProcessingContinuation( testAction, + testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -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 = @@ -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) { @@ -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; @@ -890,6 +1022,7 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC BazelCoveragePostProcessingContinuation( TestRunnerAction testAction, + @Nullable TestMetadataHandler testMetadataHandler, ActionExecutionContext actionExecutionContext, Spawn spawn, ResolvedPaths resolvedPaths, @@ -899,6 +1032,7 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC ImmutableList primarySpawnResults, SpawnContinuation spawnContinuation) { this.testAction = testAction; + this.testMetadataHandler = testMetadataHandler; this.actionExecutionContext = actionExecutionContext; this.spawn = spawn; this.resolvedPaths = resolvedPaths; @@ -923,6 +1057,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept if (!nextContinuation.isDone()) { return new BazelCoveragePostProcessingContinuation( testAction, + testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -958,6 +1093,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept return new BazelTestAttemptContinuation( testAction, + testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 8641397973726f..e24cac1ace14ab 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -84,7 +84,7 @@ public class RemoteExecutionService { private final RemoteOptions remoteOptions; @Nullable private final RemoteCache remoteCache; @Nullable private final RemoteExecutionClient remoteExecutor; - private final ImmutableSet filesToDownload; + private final ImmutableSet filesToDownload; public RemoteExecutionService( Path execRoot, @@ -104,7 +104,12 @@ public RemoteExecutionService( this.remoteOptions = remoteOptions; this.remoteCache = remoteCache; this.remoteExecutor = remoteExecutor; - this.filesToDownload = filesToDownload; + + ImmutableSet.Builder filesToDownloadBuilder = ImmutableSet.builder(); + for (ActionInput actionInput : filesToDownload) { + filesToDownloadBuilder.add(actionInput.getExecPath()); + } + this.filesToDownload = filesToDownloadBuilder.build(); } static Command buildCommand( diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java index eef958ce7df90e..1a2d8093196f55 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java @@ -63,7 +63,6 @@ import java.io.OutputStream; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.Locale; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; @@ -179,11 +178,18 @@ public static boolean shouldDownloadAllSpawnOutputs( /** Returns {@code true} if outputs contains one or more top level outputs. */ public static boolean hasFilesToDownload( - Collection outputs, ImmutableSet filesToDownload) { + Collection outputs, ImmutableSet filesToDownload) { if (filesToDownload.isEmpty()) { return false; } - return !Collections.disjoint(outputs, filesToDownload); + + for (ActionInput output : outputs) { + if (filesToDownload.contains(output.getExecPath())) { + return true; + } + } + + return false; } private static String statusName(int code) { diff --git a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java index 8672f92b4d3877..7d2607e386e810 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java @@ -37,6 +37,7 @@ import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnResult.Status; import com.google.devtools.build.lib.actions.SpawnStrategy; +import com.google.devtools.build.lib.actions.cache.MetadataHandler; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.test.TestActionContext; @@ -128,17 +129,19 @@ private class FakeActionExecutionContext extends ActionExecutionContext { public FakeActionExecutionContext( FileOutErr fileOutErr, SpawnStrategy spawnStrategy, BinTools binTools) { - this(fileOutErr, toContextRegistry(spawnStrategy, binTools, fileSystem, directories)); + this(fileOutErr, toContextRegistry(spawnStrategy, binTools, fileSystem, directories), null); } public FakeActionExecutionContext( - FileOutErr fileOutErr, ActionContext.ActionContextRegistry actionContextRegistry) { + FileOutErr fileOutErr, + ActionContext.ActionContextRegistry actionContextRegistry, + MetadataHandler metadataHandler) { super( /*executor=*/ null, /*actionInputFileCache=*/ null, ActionInputPrefetcher.NONE, new ActionKeyContext(), - /*metadataHandler=*/ null, + /*metadataHandler=*/ metadataHandler, /*rewindingEnabled=*/ false, LostInputsCheck.NONE, fileOutErr, @@ -175,7 +178,14 @@ public Path getExecRoot() { @Override public ActionExecutionContext withFileOutErr(FileOutErr fileOutErr) { - return new FakeActionExecutionContext(fileOutErr, actionContextRegistry); + return new FakeActionExecutionContext( + fileOutErr, actionContextRegistry, getMetadataHandler()); + } + + @Override + public ActionExecutionContext withMetadataHandler(MetadataHandler metadataHandler) { + return new FakeActionExecutionContext( + getFileOutErr(), actionContextRegistry, metadataHandler); } } diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index ba53cc638e4bba..a4ab63aa450ad6 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -2649,4 +2649,58 @@ EOF fi } +# Test that when testing with --remote_download_minimal, Bazel doesn't +# regenerate the test.xml if the action actually produced it. See +# https://github.com/bazelbuild/bazel/issues/12554 +function test_remote_download_minimal_with_test_xml_generation() { + mkdir -p a + + cat > a/BUILD <<'EOF' +sh_test( + name = "test0", + srcs = ["test.sh"], +) + +java_test( + name = "test1", + srcs = ["JavaTest.java"], + test_class = "JavaTest", +) +EOF + + cat > a/test.sh <<'EOF' +#!/bin/bash +echo 'Hello' +EOF + chmod a+x a/test.sh + + cat > a/JavaTest.java <<'EOF' +import org.junit.Test; + +public class JavaTest { + @Test + public void test() {} +} +EOF + + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + //a:test0 //a:test1 >& $TEST_log || fail "Failed to build" + + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + //a:test0 >& $TEST_log || fail "Failed to test" + # 2 remote spawns: 1 for executing the test, 1 for generating the test.xml + expect_log "2 processes: 2 remote" + + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + //a:test1 >& $TEST_log || fail "Failed to test" + # only 1 remote spawn: test.xml is generated by junit + expect_log "2 processes: 1 internal, 1 remote" +} + run_suite "Remote execution and remote cache tests"