Skip to content

Commit

Permalink
Handle lost outputs in partially built top-level targets.
Browse files Browse the repository at this point in the history 
A partially built top-level target can occur if some but not all of its artifacts are successfully built. When this happens, the built artifacts are still reported in the BEP, so we need to treat them the same as outputs from a fully successful target.

Note that this change only handles the `--keep_going` case properly. Attempting to initiate rewinding during error bubbling won't work, so we'll need a different approach for `--nokeep_going`, which I will implement separately.

In addition to the new test case for this scenario, I added assertions to existing top-level output rewinding test cases to verify artifact reporting.

PiperOrigin-RevId: 606391715
Change-Id: Iaac24df75bfb15f10c046704dfb7ff1d807c1b8f
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed Feb 12, 2024
1 parent add245c commit 3bdeaea
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 17 deletions.
65 changes: 50 additions & 15 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 @@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputDepOwnerMap;
Expand Down Expand Up @@ -58,8 +59,10 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.SkyframeLookupResult;
import java.util.Collection;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import javax.annotation.Nullable;
import net.starlark.java.syntax.Location;

Expand Down Expand Up @@ -267,9 +270,23 @@ public SkyValue compute(SkyKey skyKey, Environment env)
workspaceNameValue.getName());

if (!rootCauses.isEmpty()) {
// TODO: b/321044105 - Check for lost built outputs in the error case.
ImmutableSet<Artifact> builtArtifacts = builtArtifactsBuilder.build();
if (!builtArtifacts.isEmpty()) {
Reset reset =
informImportantOutputHandler(
key,
env,
allArtifactsAreImportant
? builtArtifacts
: Sets.intersection(builtArtifacts, (Set<?>) importantArtifacts),
ctx);
if (reset != null) {
// TODO: b/321044105 - Handle error bubbling, in which case we can't rewind.
return reset;
}
}
ImmutableMap<String, ArtifactsInOutputGroup> builtOutputs =
new SuccessfulArtifactFilter(builtArtifactsBuilder.build())
new SuccessfulArtifactFilter(builtArtifacts)
.filterArtifactsInOutputGroup(artifactsToBuild.getAllArtifactsByOutputGroup());
env.getListener()
.post(completor.createFailed(key, rootCauses, ctx, builtOutputs, failureData));
Expand Down Expand Up @@ -300,19 +317,9 @@ public SkyValue compute(SkyKey skyKey, Environment env)
return null;
}

