diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index d8bdedc6a92cf5..c53a6b7022bc48 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -28,8 +28,8 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.common.collect.Multimap; import com.google.common.collect.MultimapBuilder; +import com.google.common.collect.SetMultimap; import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionCacheChecker.Token; @@ -248,11 +248,6 @@ private SkyValue computeInternal(SkyKey skyKey, Environment env) // Missing deps. return null; } - } else if (state.allInputs.packageLookupsRequested != null) { - // Preserve the invariant that we ask for the same deps each build. Because we know these deps - // were already retrieved successfully, we don't request them with exceptions. - env.getValues(state.allInputs.packageLookupsRequested); - Preconditions.checkState(!env.valuesMissing(), "%s %s", action, state); } CheckInputResults checkedInputs = null; @@ -262,22 +257,22 @@ private SkyValue computeInternal(SkyKey skyKey, Environment env) .actionFileSystemType() .equals(ActionFileSystemType.STAGE_REMOTE_FILES)); - Iterable depKeys = getInputDepKeys(allInputs, state); - // We do this unconditionally to maintain our invariant of asking for the same deps each build. - List< - ValueOrException3< - SourceArtifactException, ActionExecutionException, ArtifactNestedSetEvalException>> - inputDeps = - env.getOrderedValuesOrThrow( - depKeys, - SourceArtifactException.class, - ActionExecutionException.class, - ArtifactNestedSetEvalException.class); - try { if (previousExecution == null && !state.hasArtifactData()) { // Do we actually need to find our metadata? - checkedInputs = checkInputs(env, action, inputDeps, allInputs, depKeys, actionLookupData); + Iterable depKeys = getInputDepKeys(allInputs, state); + checkedInputs = + checkInputs( + env, + action, + env.getOrderedValuesOrThrow( + depKeys, + SourceArtifactException.class, + ActionExecutionException.class, + ArtifactNestedSetEvalException.class), + allInputs, + depKeys, + actionLookupData); } } catch (ActionExecutionException e) { throw new ActionExecutionFunctionException(e); @@ -339,7 +334,14 @@ private SkyValue computeInternal(SkyKey skyKey, Environment env) actionStartTime); } catch (LostInputsActionExecutionException e) { return handleLostInputs( - e, actionLookupData, action, actionStartTime, env, inputDeps, allInputs, depKeys, state); + e, + actionLookupData, + action, + actionStartTime, + env, + allInputs, + getInputDepKeys(allInputs, state), + state); } catch (ActionExecutionException e) { // In this case we do not report the error to the action reporter because we have already // done it in SkyframeActionExecutor.reportErrorIfNotAbortingMode() method. That method @@ -418,12 +420,6 @@ private SkyFunction.Restart handleLostInputs( Action action, long actionStartTime, Environment env, - List< - ValueOrException3< - SourceArtifactException, - ActionExecutionException, - ArtifactNestedSetEvalException>> - inputDeps, NestedSet allInputs, Iterable depKeys, InputDiscoveryState state) @@ -436,8 +432,7 @@ private SkyFunction.Restart handleLostInputs( RewindPlan rewindPlan = null; try { ActionInputDepOwners inputDepOwners = - createAugmentedInputDepOwners( - e, action, env, inputDeps, allInputs, depKeys, actionLookupData); + createAugmentedInputDepOwners(e, action, env, allInputs, actionLookupData); // Collect the set of direct deps of this action which may be responsible for the lost inputs, // some of which may be discovered. @@ -516,14 +511,7 @@ private ActionInputDepOwners createAugmentedInputDepOwners( LostInputsActionExecutionException e, Action action, Environment env, - List< - ValueOrException3< - SourceArtifactException, - ActionExecutionException, - ArtifactNestedSetEvalException>> - inputDeps, NestedSet allInputs, - Iterable requestedSkyKeys, ActionLookupData actionLookupDataForError) throws InterruptedException { @@ -540,9 +528,7 @@ private ActionInputDepOwners createAugmentedInputDepOwners( getInputDepOwners( env, action, - inputDeps, allInputs, - requestedSkyKeys, lostInputsAndOwnersSoFar, actionLookupDataForError); } catch (ActionExecutionException unexpected) { @@ -1099,14 +1085,7 @@ private CheckInputResults checkInputs( private ActionInputDepOwnerMap getInputDepOwners( Environment env, Action action, - List< - ValueOrException3< - SourceArtifactException, - ActionExecutionException, - ArtifactNestedSetEvalException>> - inputDeps, NestedSet allInputs, - Iterable requestedSkyKeys, Collection lostInputs, ActionLookupData actionLookupDataForError) throws ActionExecutionException, InterruptedException { @@ -1117,9 +1096,9 @@ private ActionInputDepOwnerMap getInputDepOwners( return accumulateInputs( env, action, - inputDeps, + /*inputDeps=*/ null, allInputs, - requestedSkyKeys, + /*requestedSkyKeys=*/ null, ignoredInputDepsSize -> new ActionInputDepOwnerMap(lostInputs), (actionInputMapSink, expandedArtifacts, @@ -1156,48 +1135,56 @@ public boolean test(Artifact input) { /** * May return {@code null} if {@code allowValuesMissingEarlyReturn} and {@code * env.valuesMissing()} are true and no inputs result in {@link ActionExecutionException}s. + * + *

