From e8ff51f4d498380b7287df8b007f5bb0810c2b2c Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 18 Jan 2023 06:00:19 -0800 Subject: [PATCH] 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 71d01f3a6e7f68..3139ad3bdcc3c6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -2518,6 +2518,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_configuration", srcs = ["test/CoverageConfiguration.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 66ecb01b2e6ef0..5a442d856051b5 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 @@ -771,7 +771,10 @@ private TestAttemptResult runTestAttempt( 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 2f6c06ffb101e7..7baf2cacd55f5e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -203,6 +203,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 b1c26be5815b5d..096974263a7d21 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 @@ -1583,4 +1583,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"