From 8555cf0aabdababad739b38aa7d37d0d5fcdc322 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 26 Aug 2024 04:05:09 -0700 Subject: [PATCH] Download runfiles in CompletionFunction. Previously, we download outputs of toplevel artifacts in CompletionFunction because intermediate targets might be promoted to toplevel by skymeld in the middle of the build. However, it didn't handle runfiles of the target. This CL fixes that. A test case will be added in a followup CL because it requires changes to skymeld & BwoB to unveil the bug. Working towards #22367. PiperOrigin-RevId: 667532830 Change-Id: Idfe2a0b693f103d501c9b24a454c16186468667a --- .../lib/analysis/AspectCompleteEvent.java | 4 + .../lib/analysis/TargetCompleteEvent.java | 12 ++ .../lib/skyframe/CompletionFunction.java | 195 +++++++++++------- .../remote/build_without_the_bytes_test.sh | 2 +- 4 files changed, 134 insertions(+), 79 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java index 438863e9167daa..b2b39ff6a62f93 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java @@ -113,6 +113,10 @@ public ArtifactsInOutputGroup getArtifacts(String outputGroup) { return artifactOutputGroups.get(outputGroup); } + public ImmutableMap getOutputs() { + return artifactOutputGroups; + } + public CompletionContext getCompletionContext() { return completionContext; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java index 52316f713302d9..d87e1c892cb075 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java @@ -102,6 +102,14 @@ public Path getRunfilesDirectory() { return null; } + @Nullable + public Runfiles getRunfiles() { + if (runfilesSupport != null) { + return runfilesSupport.getRunfiles(); + } + return null; + } + @Nullable public Artifact getExecutable() { return executable; @@ -298,6 +306,10 @@ public ArtifactsInOutputGroup getOutputGroup(String outputGroup) { return outputs.get(outputGroup); } + public ImmutableMap getOutputs() { + return outputs; + } + // TODO(aehlig): remove as soon as we managed to get rid of the deprecated "important_output" // field. 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 07e94efde35fc3..d295bb33f1c151 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 @@ -38,8 +38,11 @@ import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.actions.InputFileErrorException; import com.google.devtools.build.lib.actions.RemoteArtifactChecker; +import com.google.devtools.build.lib.analysis.AspectCompleteEvent; import com.google.devtools.build.lib.analysis.ConfiguredObjectValue; import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.Runfiles; +import com.google.devtools.build.lib.analysis.TargetCompleteEvent; import com.google.devtools.build.lib.analysis.TopLevelArtifactContext; import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper; import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup; @@ -69,6 +72,7 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutionException; @@ -326,13 +330,12 @@ 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) { return null; } + ensureToplevelArtifacts(env, postable, inputMap); env.getListener().post(postable); topLevelArtifactsMetric.mergeIn(currentConsumer); @@ -340,7 +343,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) } private void ensureToplevelArtifacts( - Environment env, ImmutableCollection artifacts, ActionInputMap inputMap) + Environment env, ExtendedEventHandler.Postable postable, 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 @@ -364,86 +367,33 @@ private void ensureToplevelArtifacts( return; } + ImmutableMap allOutputGroups; + Runfiles runfiles = null; + if (postable instanceof TargetCompleteEvent targetCompleteEvent) { + allOutputGroups = targetCompleteEvent.getOutputs(); + runfiles = targetCompleteEvent.getExecutableTargetData().getRunfiles(); + } else if (postable instanceof AspectCompleteEvent aspectCompleteEvent) { + allOutputGroups = aspectCompleteEvent.getOutputs(); + } else { + return; + } + var futures = new ArrayList>(); - for (var artifact : artifacts) { - if (!(artifact instanceof DerivedArtifact derivedArtifact)) { + for (var outputGroup : allOutputGroups.values()) { + if (!outputGroup.areImportant()) { continue; } - // Metadata can be null during error bubbling, only download outputs that are already - // generated. b/342188273 - if (artifact.isTreeArtifact()) { - var treeMetadata = inputMap.getTreeMetadata(artifact.getExecPath()); - if (treeMetadata == null) { - continue; - } - - var filesToDownload = new ArrayList(treeMetadata.getChildValues().size()); - for (var child : treeMetadata.getChildValues().entrySet()) { - var treeFile = child.getKey(); - var metadata = child.getValue(); - if (metadata.isRemote() - && !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 = inputMap.getInputMetadata(artifact); - if (metadata == null) { - continue; - } + for (var artifact : outputGroup.getArtifacts().toList()) { + downloadArtifact( + env, remoteArtifactChecker, actionInputPrefetcher, inputMap, artifact, futures); + } + } - if (metadata.isRemote() - && !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()) - .setRemoteExecution( - RemoteExecution.newBuilder() - .setCode( - RemoteExecution.Code - .TOPLEVEL_OUTPUTS_DOWNLOAD_FAILURE) - .build()) - .build()))), - directExecutor())); - } + if (runfiles != null) { + for (var artifact : runfiles.getAllArtifacts().toList()) { + downloadArtifact( + env, remoteArtifactChecker, actionInputPrefetcher, inputMap, artifact, futures); } } @@ -458,6 +408,95 @@ private void ensureToplevelArtifacts( } } + private void downloadArtifact( + Environment env, + RemoteArtifactChecker remoteArtifactChecker, + ActionInputPrefetcher actionInputPrefetcher, + ActionInputMap inputMap, + Artifact artifact, + List> futures + ) throws InterruptedException { + if (!(artifact instanceof DerivedArtifact derivedArtifact)) { + return; + } + + // Metadata can be null during error bubbling, only download outputs that are already + // generated. b/342188273 + if (artifact.isTreeArtifact()) { + var treeMetadata = inputMap.getTreeMetadata(artifact.getExecPath()); + if (treeMetadata == null) { + return; + } + + var filesToDownload = new ArrayList(treeMetadata.getChildValues().size()); + for (var child : treeMetadata.getChildValues().entrySet()) { + var treeFile = child.getKey(); + var metadata = child.getValue(); + if (metadata.isRemote() + && !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 = inputMap.getInputMetadata(artifact); + if (metadata == null) { + return; + } + + if (metadata.isRemote() + && !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()) + .setRemoteExecution( + RemoteExecution.newBuilder() + .setCode( + RemoteExecution.Code + .TOPLEVEL_OUTPUTS_DOWNLOAD_FAILURE) + .build()) + .build()))), + directExecutor())); + } + } + } + private void handleSourceFileError( Artifact input, DetailedExitCode detailedExitCode, 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 d800d7a50c6b85..5b7cee2740601a 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 @@ -420,7 +420,7 @@ cc_binary( ) EOF - bazel build \ + bazel --host_jvm_debug build \ --remote_executor=grpc://localhost:${worker_port} \ --remote_download_toplevel \ //a:foo || fail "Failed to build //a:foobar"