If {@code inputDeps} (and therefore {@code requestedSkyKeys}) is null, assumes that deps + * have already been checked for exceptions and inserted into the {@link + * ArtifactNestedSetFunction} cache, so those steps are skipped. (This codepath currently only + * used for action rewinding.) */ @Nullable private R accumulateInputs( Environment env, Action action, - List< - ValueOrException3< - SourceArtifactException, - ActionExecutionException, - ArtifactNestedSetEvalException>> - inputDeps, + @Nullable + List< + ValueOrException3< + SourceArtifactException, + ActionExecutionException, + ArtifactNestedSetEvalException>> + inputDeps, NestedSet allInputs, - Iterable requestedSkyKeys, + @Nullable Iterable requestedSkyKeys, IntFunction actionInputMapSinkFactory, AccumulateInputResultsFactory accumulateInputResultsFactory, boolean allowValuesMissingEarlyReturn, ActionLookupData actionLookupDataForError) throws ActionExecutionException, InterruptedException { Predicate isMandatoryInput = makeMandatoryInputPredicate(action); - ActionExecutionFunctionExceptionHandler actionExecutionFunctionExceptionHandler = - new ActionExecutionFunctionExceptionHandler( - Suppliers.memoize( - () -> { - ImmutableList allInputsList = allInputs.toList(); - Multimap skyKeyToArtifactSet = - MultimapBuilder.hashKeys().hashSetValues().build(); - allInputsList.forEach( - input -> { - SkyKey key = Artifact.key(input); - if (key != input) { - skyKeyToArtifactSet.put(key, input); - } - }); - return skyKeyToArtifactSet; - }), - inputDeps, - action, - isMandatoryInput, - requestedSkyKeys, - env.valuesMissing()); - - actionExecutionFunctionExceptionHandler.accumulateAndMaybeThrowExceptions(); + ActionExecutionFunctionExceptionHandler actionExecutionFunctionExceptionHandler = null; + if (inputDeps != null) { + actionExecutionFunctionExceptionHandler = + new ActionExecutionFunctionExceptionHandler( + Suppliers.memoize( + () -> { + ImmutableList allInputsList = allInputs.toList(); + SetMultimap skyKeyToArtifactSet = + MultimapBuilder.hashKeys().hashSetValues().build(); + allInputsList.forEach( + input -> { + SkyKey key = Artifact.key(input); + if (key != input) { + skyKeyToArtifactSet.put(key, input); + } + }); + return skyKeyToArtifactSet; + }), + inputDeps, + action, + isMandatoryInput, + requestedSkyKeys, + env.valuesMissing()); + actionExecutionFunctionExceptionHandler.accumulateAndMaybeThrowExceptions(); + } if (env.valuesMissing() && allowValuesMissingEarlyReturn) { return null; @@ -1264,8 +1251,13 @@ private R accumulateInputs( } if (value instanceof MissingArtifactValue) { if (isMandatoryInput.test(input)) { - actionExecutionFunctionExceptionHandler.accumulateMissingFileArtifactValue( - input, (MissingArtifactValue) value); + checkNotNull( + actionExecutionFunctionExceptionHandler, + "Missing artifact should have been caught already %s %s %s", + input, + value, + action) + .accumulateMissingFileArtifactValue(input, (MissingArtifactValue) value); continue; } else { value = FileArtifactValue.MISSING_FILE_MARKER; @@ -1281,9 +1273,12 @@ private R accumulateInputs( value, env); } - // After accumulating the inputs, we might find some mandatory artifact with - // SourceFileInErrorArtifactValue. - actionExecutionFunctionExceptionHandler.maybeThrowException(); + + if (actionExecutionFunctionExceptionHandler != null) { + // After accumulating the inputs, we might find some mandatory artifact with + // SourceFileInErrorArtifactValue. + actionExecutionFunctionExceptionHandler.maybeThrowException(); + } return accumulateInputResultsFactory.create( inputArtifactData, @@ -1473,7 +1468,7 @@ public boolean isCatastrophic() { /** Helper subclass for the error-handling logic for ActionExecutionFunction#accumulateInputs. */ private final class ActionExecutionFunctionExceptionHandler { - private final Supplier> skyKeyToDerivedArtifactSetForExceptions; + private final Supplier> skyKeyToDerivedArtifactSetForExceptions; private final List< ValueOrException3< SourceArtifactException, ActionExecutionException, ArtifactNestedSetEvalException>> @@ -1487,7 +1482,7 @@ private final class ActionExecutionFunctionExceptionHandler { private ActionExecutionException firstActionExecutionException; ActionExecutionFunctionExceptionHandler( - Supplier> skyKeyToDerivedArtifactSetForExceptions, + Supplier> skyKeyToDerivedArtifactSetForExceptions, List< ValueOrException3< SourceArtifactException,