Skip to content

Commit

Permalink
Download runfiles in CompletionFunction.
Browse files Browse the repository at this point in the history 
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 bazelbuild#22367.

PiperOrigin-RevId: 667532830
Change-Id: Idfe2a0b693f103d501c9b24a454c16186468667a
  • Loading branch information
@coeuvre
coeuvre committed Aug 28, 2024
1 parent a6f5449 commit 707e2f0
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 78 deletions.
4 changes: 4 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ public ArtifactsInOutputGroup getArtifacts(String outputGroup) {
return artifactOutputGroups.get(outputGroup);
}

public ImmutableMap<String, ArtifactsInOutputGroup> getOutputs() {
return artifactOutputGroups;
}

public CompletionContext getCompletionContext() {
return completionContext;
}
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -298,6 +306,10 @@ public ArtifactsInOutputGroup getOutputGroup(String outputGroup) {
return outputs.get(outputGroup);
}

public ImmutableMap<String, ArtifactsInOutputGroup> getOutputs() {
return outputs;
}

// TODO(aehlig): remove as soon as we managed to get rid of the deprecated "important_output"
// field.

Expand Down
195 changes: 117 additions & 78 deletions src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -326,21 +330,20 @@ 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);

return completor.getResult();
}

private void ensureToplevelArtifacts(
Environment env, ImmutableCollection<Artifact> 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
Expand All @@ -364,86 +367,33 @@ private void ensureToplevelArtifacts(
return;
}

ImmutableMap<String, ArtifactsInOutputGroup> 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<ListenableFuture<Void>>();
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<ActionInput>(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);
}
}

Expand All @@ -458,6 +408,95 @@ private void ensureToplevelArtifacts(
}
}

private void downloadArtifact(
Environment env,
RemoteArtifactChecker remoteArtifactChecker,
ActionInputPrefetcher actionInputPrefetcher,
ActionInputMap inputMap,
Artifact artifact,
List<ListenableFuture<Void>> 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<ActionInput>(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,
Expand Down

0 comments on commit 707e2f0

Please sign in to comment.