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 f9f5de22587bf0..66f26fb0289b69 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 @@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactRoot; @@ -57,6 +58,13 @@ /** Helper class to create test actions. */ public final class TestActionBuilder { + + // Whether the test.xml is a declared output of this action rather than just an output of the test + // spawn. True for Bazel so that it behaves properly with Build without the Bytes (i.e., + // --remote_download_regex), but false for Blaze because not all test rules generate a test.xml. + // DO NOT inline this constant, as it's rewritten by Copybara on import/export. + private static final boolean TEST_XML_IS_ACTION_OUTPUT = true; + private static final String CC_CODE_COVERAGE_SCRIPT = "CC_CODE_COVERAGE_SCRIPT"; private static final String LCOV_MERGER = "LCOV_MERGER"; // The coverage tool Bazel uses to generate a code coverage report for C++. @@ -385,6 +393,11 @@ private TestParams createTestAction() Artifact.DerivedArtifact testLog = ruleContext.getPackageRelativeArtifact(dir.getRelative("test.log"), root); + ActionInput testXml = + TEST_XML_IS_ACTION_OUTPUT + ? ruleContext.getPackageRelativeArtifact(dir.getRelative("test.xml"), root) + : ActionInputHelper.fromPath( + testLog.getExecPath().getParentDirectory().getRelative("test.xml")); Artifact.DerivedArtifact cacheStatus = ruleContext.getPackageRelativeArtifact(dir.getRelative("test.cache_status"), root); @@ -419,6 +432,7 @@ private TestParams createTestAction() 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 2262cac1b6ae2c..da6ab800eb5ffb 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 @@ -119,6 +119,7 @@ public class TestRunnerAction extends AbstractAction private final BuildConfigurationValue configuration; private final TestConfiguration testConfiguration; private final Artifact testLog; + private final ActionInput testXml; private final Artifact cacheStatus; private final PathFragment testWarningsPath; private final PathFragment unusedRunfilesLogPath; @@ -129,7 +130,6 @@ public class TestRunnerAction extends AbstractAction private final PathFragment undeclaredOutputsManifestPath; private final PathFragment undeclaredOutputsAnnotationsPath; private final PathFragment undeclaredOutputsAnnotationsPbPath; - private final PathFragment xmlOutputPath; @Nullable private final PathFragment testShard; private final PathFragment testExitSafe; private final PathFragment testStderr; @@ -199,6 +199,7 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { Artifact testXmlGeneratorScript, // Must be in inputs @Nullable Artifact collectCoverageScript, // Must be in inputs, if not null Artifact testLog, + ActionInput testXml, Artifact cacheStatus, Artifact coverageArtifact, @Nullable Artifact coverageDirectory, @@ -220,7 +221,13 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { owner, inputs, nonNullAsSet( - testLog, cacheStatus, coverageArtifact, coverageDirectory, undeclaredOutputsDir)); + testLog, + // See TestActionBuilder.TEST_XML_IS_ACTION_OUTPUT for details. + testXml instanceof Artifact testXmlArtifact ? testXmlArtifact : null, + cacheStatus, + coverageArtifact, + coverageDirectory, + undeclaredOutputsDir)); Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null)); this.runfilesMiddleman = runfilesMiddleman; this.testSetupScript = testSetupScript; @@ -229,6 +236,7 @@ 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; @@ -247,7 +255,6 @@ 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"); @@ -284,7 +291,6 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { ImmutableSet.Builder filesToDeleteBuilder = ImmutableSet.builder() .add( - xmlOutputPath, testWarningsPath, unusedRunfilesLogPath, testStderr, @@ -294,6 +300,9 @@ private static ImmutableSet nonNullAsSet(Artifact... artifacts) { // instead. baseDir.getChild("coverage.dat"), baseDir.getChild("test.zip")); // Delete files fetched from remote execution. + if (!(testXml instanceof Artifact)) { + filesToDeleteBuilder.add(testXml.getExecPath()); + } if (testShard != null) { filesToDeleteBuilder.add(testShard); } @@ -367,7 +376,7 @@ public boolean checkShardingSupport() { public List getSpawnOutputs() { final List outputs = new ArrayList<>(); - outputs.add(ActionInputHelper.fromPath(getXmlOutputPath())); + outputs.add(testXml); outputs.add(ActionInputHelper.fromPath(getExitSafeFile())); if (isSharded()) { outputs.add(ActionInputHelper.fromPath(getTestShard())); @@ -774,7 +783,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", getXmlOutputPath().getPathString()); + env.put("XML_OUTPUT_FILE", testXml.getExecPathString()); if (!configuration.runfilesEnabled()) { // If runfiles are disabled, tell remote-runtest.sh/local-runtest.sh about that. @@ -828,6 +837,10 @@ public Artifact getTestLog() { return testLog; } + public ActionInput getTestXml() { + return testXml; + } + /** Returns all environment variables which must be set in order to run this test. */ public ActionEnvironment getExtraTestEnv() { return extraTestEnv; @@ -898,11 +911,6 @@ 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() { @@ -1182,7 +1190,7 @@ public Path getInfrastructureFailureFile() { /** Returns path to the optionally created XML output file created by the test. */ public Path getXmlOutputPath() { - return getPath(xmlOutputPath); + return getPath(testXml.getExecPath()); } public Path getCoverageDirectory() { 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 7d50faa10062f8..c8e298b21c9ef7 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 @@ -26,7 +26,6 @@ import com.google.common.io.ByteStreams; 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.SpecialArtifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.EnvironmentalExecException; @@ -454,7 +453,7 @@ private static Spawn createXmlGeneratingSpawn( .getExecPath() .getCallablePathStringForOs(action.getExecutionSettings().getExecutionOs()), action.getTestLog().getExecPathString(), - action.getXmlOutputPath().getPathString(), + action.getTestXml().getExecPathString(), Integer.toString(result.getWallTimeInMs() / 1000), Integer.toString(result.exitCode())); ImmutableMap.Builder envBuilder = ImmutableMap.builder(); @@ -479,7 +478,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(action.getTestXml()), /* mandatoryOutputs= */ null, SpawnAction.DEFAULT_RESOURCE_SET); } diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh index 21421ef88d0084..dae1e7f12ff77f 100755 --- a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh +++ b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh @@ -648,6 +648,63 @@ EOF assert_exists bazel-bin/a/test } +function test_download_regex_changed_with_action_cache_hit_for_test() { + # Regression test for #25762 + # Test that changes to --remote_download_regex are effective when matching + # test.xml even when the test is cached. + + add_rules_shell "MODULE.bazel" + mkdir -p a + + cat > a/test.sh <<'EOF' +#!/bin/sh + +cat > "$XML_OUTPUT_FILE" < + + + + test_case succeeded. + + + +EOF2 +echo "hi" > "$TEST_UNDECLARED_OUTPUTS_DIR/out.txt" +EOF + + chmod +x a/test.sh + + cat > a/BUILD <& $TEST_log || fail "Expected success" + + rm -rf bazel-out bazel-testlogs + + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + //a:test >& $TEST_log || fail "Expected success" + + assert_not_exists bazel-testlogs/a/test/test.xml + + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + --remote_download_regex='.*/test.xml' \ + //a:test >& $TEST_log || fail "Expected success" + + assert_exists bazel-testlogs/a/test/test.xml +} + function do_test_non_test_toplevel_targets() { # Regression test for https://github.com/bazelbuild/bazel/issues/17190. #