From eedb7758e1aa46528e53f0b37fcae40c9a7b1e40 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 10 May 2024 02:39:06 -0700 Subject: [PATCH] Ensure toplevel artifacts are downloaded before sending completion event. Fixes https://github.com/bazelbuild/bazel/issues/20737. PiperOrigin-RevId: 632430888 Change-Id: I1f2e1034c73162e925e25eb57b31d6a32b30fad2 --- .../build/lib/skyframe/AspectCompletor.java | 7 +- .../lib/skyframe/CompletionFunction.java | 115 +++++++++++++++++- .../lib/skyframe/SkyframeActionExecutor.java | 10 ++ .../build/lib/skyframe/SkyframeExecutor.java | 45 ++++--- .../build/lib/skyframe/TargetCompletor.java | 7 +- .../google/devtools/build/lib/remote/BUILD | 2 + ...ildWithoutTheBytesIntegrationTestBase.java | 110 +++++++++++++++++ 7 files changed, 274 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletor.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletor.java index 6e7ee51103d2fe..c73356c7fc7985 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletor.java @@ -36,6 +36,7 @@ import com.google.devtools.build.lib.skyframe.CompletionFunction.Completor; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunction.Environment; +import java.util.function.Supplier; import javax.annotation.Nullable; /** Manages completing builds for aspects. */ @@ -46,13 +47,15 @@ static SkyFunction aspectCompletionFunction( PathResolverFactory pathResolverFactory, SkyframeActionExecutor skyframeActionExecutor, MetadataConsumerForMetrics.FilesMetricConsumer topLevelArtifactsMetric, - BugReporter bugReporter) { + BugReporter bugReporter, + Supplier isSkymeld) { return new CompletionFunction<>( pathResolverFactory, new AspectCompletor(), skyframeActionExecutor, topLevelArtifactsMetric, - bugReporter); + bugReporter, + isSkymeld); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java index 127fc19a374a99..a14ff34f156e2a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java @@ -13,18 +13,27 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.util.concurrent.MoreExecutors.directExecutor; + import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.actions.ActionExecutionException; +import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputMap; +import com.google.devtools.build.lib.actions.ActionInputPrefetcher.Priority; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; +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.CompletionContext; import com.google.devtools.build.lib.actions.CompletionContext.PathResolverFactory; import com.google.devtools.build.lib.actions.EventReportingArtifacts; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.actions.InputFileErrorException; import com.google.devtools.build.lib.analysis.ConfiguredObjectValue; @@ -42,6 +51,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.skyframe.ArtifactFunction.MissingArtifactValue; import com.google.devtools.build.lib.skyframe.ArtifactFunction.SourceArtifactException; import com.google.devtools.build.lib.skyframe.MetadataConsumerForMetrics.FilesMetricConsumer; @@ -52,10 +62,13 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.SkyframeLookupResult; +import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Set; +import java.util.concurrent.ExecutionException; +import java.util.function.Supplier; import javax.annotation.Nullable; import net.starlark.java.syntax.Location; @@ -114,18 +127,21 @@ EventReportingArtifacts createSucceeded( private final SkyframeActionExecutor skyframeActionExecutor; private final FilesMetricConsumer topLevelArtifactsMetric; private final BugReporter bugReporter; + private final Supplier isSkymeld; CompletionFunction( PathResolverFactory pathResolverFactory, Completor completor, SkyframeActionExecutor skyframeActionExecutor, FilesMetricConsumer topLevelArtifactsMetric, - BugReporter bugReporter) { + BugReporter bugReporter, + Supplier isSkymeld) { this.pathResolverFactory = pathResolverFactory; this.completor = completor; this.skyframeActionExecutor = skyframeActionExecutor; this.topLevelArtifactsMetric = topLevelArtifactsMetric; this.bugReporter = bugReporter; + this.isSkymeld = isSkymeld; } @SuppressWarnings("unchecked") // Cast to KeyT @@ -294,6 +310,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) return null; } + ensureToplevelArtifacts(env, allArtifacts, inputMap); + ExtendedEventHandler.Postable postable = completor.createSucceeded(key, value, ctx, artifactsToBuild, env); if (postable == null) { @@ -305,6 +323,101 @@ public SkyValue compute(SkyKey skyKey, Environment env) return completor.getResult(); } + private void ensureToplevelArtifacts( + Environment env, ImmutableCollection artifacts, ActionInputMap inputMap) + throws CompletionFunctionException, InterruptedException { + // For skymeld, a non-toplevel target might become a toplevel after it has been executed. This + // is the last chance to download the missing toplevel outputs in this case before sending out + // TargetCompleteEvent. See https://github.com/bazelbuild/bazel/issues/20737. + if (!isSkymeld.get()) { + return; + } + + var outputService = skyframeActionExecutor.getOutputService(); + var actionInputPrefetcher = skyframeActionExecutor.getActionInputPrefetcher(); + if (outputService == null || actionInputPrefetcher == null) { + return; + } + + var remoteArtifactChecker = outputService.getRemoteArtifactChecker(); + var futures = new ArrayList>(); + for (var artifact : artifacts) { + if (!(artifact instanceof DerivedArtifact derivedArtifact)) { + continue; + } + + if (artifact.isTreeArtifact()) { + var treeMetadata = checkNotNull(inputMap.getTreeMetadata(artifact.getExecPath())); + var filesToDownload = new ArrayList(treeMetadata.getChildValues().size()); + for (var child : treeMetadata.getChildValues().entrySet()) { + var treeFile = child.getKey(); + var metadata = child.getValue(); + if (metadata.isRemote() + && !treeFile.getPath().exists() + && !remoteArtifactChecker.shouldTrustRemoteArtifact( + treeFile, (RemoteFileArtifactValue) metadata)) { + filesToDownload.add(treeFile); + } + } + if (!filesToDownload.isEmpty()) { + var action = + ActionUtils.getActionForLookupData(env, derivedArtifact.getGeneratingActionKey()); + var future = + actionInputPrefetcher.prefetchFiles( + action, filesToDownload, inputMap::getInputMetadata, Priority.LOW); + futures.add( + Futures.catchingAsync( + future, + Throwable.class, + e -> + Futures.immediateFailedFuture( + new ActionExecutionException( + e, + action, + true, + DetailedExitCode.of( + FailureDetail.newBuilder().setMessage(e.getMessage()).build()))), + directExecutor())); + } + } else { + var metadata = checkNotNull(inputMap.getInputMetadata(artifact)); + if (metadata.isRemote() + && !artifact.getPath().exists() + && !remoteArtifactChecker.shouldTrustRemoteArtifact( + artifact, (RemoteFileArtifactValue) metadata)) { + var action = + ActionUtils.getActionForLookupData(env, derivedArtifact.getGeneratingActionKey()); + var future = + actionInputPrefetcher.prefetchFiles( + action, ImmutableList.of(artifact), inputMap::getInputMetadata, Priority.LOW); + futures.add( + Futures.catchingAsync( + future, + Throwable.class, + e -> + Futures.immediateFailedFuture( + new ActionExecutionException( + e, + action, + true, + DetailedExitCode.of( + FailureDetail.newBuilder().setMessage(e.getMessage()).build()))), + directExecutor())); + } + } + } + + try { + var unused = Futures.whenAllSucceed(futures).call(() -> null, directExecutor()).get(); + } catch (ExecutionException e) { + var cause = e.getCause(); + if (cause instanceof ActionExecutionException aee) { + throw new CompletionFunctionException(aee); + } + throw new RuntimeException(cause); + } + } + private void handleSourceFileError( Artifact input, DetailedExitCode detailedExitCode, 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 d13b191a1884f7..e6131360839bbe 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 @@ -329,6 +329,16 @@ public void setClientEnv(Map clientEnv) { this.clientEnv = ImmutableMap.copyOf(clientEnv); } + @Nullable + public OutputService getOutputService() { + return outputService; + } + + @Nullable + public ActionInputPrefetcher getActionInputPrefetcher() { + return actionInputPrefetcher; + } + ActionFileSystemType actionFileSystemType() { return outputService != null ? outputService.actionFileSystemType() diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 78cab3a77e582a..a27685dc05ee31 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -534,7 +534,8 @@ protected SkyframeExecutor( skyKeyStateReceiver::makeThreadStateReceiver); this.artifactFactory = new ArtifactFactory( - /*execRootParent=*/ directories.getExecRootBase(), directories.getRelativeOutputPath()); + /* execRootParent= */ directories.getExecRootBase(), + directories.getRelativeOutputPath()); this.skyframeBuildView = new SkyframeBuildView(artifactFactory, this, ruleClassProvider, actionKeyContext); this.externalFilesHelper = @@ -673,7 +674,7 @@ private ImmutableMap skyFunctions() { SkyFunctions.REPO_FILE, shouldUseRepoDotBazel ? new RepoFileFunction( - ruleClassProvider.getBazelStarlarkEnvironment(), directories.getWorkspace()) + ruleClassProvider.getBazelStarlarkEnvironment(), directories.getWorkspace()) : (k, env) -> { throw new IllegalStateException("supposed to be unused"); }); @@ -694,11 +695,19 @@ public SkyValue compute(SkyKey skyKey, Environment env) { map.put( SkyFunctions.TARGET_COMPLETION, TargetCompletor.targetCompletionFunction( - pathResolverFactory, skyframeActionExecutor, topLevelArtifactsMetric, bugReporter)); + pathResolverFactory, + skyframeActionExecutor, + topLevelArtifactsMetric, + bugReporter, + this::isMergedSkyframeAnalysisExecution)); map.put( SkyFunctions.ASPECT_COMPLETION, AspectCompletor.aspectCompletionFunction( - pathResolverFactory, skyframeActionExecutor, topLevelArtifactsMetric, bugReporter)); + pathResolverFactory, + skyframeActionExecutor, + topLevelArtifactsMetric, + bugReporter, + this::isMergedSkyframeAnalysisExecution)); map.put(SkyFunctions.TEST_COMPLETION, new TestCompletionFunction()); map.put( Artifact.ARTIFACT, @@ -869,7 +878,6 @@ protected final void init() { progressReceiver = newSkyframeProgressReceiver(); memoizingEvaluator = createEvaluator(skyFunctions(), progressReceiver, emittedEventState); skyframeExecutorConsumerOnInit.accept(this); - } @ForOverride @@ -1279,8 +1287,8 @@ public ImmutableList getWorkspaceStatusArtifacts(ExtendedEventHandler EvaluationResult result = evaluate( ImmutableList.of(WorkspaceStatusValue.BUILD_INFO_KEY), - /*keepGoing=*/ true, - /*numThreads=*/ 1, + /* keepGoing= */ true, + /* numThreads= */ 1, eventHandler); WorkspaceStatusValue value = checkNotNull(result.get(WorkspaceStatusValue.BUILD_INFO_KEY)); return ImmutableList.of(value.getStableArtifact(), value.getVolatileArtifact()); @@ -1541,7 +1549,7 @@ public EvaluationResult buildArtifacts( Iterable aspectKeys = AspectCompletionValue.keys(aspects, topLevelArtifactContext); Iterable testKeys = TestCompletionValue.keys( - parallelTests, topLevelArtifactContext, /*exclusiveTesting=*/ false); + parallelTests, topLevelArtifactContext, /* exclusiveTesting= */ false); EvaluationContext evaluationContext = newEvaluationContextBuilder() .setKeepGoing(options.getOptions(KeepGoingOption.class).keepGoing) @@ -1604,11 +1612,13 @@ public EvaluationResult runExclusiveTest( try { Iterable testKeys = TestCompletionValue.keys( - ImmutableSet.of(exclusiveTest), topLevelArtifactContext, /*exclusiveTesting=*/ true); + ImmutableSet.of(exclusiveTest), + topLevelArtifactContext, + /* exclusiveTesting= */ true); return evaluate( testKeys, - /*keepGoing=*/ options.getOptions(KeepGoingOption.class).keepGoing, - /*numThreads=*/ options.getOptions(BuildRequestOptions.class).jobs, + /* keepGoing= */ options.getOptions(KeepGoingOption.class).keepGoing, + /* numThreads= */ options.getOptions(BuildRequestOptions.class).jobs, reporter); } finally { // Also releases thread locks. @@ -2086,8 +2096,8 @@ TopLevelActionConflictReport filterActionConflictsForConfiguredTargetsAndAspects EvaluationResult result = evaluate( TopLevelActionLookupConflictFindingFunction.keys(keys, topLevelArtifactContext), - /*keepGoing=*/ true, - /*numThreads=*/ ResourceUsage.getAvailableProcessors(), + /* keepGoing= */ true, + /* numThreads= */ ResourceUsage.getAvailableProcessors(), eventHandler); // Remove top-level action-conflict detection values for memory efficiency. Non-top-level ones @@ -2191,8 +2201,8 @@ public Predicate filterActionConflictsForTopLevelArtifacts( EvaluationResult result = evaluate( Iterables.transform(artifacts, ActionLookupConflictFindingValue::key), - /*keepGoing=*/ true, - /*numThreads=*/ ResourceUsage.getAvailableProcessors(), + /* keepGoing= */ true, + /* numThreads= */ ResourceUsage.getAvailableProcessors(), eventHandler); // Remove remaining action-conflict detection values immediately for memory efficiency. @@ -2213,7 +2223,7 @@ public Predicate filterActionConflictsForTopLevelArtifacts( public final EvaluationResult prepareAndGet( Set roots, EvaluationContext evaluationContext) throws InterruptedException { EvaluationContext evaluationContextToUse = - evaluationContext.builder().setKeepGoing(/*keepGoing=*/ true).build(); + evaluationContext.builder().setKeepGoing(/* keepGoing= */ true).build(); return memoizingEvaluator.evaluate(roots, evaluationContextToUse); } @@ -2357,7 +2367,8 @@ Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier pkgName) // overall is in nokeep_going mode: the worst that happens is we parse some unnecessary // .bzl files. result = - evaluate(keys, /*keepGoing=*/ true, /*numThreads=*/ DEFAULT_THREAD_COUNT, eventHandler); + evaluate( + keys, /* keepGoing= */ true, /* numThreads= */ DEFAULT_THREAD_COUNT, eventHandler); } ErrorInfo error = result.getError(pkgName); if (error != null) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletor.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletor.java index 413b9132cd9c5a..f9ca04f81f5827 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletor.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.skyframe.TargetCompletionValue.TargetCompletionKey; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunction.Environment; +import java.util.function.Supplier; import javax.annotation.Nullable; import net.starlark.java.syntax.Location; @@ -49,13 +50,15 @@ static SkyFunction targetCompletionFunction( PathResolverFactory pathResolverFactory, SkyframeActionExecutor skyframeActionExecutor, MetadataConsumerForMetrics.FilesMetricConsumer topLevelArtifactsMetric, - BugReporter bugReporter) { + BugReporter bugReporter, + Supplier isSkymeld) { return new CompletionFunction<>( pathResolverFactory, new TargetCompletor(skyframeActionExecutor), skyframeActionExecutor, topLevelArtifactsMetric, - bugReporter); + bugReporter, + isSkymeld); } private TargetCompletor(SkyframeActionExecutor announceTargetSummaries) { diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 8f3070a6c0d1fe..364afac7d21aa2 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -197,8 +197,10 @@ 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:file_metadata", + "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value", "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", + "//src/main/java/com/google/devtools/build/lib/util:command", "//src/main/java/com/google/devtools/build/lib/util:os", "//src/main/java/com/google/devtools/build/lib/util/io", "//src/main/java/com/google/devtools/build/lib/vfs", diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java index f69e17d759c739..1c74b16e322a36 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java @@ -29,9 +29,11 @@ import com.google.devtools.build.lib.actions.BuildFailedException; import com.google.devtools.build.lib.actions.CachedActionEvent; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.analysis.TargetCompleteEvent; import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase; import com.google.devtools.build.lib.skyframe.ActionExecutionValue; import com.google.devtools.build.lib.skyframe.TreeArtifactValue; +import com.google.devtools.build.lib.util.CommandBuilder; import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.util.io.RecordingOutErr; import com.google.devtools.build.lib.vfs.Path; @@ -1649,6 +1651,114 @@ public void nonDeclaredSymlinksFromLocalActions() throws Exception { assertValidOutputFile("foobar.txt", "foo\nbar\n"); } + @Test + public void skymeldPromoIntermediateTargetToToplevel_outputFile_downloadFile() throws Exception { + // Regression test for https://github.com/bazelbuild/bazel/issues/20737. + + // Disable on Windows since mkfifo doesn't work there. + assumeFalse(OS.getCurrent() == OS.WINDOWS); + write( + "BUILD", + """ + filegroup(name = "top", srcs = [":actual", "//slow"]) + genrule(name = "proxy", srcs = [":actual"], outs = ["proxy_file"], cmd = "cp $< $@") + genrule(name = "actual", srcs = [], outs = ["actual_file"], cmd = "echo ACTUAL > $@") + """); + + getWorkspace().getRelative("slow").createDirectoryAndParents(); + // Only write the content of slow/BUILD after //:proxy is built, so we can artificially delay + // the analysis of //:top + var unused = + new CommandBuilder() + .addArgs("mkfifo", "slow/BUILD") + .setWorkingDir(getWorkspace()) + .build() + .execute(); + + buildTarget("//:proxy"); + restartServer(); + + getRuntimeWrapper() + .registerSubscriber( + new Object() { + @Subscribe + public void onTargetCompleted(TargetCompleteEvent event) { + if (event.getLabel().toString().equals("//:proxy")) { + try { + write( + "slow/BUILD", + "filegroup(name = 'slow', visibility = ['//visibility:public'])"); + } catch (IOException e) { + throw new IllegalStateException(e); + } + } + } + }); + setDownloadToplevel(); + buildTarget("//:top", "//:proxy"); + waitDownloads(); + + assertValidOutputFile("actual_file", "ACTUAL\n"); + } + + @Test + public void skymeldPromoIntermediateTargetToToplevel_outputDirectory_downloadDirectory() + throws Exception { + // Regression test for https://github.com/bazelbuild/bazel/issues/20737. + + // Disable on Windows since mkfifo doesn't work there. + assumeFalse(OS.getCurrent() == OS.WINDOWS); + writeOutputDirRule(); + write( + "BUILD", + """ + load(':output_dir.bzl', 'output_dir') + filegroup(name = "top", srcs = [":actual", "//slow"]) + genrule(name = "proxy", srcs = [":actual"], outs = ["proxy_file"], cmd = "cp $