From 1b19cd310418b850e8e0ca2086ffe50755c9ed7e Mon Sep 17 00:00:00 2001 From: Lars Clausen Date: Wed, 28 Jul 2021 12:11:15 +0200 Subject: [PATCH] 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 9f8c678d7054548865f56f3464f778c751657074. --- .../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, 20 insertions(+), 255 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 218f732e32e56c..fffb6a32dd1e23 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,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. 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 bc21d403468eda..11606c087752de 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -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", 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 1bad7dcb8471c8..d8a8030b03f9b1 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,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; @@ -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; @@ -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. */ @@ -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(); @@ -154,21 +146,6 @@ 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, @@ -317,84 +294,11 @@ private static Map 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 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); - } + action, + clientEnv, + getTimeout(action), + runfilesDir.relativeTo(execRoot), + relativeTmpDir); } private TestAttemptContinuation beginTestAttempt( @@ -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(); @@ -442,7 +332,6 @@ private TestAttemptContinuation beginTestAttempt( } return new BazelTestAttemptContinuation( testAction, - testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -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. @@ -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); } @@ -693,7 +576,6 @@ 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; @@ -706,7 +588,6 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation BazelTestAttemptContinuation( TestRunnerAction testAction, - @Nullable TestMetadataHandler testMetadataHandler, ActionExecutionContext actionExecutionContext, Spawn spawn, ResolvedPaths resolvedPaths, @@ -717,7 +598,6 @@ 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; @@ -758,7 +638,6 @@ public TestAttemptContinuation execute() if (!nextContinuation.isDone()) { return new BazelTestAttemptContinuation( testAction, - testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -848,7 +727,6 @@ public TestAttemptContinuation execute() appendCoverageLog(coverageOutErr, fileOutErr); return new BazelCoveragePostProcessingContinuation( testAction, - testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -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 = @@ -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) { @@ -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; @@ -1022,7 +890,6 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC BazelCoveragePostProcessingContinuation( TestRunnerAction testAction, - @Nullable TestMetadataHandler testMetadataHandler, ActionExecutionContext actionExecutionContext, Spawn spawn, ResolvedPaths resolvedPaths, @@ -1032,7 +899,6 @@ 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; @@ -1057,7 +923,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept if (!nextContinuation.isDone()) { return new BazelCoveragePostProcessingContinuation( testAction, - testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -1093,7 +958,6 @@ 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 e24cac1ace14ab..8641397973726f 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,12 +104,7 @@ public RemoteExecutionService( this.remoteOptions = remoteOptions; this.remoteCache = remoteCache; this.remoteExecutor = remoteExecutor; - - ImmutableSet.Builder filesToDownloadBuilder = ImmutableSet.builder(); - for (ActionInput actionInput : filesToDownload) { - filesToDownloadBuilder.add(actionInput.getExecPath()); - } - this.filesToDownload = filesToDownloadBuilder.build(); + this.filesToDownload = filesToDownload; } 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 1a2d8093196f55..eef958ce7df90e 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,6 +63,7 @@ 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; @@ -178,18 +179,11 @@ 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; } - - for (ActionInput output : outputs) { - if (filesToDownload.contains(output.getExecPath())) { - return true; - } - } - - return false; + return !Collections.disjoint(outputs, filesToDownload); } 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 7d2607e386e810..8672f92b4d3877 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,7 +37,6 @@ 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; @@ -129,19 +128,17 @@ private class FakeActionExecutionContext extends ActionExecutionContext { public FakeActionExecutionContext( FileOutErr fileOutErr, SpawnStrategy spawnStrategy, BinTools binTools) { - this(fileOutErr, toContextRegistry(spawnStrategy, binTools, fileSystem, directories), null); + this(fileOutErr, toContextRegistry(spawnStrategy, binTools, fileSystem, directories)); } public FakeActionExecutionContext( - FileOutErr fileOutErr, - ActionContext.ActionContextRegistry actionContextRegistry, - MetadataHandler metadataHandler) { + FileOutErr fileOutErr, ActionContext.ActionContextRegistry actionContextRegistry) { super( /*executor=*/ null, /*actionInputFileCache=*/ null, ActionInputPrefetcher.NONE, new ActionKeyContext(), - /*metadataHandler=*/ metadataHandler, + /*metadataHandler=*/ null, /*rewindingEnabled=*/ false, LostInputsCheck.NONE, fileOutErr, @@ -178,14 +175,7 @@ public Path getExecRoot() { @Override public ActionExecutionContext withFileOutErr(FileOutErr fileOutErr) { - return new FakeActionExecutionContext( - fileOutErr, actionContextRegistry, getMetadataHandler()); - } - - @Override - public ActionExecutionContext withMetadataHandler(MetadataHandler metadataHandler) { - return new FakeActionExecutionContext( - getFileOutErr(), actionContextRegistry, metadataHandler); + return new FakeActionExecutionContext(fileOutErr, actionContextRegistry); } } diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index a4ab63aa450ad6..ba53cc638e4bba 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -2649,58 +2649,4 @@ 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"