Skip to content

Commit

Permalink
Ensure toplevel artifacts are downloaded before sending completion ev…
Browse files Browse the repository at this point in the history 
…ent.

Fixes bazelbuild#20737.

PiperOrigin-RevId: 632430888
Change-Id: I1f2e1034c73162e925e25eb57b31d6a32b30fad2
  • Loading branch information
@coeuvre
coeuvre committed May 10, 2024
1 parent 7d6b78e commit eedb775
Show file tree
Hide file tree
Showing 7 changed files with 274 additions and 22 deletions.
7 changes: 5 additions & 2 deletions src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand All @@ -46,13 +47,15 @@ static SkyFunction aspectCompletionFunction(
PathResolverFactory pathResolverFactory,
SkyframeActionExecutor skyframeActionExecutor,
MetadataConsumerForMetrics.FilesMetricConsumer topLevelArtifactsMetric,
BugReporter bugReporter) {
BugReporter bugReporter,
Supplier<Boolean> isSkymeld) {
return new CompletionFunction<>(
pathResolverFactory,
new AspectCompletor(),
skyframeActionExecutor,
topLevelArtifactsMetric,
bugReporter);
bugReporter,
isSkymeld);
}

@Override
Expand Down
115 changes: 114 additions & 1 deletion 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 @@ -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;
Expand All @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -114,18 +127,21 @@ EventReportingArtifacts createSucceeded(
private final SkyframeActionExecutor skyframeActionExecutor;
private final FilesMetricConsumer topLevelArtifactsMetric;
private final BugReporter bugReporter;
private final Supplier<Boolean> isSkymeld;

CompletionFunction(
PathResolverFactory pathResolverFactory,
Completor<ValueT, ResultT, KeyT, FailureT> completor,
SkyframeActionExecutor skyframeActionExecutor,
FilesMetricConsumer topLevelArtifactsMetric,
BugReporter bugReporter) {
BugReporter bugReporter,
Supplier<Boolean> isSkymeld) {
this.pathResolverFactory = pathResolverFactory;
this.completor = completor;
this.skyframeActionExecutor = skyframeActionExecutor;
this.topLevelArtifactsMetric = topLevelArtifactsMetric;
this.bugReporter = bugReporter;
this.isSkymeld = isSkymeld;
}

@SuppressWarnings("unchecked") // Cast to KeyT
Expand Down Expand Up @@ -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) {
Expand All @@ -305,6 +323,101 @@ public SkyValue compute(SkyKey skyKey, Environment env)
return completor.getResult();
}

private void ensureToplevelArtifacts(
Environment env, ImmutableCollection<Artifact> 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<ListenableFuture<Void>>();
for (var artifact : artifacts) {
if (!(artifact instanceof DerivedArtifact derivedArtifact)) {
continue;
}

if (artifact.isTreeArtifact()) {
var treeMetadata = checkNotNull(inputMap.getTreeMetadata(artifact.getExecPath()));
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()
&& !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,
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,16 @@ public void setClientEnv(Map<String, String> 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()
Expand Down
45 changes: 28 additions & 17 deletions src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -673,7 +674,7 @@ private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions() {
SkyFunctions.REPO_FILE,
shouldUseRepoDotBazel
? new RepoFileFunction(
ruleClassProvider.getBazelStarlarkEnvironment(), directories.getWorkspace())
ruleClassProvider.getBazelStarlarkEnvironment(), directories.getWorkspace())
: (k, env) -> {
throw new IllegalStateException("supposed to be unused");
});
Expand All @@ -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,
Expand Down Expand Up @@ -869,7 +878,6 @@ protected final void init() {
progressReceiver = newSkyframeProgressReceiver();
memoizingEvaluator = createEvaluator(skyFunctions(), progressReceiver, emittedEventState);
skyframeExecutorConsumerOnInit.accept(this);

}

@ForOverride
Expand Down Expand Up @@ -1279,8 +1287,8 @@ public ImmutableList<Artifact> getWorkspaceStatusArtifacts(ExtendedEventHandler
EvaluationResult<WorkspaceStatusValue> 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());
Expand Down Expand Up @@ -1541,7 +1549,7 @@ public EvaluationResult<?> buildArtifacts(
Iterable<SkyKey> aspectKeys = AspectCompletionValue.keys(aspects, topLevelArtifactContext);
Iterable<SkyKey> testKeys =
TestCompletionValue.keys(
parallelTests, topLevelArtifactContext, /*exclusiveTesting=*/ false);
parallelTests, topLevelArtifactContext, /* exclusiveTesting= */ false);
EvaluationContext evaluationContext =
newEvaluationContextBuilder()
.setKeepGoing(options.getOptions(KeepGoingOption.class).keepGoing)
Expand Down Expand Up @@ -1604,11 +1612,13 @@ public EvaluationResult<?> runExclusiveTest(
try {
Iterable<SkyKey> 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.
Expand Down Expand Up @@ -2086,8 +2096,8 @@ TopLevelActionConflictReport filterActionConflictsForConfiguredTargetsAndAspects
EvaluationResult<ActionLookupConflictFindingValue> 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
Expand Down Expand Up @@ -2191,8 +2201,8 @@ public Predicate<Artifact> filterActionConflictsForTopLevelArtifacts(
EvaluationResult<ActionLookupConflictFindingValue> 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.
Expand All @@ -2213,7 +2223,7 @@ public Predicate<Artifact> filterActionConflictsForTopLevelArtifacts(
public final EvaluationResult<SkyValue> prepareAndGet(
Set<SkyKey> roots, EvaluationContext evaluationContext) throws InterruptedException {
EvaluationContext evaluationContextToUse =
evaluationContext.builder().setKeepGoing(/*keepGoing=*/ true).build();
evaluationContext.builder().setKeepGoing(/* keepGoing= */ true).build();
return memoizingEvaluator.evaluate(roots, evaluationContextToUse);
}

Expand Down Expand Up @@ -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) {
Expand Down
7 changes: 5 additions & 2 deletions src/main/java/com/google/devtools/build/lib/skyframe/TargetCompletor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -49,13 +50,15 @@ static SkyFunction targetCompletionFunction(
PathResolverFactory pathResolverFactory,
SkyframeActionExecutor skyframeActionExecutor,
MetadataConsumerForMetrics.FilesMetricConsumer topLevelArtifactsMetric,
BugReporter bugReporter) {
BugReporter bugReporter,
Supplier<Boolean> isSkymeld) {
return new CompletionFunction<>(
pathResolverFactory,
new TargetCompletor(skyframeActionExecutor),
skyframeActionExecutor,
topLevelArtifactsMetric,
bugReporter);
bugReporter,
isSkymeld);
}

private TargetCompletor(SkyframeActionExecutor announceTargetSummaries) {
Expand Down
2 changes: 2 additions & 0 deletions src/test/java/com/google/devtools/build/lib/remote/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Loading

0 comments on commit eedb775

Please sign in to comment.