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 d9133bce3a01dd..729f0f12a6df30 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 @@ -407,6 +407,30 @@ public ActionExecutionContext withFileOutErr(FileOutErr fileOutErr) { syscalls); } + /** + * 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/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java index 85a985ce5803a5..ecb2f35a6a1795 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java @@ -381,8 +381,6 @@ private TestParams createTestAction(int shards) Artifact.DerivedArtifact testLog = ruleContext.getPackageRelativeArtifact(dir.getRelative("test.log"), root); - Artifact.DerivedArtifact testXml = - ruleContext.getPackageRelativeArtifact(dir.getRelative("test.xml"), root); Artifact.DerivedArtifact cacheStatus = ruleContext.getPackageRelativeArtifact(dir.getRelative("test.cache_status"), root); @@ -419,7 +417,6 @@ private TestParams createTestAction(int shards) testXmlGeneratorExecutable, collectCoverageScript, testLog, - testXml, cacheStatus, coverageArtifact, coverageDirectory, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index 58458529e2a96c..8ad29753e148b6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java @@ -108,7 +108,6 @@ public class TestRunnerAction extends AbstractAction private final BuildConfiguration configuration; private final TestConfiguration testConfiguration; private final Artifact testLog; - private final Artifact testXml; private final Artifact cacheStatus; private final PathFragment testWarningsPath; private final PathFragment unusedRunfilesLogPath; @@ -120,6 +119,7 @@ public class TestRunnerAction extends AbstractAction private final PathFragment undeclaredOutputsAnnotationsDir; private final PathFragment undeclaredOutputsManifestPath; private final PathFragment undeclaredOutputsAnnotationsPath; + private final PathFragment xmlOutputPath; @Nullable private final PathFragment testShard; private final PathFragment testExitSafe; private final PathFragment testStderr; @@ -181,7 +181,6 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { Artifact testXmlGeneratorScript, // Must be in inputs @Nullable Artifact collectCoverageScript, // Must be in inputs, if not null Artifact testLog, - Artifact testXml, Artifact cacheStatus, Artifact coverageArtifact, @Nullable Artifact coverageDirectory, @@ -203,7 +202,7 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { NestedSetBuilder.wrap(Order.STABLE_ORDER, tools), inputs, runfilesSupplier, - nonNullAsSet(testLog, testXml, cacheStatus, coverageArtifact, coverageDirectory), + nonNullAsSet(testLog, cacheStatus, coverageArtifact, coverageDirectory), configuration.getActionEnvironment()); Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null)); this.testSetupScript = testSetupScript; @@ -212,7 +211,6 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { this.configuration = checkNotNull(configuration); this.testConfiguration = checkNotNull(configuration.getFragment(TestConfiguration.class)); this.testLog = testLog; - this.testXml = testXml; this.cacheStatus = cacheStatus; this.coverageData = coverageArtifact; this.coverageDirectory = coverageDirectory; @@ -230,6 +228,7 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { this.testExitSafe = baseDir.getChild("test.exited_prematurely"); // testShard Path should be set only if sharding is enabled. this.testShard = totalShards > 1 ? baseDir.getChild("test.shard") : null; + this.xmlOutputPath = baseDir.getChild("test.xml"); this.testWarningsPath = baseDir.getChild("test.warnings"); this.unusedRunfilesLogPath = baseDir.getChild("test.unused_runfiles_log"); this.testStderr = baseDir.getChild("test.err"); @@ -266,6 +265,7 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { ImmutableSet.Builder filesToDeleteBuilder = ImmutableSet.builder() .add( + xmlOutputPath, testWarningsPath, unusedRunfilesLogPath, testStderr, @@ -320,7 +320,7 @@ public boolean showsOutputUnconditionally() { public List getSpawnOutputs() { final List outputs = new ArrayList<>(); - outputs.add(this.testXml); + outputs.add(ActionInputHelper.fromPath(getXmlOutputPath())); outputs.add(ActionInputHelper.fromPath(getExitSafeFile())); if (isSharded()) { outputs.add(ActionInputHelper.fromPath(getTestShard())); @@ -607,7 +607,7 @@ public void setupEnvVariables(Map env, Duration timeout) { env.put("TEST_TOTAL_SHARDS", Integer.toString(getExecutionSettings().getTotalShards())); env.put("TEST_SHARD_STATUS_FILE", getTestShard().getPathString()); } - env.put("XML_OUTPUT_FILE", testXml.getExecPathString()); + env.put("XML_OUTPUT_FILE", getXmlOutputPath().getPathString()); if (!isEnableRunfiles()) { // If runfiles are disabled, tell remote-runtest.sh/local-runtest.sh about that. @@ -666,10 +666,6 @@ public Artifact getTestLog() { return testLog; } - public Artifact getTestXml() { - return testXml; - } - /** Returns all environment variables which must be set in order to run this test. */ public ActionEnvironment getExtraTestEnv() { return extraTestEnv; @@ -735,6 +731,11 @@ public PathFragment getInfrastructureFailureFile() { return testInfrastructureFailure; } + /** Returns path to the optionally created XML output file created by the test. */ + public PathFragment getXmlOutputPath() { + return xmlOutputPath; + } + /** Returns coverage data artifact or null if code coverage was not requested. */ @Nullable public Artifact getCoverageData() { @@ -1034,7 +1035,7 @@ public Path getInfrastructureFailureFile() { /** Returns path to the optionally created XML output file created by the test. */ public Path getXmlOutputPath() { - return getPath(testXml.getExecPath()); + return getPath(xmlOutputPath); } public Path getCoverageDirectory() { 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 25ac115b44341d..91ba46b49d011a 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 100be2c9e618a3..b4acd0162e9e80 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,11 +25,15 @@ 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; @@ -52,6 +56,7 @@ import com.google.devtools.build.lib.server.FailureDetails.Execution.Code; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; 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; @@ -69,6 +74,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. */ @@ -142,7 +149,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(); @@ -153,6 +160,19 @@ 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())) { + // Convert type of test.xml from BasicActionInput to DerivedArtifact + builder.add(createArtifactOutput(action, output.getExecPath())); + } else { + builder.add(output); + } + } + return builder.build(); + } + private static ImmutableList> renameOutputs( ActionExecutionContext actionExecutionContext, TestRunnerAction action, @@ -309,6 +329,83 @@ private static Map setupEnvironment( 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( TestRunnerAction testAction, Spawn spawn, @@ -325,12 +422,23 @@ private TestAttemptContinuation beginTestAttempt( createStreamedTestOutput( Reporter.outErrForReporter(actionExecutionContext.getEventHandler()), out); } + + 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(); @@ -340,6 +448,7 @@ private TestAttemptContinuation beginTestAttempt( } return new BazelTestAttemptContinuation( testAction, + testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -366,7 +475,7 @@ private static void writeOutFile(Path inFilePath, Path outFilePath) throws IOExc outFilePath.setWritable(true); } try (OutputStream out = outFilePath.getOutputStream(true); - InputStream in = inFilePath.getInputStream()) { + InputStream in = inFilePath.getInputStream()) { ByteStreams.copy(in, out); } } @@ -395,6 +504,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. @@ -405,7 +520,7 @@ private static Spawn createXmlGeneratingSpawn( ImmutableList.of( action.getTestXmlGeneratorScript().getExecPath().getCallablePathString(), action.getTestLog().getExecPathString(), - action.getTestXml().getExecPathString(), + action.getXmlOutputPath().getPathString(), Long.toString(result.getWallTime().orElse(Duration.ZERO).getSeconds()), Integer.toString(result.exitCode())); ImmutableMap.Builder envBuilder = ImmutableMap.builder(); @@ -429,9 +544,9 @@ private static Spawn createXmlGeneratingSpawn( null, ImmutableMap.of(), /*inputs=*/ NestedSetBuilder.create( - Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()), + Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()), /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), - /*outputs=*/ ImmutableSet.of(action.getTestXml()), + /*outputs=*/ ImmutableSet.of(createArtifactOutput(action, action.getXmlOutputPath())), SpawnAction.DEFAULT_RESOURCE_SET); } @@ -473,7 +588,7 @@ private static Spawn createCoveragePostProcessingSpawn( .build(), /* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), /* outputs= */ ImmutableSet.of( - ActionInputHelper.fromPath(action.getCoverageData().getExecPathString())), + ActionInputHelper.fromPath(action.getCoverageData().getExecPath())), SpawnAction.DEFAULT_RESOURCE_SET); } @@ -587,6 +702,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; @@ -599,6 +715,7 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation BazelTestAttemptContinuation( TestRunnerAction testAction, + @Nullable TestMetadataHandler testMetadataHandler, ActionExecutionContext actionExecutionContext, Spawn spawn, ResolvedPaths resolvedPaths, @@ -609,6 +726,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; @@ -648,6 +766,7 @@ public TestAttemptContinuation execute() if (!nextContinuation.isDone()) { return new BazelTestAttemptContinuation( testAction, + testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -738,6 +857,7 @@ public TestAttemptContinuation execute() appendCoverageLog(coverageOutErr, fileOutErr); return new BazelCoveragePostProcessingContinuation( testAction, + testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -773,21 +893,12 @@ public TestAttemptContinuation execute() throw new EnvironmentalExecException(e, Code.TEST_OUT_ERR_IO_EXCEPTION); } - - MetadataHandler metadataHandler = actionExecutionContext.getMetadataHandler(); - Path xmlOutputPath = resolvedPaths.getXmlOutputPath(); - boolean testXmlExists = xmlOutputPath.exists(); - if (!testXmlExists && metadataHandler != null) { - try { - // Check whether test.xml exists. If not, an IOException will be thrown. - metadataHandler.getMetadata(testAction.getTestXml()); - testXmlExists = true; - } catch (IOException ignored) { - // If test.xml doesn't exist, A call to getMetadata above will insert a MISSING_FILE_MARKER value - // into the store. We need to reset the outputs. - metadataHandler.resetOutputs(ImmutableList.of(testAction.getTestXml())); - } + 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, @@ -796,7 +907,7 @@ public TestAttemptContinuation execute() // remote execution is enabled), and we do not want to have to download it. if (executionOptions.splitXmlGeneration && fileOutErr.getOutputPath().exists() - && !testXmlExists) { + && !testXmlGenerated) { Spawn xmlGeneratingSpawn = createXmlGeneratingSpawn(testAction, spawn.getEnvironment(), spawnResults.get(0)); SpawnStrategyResolver spawnStrategyResolver = @@ -807,7 +918,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) { @@ -816,10 +930,6 @@ public TestAttemptContinuation execute() } } - if (!testXmlExists && metadataHandler != null) { - metadataHandler.markOmitted(testAction.getTestXml()); - } - TestCase details = parseTestResult(xmlOutputPath); if (details != null) { testResultDataBuilder.setTestCase(details); @@ -909,6 +1019,8 @@ 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; @@ -920,6 +1032,7 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC BazelCoveragePostProcessingContinuation( TestRunnerAction testAction, + @Nullable TestMetadataHandler testMetadataHandler, ActionExecutionContext actionExecutionContext, Spawn spawn, ResolvedPaths resolvedPaths, @@ -929,6 +1042,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; @@ -953,6 +1067,7 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept if (!nextContinuation.isDone()) { return new BazelCoveragePostProcessingContinuation( testAction, + testMetadataHandler, actionExecutionContext, spawn, resolvedPaths, @@ -989,6 +1104,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 420fe5faa48522..6196a7ceb889cc 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 @@ -85,7 +85,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, @@ -105,7 +105,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 f3d395a9993e16..9f2d023e96786d 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; @@ -129,17 +130,18 @@ 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, @@ -177,7 +179,12 @@ 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 51e871fabd22a8..18698adf2b1d29 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -1441,7 +1441,7 @@ EOF || fail "Expected toplevel output bazel-testlogs/a/test/test.log to be downloaded" [[ -f bazel-testlogs/a/test/test.xml ]] \ - || fail "Expected toplevel output bazel-testlogs/a/test/test.log to be downloaded" + || fail "Expected toplevel output bazel-testlogs/a/test/test.xml to be downloaded" bazel clean @@ -2642,4 +2642,58 @@ EOF fi } +function test_remote_download_minimal_and_test_xml_generation() { + # Test that when testing with --remote_download_minimal, Bazel wouldn't regenerate the test.xml if the action actually + # produced it. See https://github.com/bazelbuild/bazel/issues/12554 + + 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"