diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestAttempt.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestAttempt.java index 3b2c2f522d2e00..528fde531a968b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestAttempt.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestAttempt.java @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMultimap; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType; import com.google.devtools.build.lib.buildeventstream.BuildEventContext; import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil; @@ -29,6 +30,7 @@ import com.google.devtools.build.lib.buildeventstream.BuildEventWithOrderConstraint; import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent; import com.google.devtools.build.lib.buildeventstream.PathConverter; +import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants; import com.google.devtools.build.lib.runtime.BuildEventStreamerUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus; @@ -38,6 +40,7 @@ import java.util.Collection; import java.util.List; import java.util.Map; +import javax.annotation.Nullable; /** This event is raised whenever an individual test attempt is completed. */ public class TestAttempt implements BuildEventWithOrderConstraint { @@ -49,6 +52,15 @@ public class TestAttempt implements BuildEventWithOrderConstraint { private final int attempt; private final boolean lastAttempt; private final ImmutableMultimap files; + + /** + * Metadata of the test log, if known. + * + *

The test log is a declared output of the test action, so with {@code + * --remote_download_minimal} it may not exist on the local filesystem. Carrying its metadata lets + * the build event artifact uploader report it without having to read it. + */ + @Nullable private final FileArtifactValue testLogMetadata; private final List testWarnings; private final long durationMillis; private final long startTimeMillis; @@ -71,6 +83,7 @@ private TestAttempt( long startTimeMillis, long durationMillis, ImmutableMultimap files, + @Nullable FileArtifactValue testLogMetadata, List testWarnings, boolean lastAttempt) { this.testAction = testAction; @@ -82,6 +95,7 @@ private TestAttempt( this.startTimeMillis = startTimeMillis; this.durationMillis = durationMillis; this.files = Preconditions.checkNotNull(files); + this.testLogMetadata = testLogMetadata; this.testWarnings = Preconditions.checkNotNull(testWarnings); this.lastAttempt = lastAttempt; } @@ -95,6 +109,7 @@ public static TestAttempt forExecutedTestResult( TestResultData attemptData, int attempt, ImmutableMultimap files, + @Nullable FileArtifactValue testLogMetadata, BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo, boolean lastAttempt) { return new TestAttempt( @@ -107,6 +122,7 @@ public static TestAttempt forExecutedTestResult( attemptData.getStartTimeMillisEpoch(), attemptData.getRunDurationMillis(), files, + testLogMetadata, attemptData.getWarningList(), lastAttempt); } @@ -132,6 +148,7 @@ public static TestAttempt fromCachedTestResult( attemptData.getStartTimeMillisEpoch(), attemptData.getRunDurationMillis(), files, + /* testLogMetadata= */ null, attemptData.getWarningList(), lastAttempt); } @@ -155,6 +172,7 @@ public static TestAttempt forUnstartableTestResult( attemptData.getStartTimeMillisEpoch(), attemptData.getRunDurationMillis(), /* files= */ ImmutableMultimap.of(), + /* testLogMetadata= */ null, attemptData.getWarningList(), /* lastAttempt= */ true); } @@ -230,8 +248,13 @@ public ImmutableList referencedLocalFiles() { ImmutableList.Builder localFiles = ImmutableList.builder(); for (Map.Entry file : files.entries()) { if (file.getValue() != null) { - // TODO(b/199940216): Can we populate metadata for these files? - localFiles.add(new LocalFile(file.getValue(), localFileType, /* artifactMetadata= */ null)); + // Only the test log and the coverage data are declared outputs of the test action; the + // remaining files are produced by the test spawn and always exist locally. + localFiles.add( + new LocalFile( + file.getValue(), + localFileType, + TestFileNameConstants.TEST_LOG.equals(file.getKey()) ? testLogMetadata : null)); } } return localFiles.build(); 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 895b033a0f388f..f60ad88af31535 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -413,6 +413,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:action_input", "//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/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", @@ -425,6 +426,7 @@ java_library( "//src/main/protobuf:failure_details_java_proto", "//src/main/protobuf:test_status_java_proto", "//third_party:guava", + "//third_party:jsr305", "@com_google_protobuf//:protobuf_java", "@com_google_protobuf//:protobuf_java_util", ], 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 51558c98a3d629..c0e5707e41365c 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 @@ -31,6 +31,7 @@ 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.FileArtifactValue; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.ExecutionRequirements; @@ -58,6 +59,7 @@ import com.google.devtools.build.lib.server.FailureDetails.TestAction; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.FileStatus; +import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -73,6 +75,7 @@ import java.util.List; import java.util.Map; import java.util.TreeMap; +import javax.annotation.Nullable; /** Runs TestRunnerAction actions. */ // TODO(bazel-team): add tests for this strategy. @@ -242,6 +245,42 @@ private void finalizeTest( postTestResult(actionExecutionContext, result); } + /** + * Returns {@code testOutputs} with every path on the filesystem of {@code execRoot}. + * + *

The test log and the coverage data are declared outputs of the test action, so they are + * resolved through the action filesystem. That filesystem is scoped to the action, while the + * paths below are reported in build events, which are uploaded asynchronously and thus outlive + * it. See https://github.com/bazelbuild/bazel/issues/24527. + */ + private static ImmutableMultimap onFileSystemOf( + ImmutableMultimap testOutputs, Path execRoot) { + FileSystem fileSystem = execRoot.getFileSystem(); + ImmutableMultimap.Builder builder = ImmutableMultimap.builder(); + testOutputs.forEach((name, path) -> builder.put(name, fileSystem.getPath(path.asFragment()))); + return builder.build(); + } + + /** + * Returns the metadata of the test log, or null if it is unavailable. + * + *

The test log is a declared output of the test action, so with {@code + * --remote_download_minimal} it may not exist on the local filesystem. Its metadata lets the build + * event artifact uploader report it without reading it. + */ + @Nullable + private static FileArtifactValue getTestLogMetadata( + ActionExecutionContext actionExecutionContext, TestRunnerAction action) { + try { + return actionExecutionContext.getOutputMetadataStore().getOutputMetadata(action.getTestLog()); + } catch (IOException | InterruptedException e) { + if (e instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } + return null; + } + } + private StandaloneProcessedAttemptResult processTestAttempt( int attemptId, boolean isLastAttempt, @@ -283,13 +322,21 @@ private StandaloneProcessedAttemptResult processTestAttempt( // Add the test log to the output TestResultData data = dataBuilder.build(); + ImmutableMultimap reportedOutputs = + onFileSystemOf(testOutputs, actionExecutionContext.getExecRoot()); actionExecutionContext .getEventHandler() .post( TestAttempt.forExecutedTestResult( - action, data, attemptId, testOutputs, result.executionInfo(), isLastAttempt)); + action, + data, + attemptId, + reportedOutputs, + getTestLogMetadata(actionExecutionContext, action), + result.executionInfo(), + isLastAttempt)); processTestOutput(actionExecutionContext, data, action.getTestName(), renamedTestLog); - return new StandaloneProcessedAttemptResult(data, testOutputs); + return new StandaloneProcessedAttemptResult(data, reportedOutputs); } private static Map setupEnvironment( @@ -543,7 +590,12 @@ public TestResult newCachedTestResult( TestResultData cachedResult, ImmutableMultimap testOutputs) { return new TestResult( - action, cachedResult, testOutputs, /* cached= */ true, execRoot, /* systemFailure= */ null); + action, + cachedResult, + onFileSystemOf(testOutputs, execRoot), + /* cached= */ true, + execRoot, + /* systemFailure= */ null); } @VisibleForTesting diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java index 84861540650f4d..97b6e938920287 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.remote; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import static com.google.devtools.build.lib.remote.util.DigestUtil.isOldStyleDigestFunction; import static com.google.devtools.build.lib.remote.util.RxFutures.toCompletable; @@ -30,6 +31,7 @@ import com.google.common.collect.Maps; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.buildeventstream.BuildEvent; import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile; import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType; import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; @@ -109,9 +111,20 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted } /** Returns {@code true} if Bazel knows that the file is stored on a remote system. */ - private static boolean isRemoteFile(Path file) throws IOException { - return file.getFileSystem() instanceof RemoteActionFileSystem - && ((RemoteActionFileSystem) file.getFileSystem()).isRemote(file); + /** + * Checks that a build event does not reference an action filesystem. + * + *

A build event outlives the action it refers to, since it is uploaded asynchronously, so + * referencing one keeps everything it needs to serve that action's inputs on the heap until the + * event has been delivered. Events report paths on the local filesystem and supply {@link + * BuildEvent.LocalFile#artifactMetadata} for files that may not exist there. See + * https://github.com/bazelbuild/bazel/issues/24527. + */ + private static void checkNotActionFileSystem(Path path) { + checkArgument( + !(path.getFileSystem() instanceof RemoteActionFileSystem), + "build event references an action filesystem: %s", + path); } private static final class PathMetadata { @@ -248,7 +261,8 @@ private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOExcept digest, /* directory= */ false, /* symlink= */ false, - isRemoteFile(path), + // A file that is only stored remotely is reported through its metadata above. + /* remote= */ false, isBuildToolLog, digestFunction); } @@ -392,6 +406,8 @@ private Single doUpload(Map files) { return Single.just(PathConverter.NO_CONVERSION); } + files.keySet().forEach(ByteStreamBuildEventArtifactUploader::checkNotActionFileSystem); + RequestMetadata metadata = TracingMetadataUtils.buildMetadata(buildRequestId, commandId, "bes-upload"); RemoteActionExecutionContext context = diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java index 09edcf66524e4a..7c43df5bcdf886 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java @@ -20,9 +20,7 @@ import com.google.common.eventbus.Subscribe; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; -import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.LostInputsActionExecutionException; import com.google.devtools.build.lib.actions.OutputChecker; @@ -209,24 +207,6 @@ public void clean() { // Intentionally left empty. } - @Override - public boolean supportsPathResolverForArtifactValues() { - return actionFileSystemType() != ActionFileSystemType.DISABLED; - } - - @Override - public ArtifactPathResolver createPathResolverForArtifactValues( - PathFragment execRoot, - String relativeOutputPath, - FileSystem fileSystem, - ImmutableList pathEntries, - ActionInputMap actionInputMap) { - FileSystem remoteFileSystem = - new RemoteActionFileSystem( - fileSystem, execRoot, relativeOutputPath, actionInputMap, actionInputFetcher); - return ArtifactPathResolver.createPathResolver(remoteFileSystem, fileSystem.getPath(execRoot)); - } - @Override public void checkActionFileSystemForLostInputs(FileSystem actionFileSystem, Action action) throws LostInputsActionExecutionException { diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java index b22dc64184bae8..74bc0541391fed 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java @@ -509,7 +509,7 @@ private void maybeReportArtifactSet(CompletionContext ctx, NestedSet set) { for (NestedSet succ : set.getNonLeaves()) { maybeReportArtifactSet(ctx, succ); } - post(new NamedArtifactGroup(lockedName.getName(), ctx, set)); + post(new NamedArtifactGroup(lockedName.getName(), ctx.pathResolver(), set)); } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java b/src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java index c55bcd0da98178..e53e785017e9e7 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.CompletionContext; import com.google.devtools.build.lib.actions.CompletionContext.ArtifactReceiver; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -45,16 +46,16 @@ */ class NamedArtifactGroup implements BuildEvent { private final String name; - private final CompletionContext completionContext; + private final ArtifactPathResolver pathResolver; private final NestedSet set; // of Artifact or ExpandedArtifact /** * Create a {@link NamedArtifactGroup}. Although the set may contain a mixture of Artifacts and * ExpandedArtifacts, all its leaf successors ("direct elements") are ExpandedArtifacts. */ - NamedArtifactGroup(String name, CompletionContext completionContext, NestedSet set) { + NamedArtifactGroup(String name, ArtifactPathResolver pathResolver, NestedSet set) { this.name = name; - this.completionContext = completionContext; + this.pathResolver = pathResolver; this.set = set; } @@ -76,14 +77,14 @@ public Collection referencedLocalFiles() { case NormalExpandedArtifact(Artifact artifact, FileArtifactValue metadata) -> { artifacts.add( new LocalFile( - completionContext.pathResolver().toPath(artifact), + pathResolver.toPath(artifact), LocalFileType.forArtifact(artifact, metadata), metadata)); } case FilesetExpandedArtifact(Artifact fileset, FilesetOutputSymlink link) -> { artifacts.add( new LocalFile( - completionContext.pathResolver().toPath(link.target()), + pathResolver.toPath(link.target()), LocalFileType.forArtifact(link.target(), link.metadata()), link.metadata())); } @@ -103,12 +104,11 @@ public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext convert BuildEventStreamProtos.File file = switch ((ExpandedArtifact) elem) { case NormalExpandedArtifact(Artifact artifact, FileArtifactValue metadata) -> { - String uri = pathConverter.apply(completionContext.pathResolver().toPath(artifact)); + String uri = pathConverter.apply(pathResolver.toPath(artifact)); yield TargetCompleteEvent.newFile(artifact, metadata, uri); } case FilesetExpandedArtifact(Artifact fileset, FilesetOutputSymlink link) -> { - String uri = - pathConverter.apply(completionContext.pathResolver().toPath(link.target())); + String uri = pathConverter.apply(pathResolver.toPath(link.target())); yield TargetCompleteEvent.newFile( fileset.getRoot(), fileset.getRootRelativePath().getRelative(link.name()), diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 956f51e6b9c3cd..059fc0a753058f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -1948,7 +1948,20 @@ private void informImportantOutputHandlerIfNecessary(FileOutErr outErr) { } } - private static void reportActionExecution( + /** + * Returns {@code path} on the filesystem that build events should reference. + * + *

A build event outlives the action it refers to, so it must not hold on to the action + * filesystem. That is only possible if the files also exist on the local filesystem, which is not + * the case for an output service whose action filesystem takes full control of the output base. + */ + private Path pathForBuildEvent(Path path) { + return outputService.actionFileSystemType().supportsLocalActions() + ? executorEngine.getExecRoot().getFileSystem().getPath(path.asFragment()) + : path; + } + + private void reportActionExecution( ExtendedEventHandler eventHandler, Path primaryOutputPath, @Nullable FileArtifactValue primaryOutputMetadata, @@ -1961,10 +1974,10 @@ private static void reportActionExecution( Path stderr = null; if (outErr.hasRecordedStdout()) { - stdout = outErr.getOutputPath(); + stdout = pathForBuildEvent(outErr.getOutputPath()); } if (outErr.hasRecordedStderr()) { - stderr = outErr.getErrorPath(); + stderr = pathForBuildEvent(outErr.getErrorPath()); } // Collect MetadataLogs and spawn start times/end times from the Action's SpawnResults. ImmutableList spawnResults = @@ -1985,7 +1998,7 @@ private static void reportActionExecution( action.getPrimaryOutput().getExecPath(), action, exception, - primaryOutputPath, + pathForBuildEvent(primaryOutputPath), action.getPrimaryOutput(), primaryOutputMetadata, stdout, diff --git a/src/test/java/com/google/devtools/build/lib/exec/BUILD b/src/test/java/com/google/devtools/build/lib/exec/BUILD index 239584fb387063..43b456c19e0d92 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/test/java/com/google/devtools/build/lib/exec/BUILD @@ -37,6 +37,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:virtual_action_input", "//src/main/java/com/google/devtools/build/lib/analysis:actions/symlink_action", "//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/analysis:blaze_directories", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:server_directories", 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 3d9596984020f7..d642db9fd6c19c 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 @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.Iterables; import com.google.common.collect.MoreCollectors; import com.google.devtools.build.lib.actions.ActionContext; @@ -36,17 +37,17 @@ import com.google.devtools.build.lib.actions.DiscoveredModulesPruner; import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.Spawn; -import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnResult.Status; +import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnStrategy; import com.google.devtools.build.lib.actions.ThreadStateReceiver; import com.google.devtools.build.lib.actions.cache.OutputMetadataStore; 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; import com.google.devtools.build.lib.analysis.test.TestActionContext.AttemptGroup; import com.google.devtools.build.lib.analysis.test.TestActionContext.ProcessedAttemptResult; import com.google.devtools.build.lib.analysis.test.TestActionContext.TestRunnerSpawn; +import com.google.devtools.build.lib.analysis.test.TestActionContext; import com.google.devtools.build.lib.analysis.test.TestAttempt; import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions.CancelConcurrentTests; import com.google.devtools.build.lib.analysis.test.TestProvider; @@ -56,6 +57,7 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.TestResult.ExecutionInfo; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.TestStatus; +import com.google.devtools.build.lib.buildeventstream.TestFileNameConstants; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.clock.Clock; import com.google.devtools.build.lib.events.Event; @@ -66,15 +68,17 @@ import com.google.devtools.build.lib.exec.util.FakeActionInputFileCache; import com.google.devtools.build.lib.exec.util.TestExecutorBuilder; import com.google.devtools.build.lib.runtime.TestSummaryOptions; -import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code; +import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.io.FileOutErr; +import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.SyscallCache; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus; import com.google.devtools.build.lib.view.test.TestStatus.TestResultData; import com.google.devtools.common.options.Options; @@ -225,6 +229,45 @@ private static FileOutErr createTempOutErr(Path tmpDirRoot) { return new FileOutErr(outPath, errPath); } + @Test + public void cachedTestResult_outputsNotOnActionFileSystem() throws Exception { + ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class); + Path tmpDirRoot = TestStrategy.getTmpRoot(rootDirectory, outputBase, executionOptions); + TestedStandaloneTestStrategy standaloneTestStrategy = + new TestedStandaloneTestStrategy(executionOptions, TestSummaryOptions.DEFAULTS, tmpDirRoot); + scratch.file("standalone/simple_test.sh", "this does not get executed, it is mocked out"); + scratch.file( + "standalone/BUILD", + """ + load('//test_defs:foo_test.bzl', 'foo_test') + foo_test( + name = "simple_test", + size = "small", + srcs = ["simple_test.sh"], + ) + """); + TestRunnerAction testRunnerAction = getTestAction("//standalone:simple_test"); + + // A declared test output is resolved through the action filesystem, which is scoped to the + // action, so it must not end up in the reported outputs. See #24527. + FileSystem actionFileSystem = new InMemoryFileSystem(DigestHashFunction.SHA256); + Path testLogOnActionFileSystem = + actionFileSystem.getPath(testRunnerAction.getTestLog().getPath().asFragment()); + + TestResult result = + standaloneTestStrategy.newCachedTestResult( + rootDirectory, + testRunnerAction, + TestResultData.newBuilder().setTestPassed(true).build(), + ImmutableMultimap.of(TestFileNameConstants.TEST_LOG, testLogOnActionFileSystem)); + + TestAttempt attempt = Iterables.getOnlyElement(result.getCachedTestAttempts()); + Path reported = + Iterables.getOnlyElement(attempt.getFiles().get(TestFileNameConstants.TEST_LOG)); + assertThat(reported.asFragment()).isEqualTo(testLogOnActionFileSystem.asFragment()); + assertThat(reported.getFileSystem()).isNotSameInstanceAs(actionFileSystem); + } + private TestRunnerAction getTestAction(String target) throws Exception { ConfiguredTarget configuredTarget = getConfiguredTarget(target); ImmutableList testStatusArtifacts = diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index d4b3c745ac74fb..5494f3d580632e 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -413,9 +413,10 @@ public void onCompleted() { } @Test - public void remoteFileShouldNotBeUploaded_actionFs() throws Exception { - // Test that we don't attempt to upload remotely stored file but convert the remote path - // to a bytestream:// URI. + public void remoteFileShouldNotBeUploaded_remoteMetadata() throws Exception { + // Test that we don't attempt to upload a remotely stored file, but convert it to a + // bytestream:// URI. Build events report such a file on the local filesystem, where it does not + // exist, and supply its metadata; see #24527. // arrange @@ -430,23 +431,15 @@ public void remoteFileShouldNotBeUploaded_actionFs() throws Exception { ActionInputMap outputs = new ActionInputMap(2); Artifact artifact = createRemoteArtifact("file1.txt", "foo", outputs); - RemoteActionFileSystem remoteFs = - new RemoteActionFileSystem( - fs, - execRoot.asFragment(), - outputRoot.getRoot().asPath().relativeTo(execRoot).getPathString(), - outputs, - actionInputFetcher); - Path remotePath = remoteFs.getPath(artifact.getPath().getPathString()); - assertThat(remotePath.getFileSystem()).isEqualTo(remoteFs); - LocalFile file = - new LocalFile(remotePath, LocalFileType.OUTPUT_FILE, /* artifactMetadata= */ null); + FileArtifactValue metadata = outputs.getInputMetadata(artifact); + Path remotePath = artifact.getPath(); + assertThat(remotePath.exists()).isFalse(); + LocalFile file = new LocalFile(remotePath, LocalFileType.OUTPUT_FILE, metadata); // act PathConverter pathConverter = artifactUploader.upload(ImmutableMap.of(remotePath, file)).get(); - FileArtifactValue metadata = outputs.getInputMetadata(artifact); Digest digest = DigestUtil.buildDigest(metadata.getDigest(), metadata.getSize()); // assert