From 9bebed59e8eb059288378de0bedfcc9281cec92e Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 10 Jan 2023 10:01:06 -0800 Subject: [PATCH 1/2] Returns null if filesystem of test outputs is not ActionFS when processing test attempt event. Previously, we assert that the filesystem of test outputs is ActionFS when we are processing test attempt event. However this is not true when the test hits action cache. This CL looses the check to return null. PiperOrigin-RevId: 501023752 Change-Id: I17cbb26e0a2b5fd30cb781818e42172ac672919e --- .../google/devtools/build/lib/remote/RemoteModule.java | 10 +++++----- .../build/lib/remote/ToplevelArtifactsDownloader.java | 4 ++++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 12058e00d38e32..eb0a6eee5a7a2e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -930,11 +930,11 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB actionInputFetcher, (path) -> { FileSystem fileSystem = path.getFileSystem(); - Preconditions.checkState( - fileSystem instanceof RemoteActionFileSystem, - "fileSystem must be an instance of RemoteActionFileSystem"); - return ((RemoteActionFileSystem) path.getFileSystem()) - .getRemoteMetadata(path.asFragment()); + if (fileSystem instanceof RemoteActionFileSystem) { + return ((RemoteActionFileSystem) path.getFileSystem()) + .getRemoteMetadata(path.asFragment()); + } + return null; }); env.getEventBus().register(toplevelArtifactsDownloader); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java index 71ddfc15cefcfc..b8b11d02822174 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java @@ -112,6 +112,10 @@ public void onTestAttempt(TestAttempt event) { // Since the event is fired within action execution, the skyframe doesn't know the outputs of // test actions yet, so we can't get their metadata through skyframe. However, the fileSystem // of the path is an ActionFileSystem, we use it to get the metadata for this file. + // + // If the test hit action cache, the filesystem is local filesystem because the actual test + // action didn't get the chance to execute. In this case the metadata is null which is fine + // because test outputs are already downloaded (otherwise it cannot hit the action cache). FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path); if (metadata != null) { ListenableFuture future = From bff331db7bee026aa1b06061d7f4a3f7fff376ac Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 18 Jan 2023 06:00:19 -0800 Subject: [PATCH 2/2] Make `bazel coverage` work with minimal mode This PR solves the problem in a different way that #16475 tries to solve: 1. https://github.com/bazelbuild/bazel/pull/16812 allows skyframe read metadata from ActionFS. 2. Use `ActionFileSystem` to check existence of coverage data. 3. Fire event `CoverageReport` in the action after the coverage report is generated and listen to it in `ToplevelArtifactsDownloader` to download the report. Closes #16556. PiperOrigin-RevId: 502854552 Change-Id: I2796baaa962857831ff161423be6dffa6eb73e5c --- .../google/devtools/build/lib/analysis/BUILD | 10 +++ .../lib/analysis/test/CoverageReport.java | 31 +++++++ .../devtools/build/lib/bazel/coverage/BUILD | 2 + .../coverage/CoverageReportActionBuilder.java | 11 +++ .../lib/exec/StandaloneTestStrategy.java | 5 +- .../google/devtools/build/lib/remote/BUILD | 1 + .../remote/ToplevelArtifactsDownloader.java | 70 ++++++++++------ .../remote/build_without_the_bytes_test.sh | 84 +++++++++++++++++++ 8 files changed, 187 insertions(+), 27 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/test/CoverageReport.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index d42c34e2c4b0a1..b7dd37dce0f342 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -2527,6 +2527,16 @@ java_library( ], ) +java_library( + name = "test/coverage_report", + srcs = ["test/CoverageReport.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//third_party:guava", + ], +) + java_library( name = "test/coverage_report_action_factory", srcs = ["test/CoverageReportActionFactory.java"], diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageReport.java b/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageReport.java new file mode 100644 index 00000000000000..c90fb880f6b067 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageReport.java @@ -0,0 +1,31 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.analysis.test; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.vfs.Path; + +/** This event is used to notify about a successfully generated coverage report. */ +public final class CoverageReport implements ExtendedEventHandler.Postable { + private final ImmutableList files; + + public CoverageReport(ImmutableList files) { + this.files = files; + } + + public ImmutableList getFiles() { + return files; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/bazel/coverage/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/coverage/BUILD index 42078ebfd4b87e..64e04c13d7495b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/coverage/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/coverage/BUILD @@ -25,12 +25,14 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:incompatible_platform_provider", + "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report", "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report_action_factory", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_resolver", "//src/main/java/com/google/devtools/build/lib/util", + "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/common/options", "//third_party:auto_value", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java b/src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java index 721799cbaa621b..87c61778611b01 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.bazel.coverage; +import static com.google.common.collect.ImmutableList.toImmutableList; + import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -30,6 +32,7 @@ import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.actions.ArtifactOwner; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.BaseSpawn; import com.google.devtools.build.lib.actions.ExecException; @@ -45,6 +48,7 @@ import com.google.devtools.build.lib.analysis.RunfilesSupport; import com.google.devtools.build.lib.analysis.actions.Compression; import com.google.devtools.build.lib.analysis.actions.FileWriteAction; +import com.google.devtools.build.lib.analysis.test.CoverageReport; import com.google.devtools.build.lib.analysis.test.CoverageReportActionFactory.CoverageReportActionsWrapper; import com.google.devtools.build.lib.analysis.test.TestProvider; import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams; @@ -57,6 +61,7 @@ import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.exec.SpawnStrategyResolver; import com.google.devtools.build.lib.util.Fingerprint; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; import java.util.Collection; @@ -146,6 +151,12 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext) .getContext(SpawnStrategyResolver.class) .exec(spawn, actionExecutionContext); actionExecutionContext.getEventHandler().handle(Event.info(locationMessage)); + ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver(); + ImmutableList files = + getOutputs().stream() + .map(artifact -> pathResolver.convertPath(artifact.getPath())) + .collect(toImmutableList()); + actionExecutionContext.getEventHandler().post(new CoverageReport(files)); return ActionResult.create(spawnResults); } catch (ExecException e) { throw ActionExecutionException.fromExecException(e, this); 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 fc3765080b4520..d051842e4dce0c 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 @@ -827,7 +827,10 @@ public TestAttemptContinuation execute() Verify.verify( !(testAction.isCoverageMode() && testAction.getSplitCoveragePostProcessing()) - || testAction.getCoverageData().getPath().exists()); + || actionExecutionContext + .getPathResolver() + .convertPath(testAction.getCoverageData().getPath()) + .exists()); Verify.verifyNotNull(spawnResults); Verify.verifyNotNull(testResultDataBuilder); diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index 6a844d33dd3378..692df86d825b78 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -201,6 +201,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value", + "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/remote/util", "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value", diff --git a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java index b8b11d02822174..91574d8f9d5ebd 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java @@ -35,6 +35,7 @@ import com.google.devtools.build.lib.analysis.TargetCompleteEvent; import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; +import com.google.devtools.build.lib.analysis.test.CoverageReport; import com.google.devtools.build.lib.analysis.test.TestAttempt; import com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher.Priority; import com.google.devtools.build.lib.remote.util.StaticMetadataProvider; @@ -56,7 +57,8 @@ private enum CommandMode { UNKNOWN, BUILD, TEST, - RUN; + RUN, + COVERAGE; } private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); @@ -83,6 +85,9 @@ public ToplevelArtifactsDownloader( case "run": this.commandMode = CommandMode.RUN; break; + case "coverage": + this.commandMode = CommandMode.COVERAGE; + break; default: this.commandMode = CommandMode.UNKNOWN; } @@ -104,36 +109,48 @@ public interface PathToMetadataConverter { FileArtifactValue getMetadata(Path path); } + private void downloadTestOutput(Path path) { + // Since the event is fired within action execution, the skyframe doesn't know the outputs of + // test actions yet, so we can't get their metadata through skyframe. However, the fileSystem + // of the path is an ActionFileSystem, we use it to get the metadata for this file. + // + // If the test hit action cache, the filesystem is local filesystem because the actual test + // action didn't get the chance to execute. In this case the metadata is null which is fine + // because test outputs are already downloaded (otherwise it cannot hit the action cache). + FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path); + if (metadata != null) { + ListenableFuture future = + actionInputPrefetcher.downloadFileAsync(path.asFragment(), metadata, Priority.LOW); + addCallback( + future, + new FutureCallback() { + @Override + public void onSuccess(Void unused) {} + + @Override + public void onFailure(Throwable throwable) { + logger.atWarning().withCause(throwable).log( + "Failed to download test output %s.", path); + } + }, + directExecutor()); + } + } + @Subscribe @AllowConcurrentEvents public void onTestAttempt(TestAttempt event) { for (Pair pair : event.getFiles()) { Path path = checkNotNull(pair.getSecond()); - // Since the event is fired within action execution, the skyframe doesn't know the outputs of - // test actions yet, so we can't get their metadata through skyframe. However, the fileSystem - // of the path is an ActionFileSystem, we use it to get the metadata for this file. - // - // If the test hit action cache, the filesystem is local filesystem because the actual test - // action didn't get the chance to execute. In this case the metadata is null which is fine - // because test outputs are already downloaded (otherwise it cannot hit the action cache). - FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path); - if (metadata != null) { - ListenableFuture future = - actionInputPrefetcher.downloadFileAsync(path.asFragment(), metadata, Priority.LOW); - addCallback( - future, - new FutureCallback() { - @Override - public void onSuccess(Void unused) {} + downloadTestOutput(path); + } + } - @Override - public void onFailure(Throwable throwable) { - logger.atWarning().withCause(throwable).log( - "Failed to download test output %s.", path); - } - }, - directExecutor()); - } + @Subscribe + @AllowConcurrentEvents + public void onCoverageReport(CoverageReport event) { + for (var file : event.getFiles()) { + downloadTestOutput(file); } } @@ -172,8 +189,9 @@ private boolean shouldDownloadToplevelOutputs(ConfiguredTargetKey configuredTarg case RUN: // Always download outputs of toplevel targets in RUN mode return true; + case COVERAGE: case TEST: - // Do not download test binary in test mode. + // Do not download test binary in test/coverage mode. try { var configuredTargetValue = (ConfiguredTargetValue) memoizingEvaluator.getExistingValue(configuredTargetKey); 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 3680f845c1846f..ff03785deb2d06 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 @@ -1363,4 +1363,88 @@ EOF expect_log "bin-message" } +function test_java_rbe_coverage_produces_report() { + mkdir -p java/factorial + + JAVA_TOOLS_ZIP="released" + COVERAGE_GENERATOR_DIR="released" + + cd java/factorial + + cat > BUILD <<'EOF' +java_library( + name = "fact", + srcs = ["Factorial.java"], +) +java_test( + name = "fact-test", + size = "small", + srcs = ["FactorialTest.java"], + test_class = "factorial.FactorialTest", + deps = [ + ":fact", + ], +) +EOF + + cat > Factorial.java <<'EOF' +package factorial; +public class Factorial { + public static int factorial(int x) { + return x <= 0 ? 1 : x * factorial(x-1); + } +} +EOF + + cat > FactorialTest.java <<'EOF' +package factorial; +import static org.junit.Assert.*; +import org.junit.Test; +public class FactorialTest { + @Test + public void testFactorialOfZeroIsOne() throws Exception { + assertEquals(Factorial.factorial(3),6); + } +} +EOF + cd ../.. + + cat $(rlocation io_bazel/src/test/shell/bazel/testdata/jdk_http_archives) >> WORKSPACE + + bazel coverage \ + --test_output=all \ + --experimental_fetch_all_coverage_outputs \ + --experimental_split_coverage_postprocessing \ + --remote_download_minimal \ + --combined_report=lcov \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --instrumentation_filter=//java/factorial \ + //java/factorial:fact-test &> $TEST_log || fail "Shouldn't fail" + + # Test binary shouldn't be downloaded + [[ ! -e "bazel-bin/java/factorial/libfact.jar" ]] || fail "bazel-bin/java/factorial/libfact.jar shouldn't exist!" + + local expected_result="SF:java/factorial/Factorial.java +FN:2,factorial/Factorial:: ()V +FN:4,factorial/Factorial::factorial (I)I +FNDA:0,factorial/Factorial:: ()V +FNDA:1,factorial/Factorial::factorial (I)I +FNF:2 +FNH:1 +BRDA:4,0,0,1 +BRDA:4,0,1,1 +BRF:2 +BRH:2 +DA:2,0 +DA:4,1 +LH:1 +LF:2 +end_of_record" + cat bazel-testlogs/java/factorial/fact-test/coverage.dat > $TEST_log + expect_log "$expected_result" + cat bazel-out/_coverage/_coverage_report.dat > $TEST_log + expect_log "$expected_result" +} + run_suite "Build without the Bytes tests"