From 707e2f08e3d0162726b5632d22eab8f87eeab2eb 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 +++++++++++------- 3 files changed, 133 insertions(+), 78 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,