ImmutableMap<String, ActionInput> lostOutputs =
skyframeActionExecutor
.getActionContextRegistry()
.getContext(ImportantOutputHandler.class)
.processAndGetLostArtifacts(importantArtifacts, importantInputMap);
if (!lostOutputs.isEmpty()) {
return actionRewindStrategy.prepareRewindPlanForLostTopLevelOutputs(
key,
ImmutableSet.copyOf(Artifact.keys(allArtifacts)),
lostOutputs,
// TODO: b/321044105 - Compute precise owners.
new ActionInputDepOwnerMap(lostOutputs.values()),
env);
Reset reset = informImportantOutputHandler(key, env, importantArtifacts, ctx);
if (reset != null) {
return reset; // Initiate action rewinding to regenerate lost outputs.
}

ExtendedEventHandler.Postable postable =
Expand Down Expand Up @@ -358,6 +365,34 @@ Pair<ValueT, ArtifactsToBuild> getValueAndArtifactsToBuild(
return Pair.of(value, artifactsToBuild);
}

/**
* Calls {@link ImportantOutputHandler#processAndGetLostArtifacts}.
*
* <p>If any outputs are lost, returns a {@link Reset} which can be used to initiate action
* rewinding and regenerate the lost outputs. Otherwise, returns {@code null}.
*/
@Nullable
private Reset informImportantOutputHandler(
KeyT key, Environment env, Collection<Artifact> importantArtifacts, CompletionContext ctx)
throws InterruptedException {
ImmutableMap<String, ActionInput> lostOutputs =
skyframeActionExecutor
.getActionContextRegistry()
.getContext(ImportantOutputHandler.class)
.processAndGetLostArtifacts(importantArtifacts, ctx.getImportantInputMap());
if (lostOutputs.isEmpty()) {
return null;
}

return actionRewindStrategy.prepareRewindPlanForLostTopLevelOutputs(
key,
ImmutableSet.copyOf(Artifact.keys(importantArtifacts)),
lostOutputs,
// TODO: b/321044105 - Compute precise owners.
new ActionInputDepOwnerMap(lostOutputs.values()),
env);
}

@Override
public String extractTag(SkyKey skyKey) {
return Label.print(((TopLevelActionLookupKeyWrapper) skyKey).actionLookupKey().getLabel());
Expand Down
10 changes: 8 additions & 2 deletions src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@
/**
* Integration tests for action rewinding.
*
* <p>Uses {@link TestParameter}s to run tests with all four combinations of {@code
* --track_incremental_state} and {@code --keep_going}.
* <p>Uses {@link TestParameter}s to run tests with all combinations of {@code
* --track_incremental_state}, {@code --keep_going}, and {@code
* --experimental_merged_skyframe_analysis_execution}.
*/
// TODO(b/228090759): Consider asserting on graph structure to improve coverage for incrementality.
// TODO(b/228090759): Add back actionFromPreviousBuildReevaluated.
Expand Down Expand Up @@ -262,4 +263,9 @@ public void topLevelOutputRewound_regularFile() throws Exception {
public void topLevelOutputRewound_aspectOwned() throws Exception {
helper.runTopLevelOutputRewound_aspectOwned();
}

@Test
public void topLevelOutputRewound_partiallyBuiltTarget() throws Exception {
helper.runTopLevelOutputRewound_partiallyBuiltTarget();
}
}
64 changes: 64 additions & 0 deletions src/test/java/com/google/devtools/build/lib/skyframe/rewinding/RewindingTestsHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.devtools.build.lib.vfs.FileSystemUtils.readContentAsLatin1;
import static java.util.concurrent.TimeUnit.SECONDS;
import static org.junit.Assert.assertThrows;
Expand Down Expand Up @@ -2686,6 +2687,8 @@ public final void runTopLevelOutputRewound_regularFile() throws Exception {
assertThat(ImmutableMultiset.copyOf(getExecutedSpawnDescriptions()))
.hasCount("Action foo/found.out", 1);
assertThat(targetCompleteEvents.keySet()).containsExactly(fooLostAndFound);
assertOutputsReported(
targetCompleteEvents.get(fooLostAndFound), "bin/foo/lost.out", "bin/foo/found.out");
}

public final void runTopLevelOutputRewound_aspectOwned() throws Exception {
Expand Down Expand Up @@ -2719,6 +2722,54 @@ public final void runTopLevelOutputRewound_aspectOwned() throws Exception {
assertThat(ImmutableMultiset.copyOf(getExecutedSpawnDescriptions()))
.hasCount("Action foo/found.out", 1);
assertThat(aspectCompleteEvents.keySet()).containsExactly(fooLib);
assertOutputsReported(
aspectCompleteEvents.get(fooLib), "bin/foo/lost.out", "bin/foo/found.out");
}

public final void runTopLevelOutputRewound_partiallyBuiltTarget() throws Exception {
// TODO: b/321044105 - Handle and test lost outputs during error bubbling.
testCase.getRuntimeWrapper().newCommand(); // Initialize options parser.
assume().that(keepGoing()).isTrue();
testCase.write(
"foo/defs.bzl",
"def _lost_found_and_failed_impl(ctx):",
" lost = ctx.actions.declare_file('lost.out')",
" found = ctx.actions.declare_file('found.out')",
" failed = ctx.actions.declare_file('failed.out')",
" ctx.actions.run_shell(",
" outputs = [lost, found],",
" command = 'echo lost > %s && echo found > %s' % (lost.path, found.path),",
" )",
" ctx.actions.run_shell(outputs = [failed], inputs = [found], command = 'false')",
" return DefaultInfo(files = depset([lost, found, failed]))",
"",
"lost_found_and_failed = rule(implementation = _lost_found_and_failed_impl)");
testCase.write(
"foo/BUILD",
"load(':defs.bzl', 'lost_found_and_failed')",
"lost_found_and_failed(name = 'lost_found_and_failed')");
lostOutputsModule.addLostOutput(getExecPath("bin/foo/lost.out"));
Label fooLostFoundAndFailed = Label.parseCanonical("//foo:lost_found_and_failed");
List<SkyKey> rewoundKeys = collectOrderedRewoundKeys();
Map<Label, TargetCompleteEvent> targetCompleteEvents = recordTargetCompleteEvents();
listenForNoCompletionEventsBeforeRewinding(fooLostFoundAndFailed, targetCompleteEvents);

assertThrows(
BuildFailedException.class, () -> testCase.buildTarget("//foo:lost_found_and_failed"));

lostOutputsModule.verifyAllLostOutputsConsumed();
assertThat(rewoundKeys).hasSize(1);
assertThat(rewoundArtifactOwnerLabels(rewoundKeys))
.containsExactly("//foo:lost_found_and_failed");
assertThat(ImmutableMultiset.copyOf(getExecutedSpawnDescriptions()))
.hasCount("Action foo/lost.out", 2);
assertThat(ImmutableMultiset.copyOf(getExecutedSpawnDescriptions()))
.hasCount("Action foo/failed.out", 1);

// The event is failed but still reports the built artifacts, including the one that was lost.
TargetCompleteEvent event = targetCompleteEvents.get(fooLostFoundAndFailed);
assertThat(event.failed()).isTrue();
assertOutputsReported(event, "bin/foo/lost.out", "bin/foo/found.out");
}

private void listenForNoCompletionEventsBeforeRewinding(
Expand All @@ -2734,6 +2785,19 @@ private void listenForNoCompletionEventsBeforeRewinding(
});
}

private void assertOutputsReported(
EventReportingArtifacts event, String... expectedRootRelativePaths) throws Exception {
List<String> expectedExecPaths = new ArrayList<>();
for (String path : expectedRootRelativePaths) {
expectedExecPaths.add(getExecPath(path));
}
assertThat(
event.reportedArtifacts().artifacts.stream()
.flatMap(set -> set.toList().stream())
.map(Artifact::getExecPathString))
.containsExactlyElementsIn(expectedExecPaths);
}

static boolean isActionExecutionKey(Object key, Label label) {
return key instanceof ActionLookupData && label.equals(((ActionLookupData) key).getLabel());
}
Expand Down

0 comments on commit 3bdeaea

Please sign in to comment.