diff options
Diffstat (limited to 'src')
6 files changed, 68 insertions, 63 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java index 09457474bd..324d0b4608 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java @@ -36,6 +36,8 @@ import com.google.devtools.build.lib.vfs.Symlinks; import java.io.IOException; import java.util.Collection; +import javax.annotation.Nullable; + /** * Abstract implementation of Action which implements basic functionality: the * inputs, outputs, and toString method. Both input and output sets are @@ -98,13 +100,20 @@ public abstract class AbstractAction implements Action { + " since it does not discover inputs"); } + @Nullable @Override - public boolean updateInputsFromCache(ArtifactResolver artifactResolver, + public Iterable<Artifact> resolveInputsFromCache(ArtifactResolver artifactResolver, PackageRootResolver resolver, Collection<PathFragment> inputPaths) { throw new IllegalStateException( "Method must be overridden for actions that may have unknown inputs."); } + @Override + public void updateInputs(Iterable<Artifact> inputs) { + throw new IllegalStateException( + "Method must be overridden for actions that may have unknown inputs."); + } + /** * Should only be overridden by actions that need to optionally insert inputs. Actions that * discover their inputs should use {@link #setInputs} to set the new iterable of inputs when they @@ -119,8 +128,8 @@ public abstract class AbstractAction implements Action { * Set the inputs of the action. May only be used by an action that {@link #discoversInputs()}. * The iterable passed in is automatically made immutable. */ - public void setInputs(Iterable<Artifact> inputs) { - Preconditions.checkState(discoversInputs()); + protected void setInputs(Iterable<Artifact> inputs) { + Preconditions.checkState(discoversInputs(), this); this.inputs = CollectionUtils.makeImmutable(inputs); cachedInputCount = -1; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Action.java b/src/main/java/com/google/devtools/build/lib/actions/Action.java index 09c50fb1fc..3581ae0a31 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Action.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Action.java @@ -107,26 +107,33 @@ public interface Action extends ActionMetadata, Describable { throws ActionExecutionException, InterruptedException; /** - * Method used to update action inputs based on the information contained in + * Method used to resolve action inputs based on the information contained in * the action cache. It will be called iff inputsKnown() is false for the * given action instance and there is a related cache entry in the action * cache. * * Method must be redefined for any action that may return - * inputsKnown() == false. It also expects that implementation will ensure - * that inputsKnown() returns true after call to this method. + * inputsKnown() == false. * * @param artifactResolver the artifact factory that can be used to manufacture artifacts - * @param inputPaths List of relative (to the execution root) input paths * @param resolver object which helps to resolve some of the artifacts - * @return false if some dependencies are missing and we need to update again later, - * otherwise true. + * @param inputPaths List of relative (to the execution root) input paths + * @return List of Artifacts corresponding to inputPaths, or null if some dependencies were + * missing and we need to try again later. */ - boolean updateInputsFromCache( + @Nullable + Iterable<Artifact> resolveInputsFromCache( ArtifactResolver artifactResolver, PackageRootResolver resolver, Collection<PathFragment> inputPaths); /** + * Informs the action that its inputs are {@code inputs}, and that its inputs are now known. Can + * only be called for actions that discover inputs. After this method is called, + * {@link ActionMetadata#inputsKnown} should return true. + */ + void updateInputs(Iterable<Artifact> inputs); + + /** * Return a best-guess estimate of the operation's resource consumption on the * local host itself for use in scheduling. * diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index f40003b1a5..cda9cb99ad 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -18,6 +18,7 @@ import com.google.common.base.Predicate; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Action.MiddlemanType; import com.google.devtools.build.lib.actions.cache.ActionCache; +import com.google.devtools.build.lib.actions.cache.ActionCache.Entry; import com.google.devtools.build.lib.actions.cache.Digest; import com.google.devtools.build.lib.actions.cache.Metadata; import com.google.devtools.build.lib.actions.cache.MetadataHandler; @@ -85,17 +86,19 @@ public class ActionCacheChecker { * * @param entry cached action information. * @param action action to be validated. + * @param actionInputs the inputs of the action. Normally just the result of action.getInputs(), + * but if this action doesn't yet know its inputs, we check the inputs from the cache. * @param metadataHandler provider of metadata for the artifacts this action interacts with. * @param checkOutput true to validate output artifacts, Otherwise, just * validate inputs. * * @return true if at least one artifact has changed, false - otherwise. */ - private boolean validateArtifacts(ActionCache.Entry entry, Action action, - MetadataHandler metadataHandler, boolean checkOutput) { + private boolean validateArtifacts(Entry entry, Action action, + Iterable<Artifact> actionInputs, MetadataHandler metadataHandler, boolean checkOutput) { Iterable<Artifact> artifacts = checkOutput - ? Iterables.concat(action.getOutputs(), action.getInputs()) - : action.getInputs(); + ? Iterables.concat(action.getOutputs(), actionInputs) + : actionInputs; Map<String, Metadata> mdMap = new HashMap<>(); for (Artifact artifact : artifacts) { mdMap.put(artifact.getExecPathString(), metadataHandler.getMetadataMaybe(artifact)); @@ -150,41 +153,36 @@ public class ActionCacheChecker { } return null; } - ActionCache.Entry entry = null; // Populated lazily. - - // Update action inputs from cache, if necessary. + Iterable<Artifact> actionInputs = action.getInputs(); + ActionCache.Entry entry = getCacheEntry(action); + // Resolve action inputs from cache, if necessary. boolean inputsKnown = action.inputsKnown(); - if (!inputsKnown) { - Preconditions.checkState(action.discoversInputs()); - entry = getCacheEntry(action); - boolean ranSuccessfully = updateActionInputs(action, entry, resolver); - // If during update of inputs skyframe was missing some dependencies (for example, - // ContainingPackageLookupValue inside of ArtifactFactory.resolveSourceArtifact), we need to - // wait for those dependencies to be resolved. So next time when we will call corresponding - // ActionExecutionFunction all those dependencies will have been resolved and we can continue - // action execution process. - if (!ranSuccessfully) { + if (entry != null && !entry.isCorrupted() && !inputsKnown) { + Preconditions.checkState(action.discoversInputs(), action); + actionInputs = resolveCachedActionInputs(action, entry, resolver); + if (actionInputs == null) { + // Not all inputs could be resolved. Try again next time. return Token.NEED_TO_RERUN; } } - if (mustExecute(action, entry, handler, metadataHandler)) { + if (mustExecute(action, entry, handler, metadataHandler, actionInputs)) { return new Token(getKeyString(action)); } + + if (!inputsKnown) { + action.updateInputs(actionInputs); + } return null; } protected boolean mustExecute(Action action, @Nullable ActionCache.Entry entry, - EventHandler handler, MetadataHandler metadataHandler) { + EventHandler handler, MetadataHandler metadataHandler, Iterable<Artifact> actionInputs) { // Unconditional execution can be applied only for actions that are allowed to be executed. if (unconditionalExecution(action)) { Preconditions.checkState(action.isVolatile()); reportUnconditionalExecution(handler, action); return true; // must execute - unconditional execution is requested. } - - if (entry == null) { - entry = getCacheEntry(action); - } if (entry == null) { reportNewAction(handler, action); return true; // must execute -- no cache entry (e.g. first build) @@ -193,7 +191,7 @@ public class ActionCacheChecker { if (entry.isCorrupted()) { reportCorruptedCacheEntry(handler, action); return true; // cache entry is corrupted - must execute - } else if (validateArtifacts(entry, action, metadataHandler, true)) { + } else if (validateArtifacts(entry, action, actionInputs, metadataHandler, true)) { reportChanged(handler, action); return true; // files have changed } else if (!entry.getActionKey().equals(action.getKey())){ @@ -230,11 +228,10 @@ public class ActionCacheChecker { actionCache.put(key, entry); } - protected boolean updateActionInputs(Action action, ActionCache.Entry entry, + private Iterable<Artifact> resolveCachedActionInputs(Action action, ActionCache.Entry entry, PackageRootResolver resolver) { - if (entry == null || entry.isCorrupted()) { - return true; - } + Preconditions.checkNotNull(entry, action); + Preconditions.checkState(!entry.isCorrupted(), action); List<PathFragment> outputs = new ArrayList<>(); for (Artifact output : action.getOutputs()) { @@ -249,7 +246,7 @@ public class ActionCacheChecker { inputs.add(execPath); } } - return action.updateInputsFromCache(artifactResolver, resolver, inputs); + return action.resolveInputsFromCache(artifactResolver, resolver, inputs); } /** @@ -273,7 +270,7 @@ public class ActionCacheChecker { if (entry.isCorrupted()) { reportCorruptedCacheEntry(handler, action); changed = true; - } else if (validateArtifacts(entry, action, metadataHandler, false)) { + } else if (validateArtifacts(entry, action, action.getInputs(), metadataHandler, false)) { reportChanged(handler, action); changed = true; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 5d2857a995..1d49d676f3 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -836,7 +836,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable } @Override - public boolean updateInputsFromCache( + public Iterable<Artifact> resolveInputsFromCache( ArtifactResolver artifactResolver, PackageRootResolver resolver, Collection<PathFragment> inputPaths) { // Note that this method may trigger a violation of the desirable invariant that getInputs() @@ -859,7 +859,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable artifactResolver.resolveSourceArtifacts(unresolvedPaths, resolver); if (resolvedArtifacts == null) { // We are missing some dependencies. We need to rerun this update later. - return false; + return null; } for (PathFragment execPath : unresolvedPaths) { @@ -873,11 +873,15 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable inputs.add(artifact); } } + return inputs; + } + + @Override + public synchronized void updateInputs(Iterable<Artifact> inputs) { inputsKnown = true; synchronized (this) { setInputs(inputs); } - return true; } private Map<PathFragment, Artifact> getAllowedDerivedInputsMap() { diff --git a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java index a1f82a9785..343b44302c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java @@ -152,28 +152,26 @@ public final class ExtraAction extends SpawnAction { return result.addAll(extraActionInputs).build(); } - private void updateInputs(Iterable<Artifact> shadowedActionInputs) { + @Override + public void updateInputs(Iterable<Artifact> shadowedActionInputs) { + shadowedAction.updateInputs(shadowedActionInputs); + Preconditions.checkArgument(shadowedAction.inputsKnown(), "%s %s", this, shadowedAction); synchronized (this) { setInputs(createInputs(shadowedActionInputs, extraActionInputs)); inputsKnown = true; } } + @Nullable @Override - public boolean updateInputsFromCache(ArtifactResolver artifactResolver, + public Iterable<Artifact> resolveInputsFromCache(ArtifactResolver artifactResolver, PackageRootResolver resolver, Collection<PathFragment> inputPaths) { // We update the inputs directly from the shadowed action. Set<PathFragment> extraActionPathFragments = ImmutableSet.copyOf(Artifact.asPathFragments(extraActionInputs)); - boolean noMissingDependencies = shadowedAction.updateInputsFromCache(artifactResolver, resolver, + return shadowedAction.resolveInputsFromCache(artifactResolver, + resolver, Collections2.filter(inputPaths, Predicates.in(extraActionPathFragments))); - if (!noMissingDependencies) { - // This update needs to be rerun. - return false; - } - Preconditions.checkState(shadowedAction.inputsKnown(), "%s %s", this, shadowedAction); - updateInputs(shadowedAction.getInputs()); - return true; } /** 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 812d1fa89b..2958c6e3c7 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 @@ -254,16 +254,6 @@ public class ActionExecutionFunction implements SkyFunction { for (Artifact artifact : inputs) { discoveredArtifacts.add(ArtifactValue.key(artifact, mandatory.contains(artifact))); } - - // In case the action violates the invariant that getInputs() is a superset of - // getMandatoryInputs(), explicitly add the mandatory inputs. See bug about an - // "action not in canonical form" error message. Also note that we may add Skyframe edges on - // these potentially stale deps due to the way loading inputs from the action cache functions. - // In practice, this is safe since C++ actions (the only ones which discover inputs) only add - // possibly stale inputs on source artifacts, which we treat as non-mandatory. - for (Artifact artifact : mandatory) { - discoveredArtifacts.add(ArtifactValue.key(artifact, true)); - } return discoveredArtifacts; } } |