Simplify the Action interface by asking it a set of allowed inputs instead of to resolve exec paths found in the action cache.
The resolution algorithm was the same in all cases where it was implemented.
--
PiperOrigin-RevId: 146344672
MOS_MIGRATED_REVID=146344672
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 4acc46a..7919529 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
@@ -39,14 +39,12 @@
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
-import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.skyframe.SkyFunction;
import java.io.IOException;
import java.util.Collection;
import java.util.Map;
import java.util.Map.Entry;
-import javax.annotation.Nullable;
/**
* Abstract implementation of Action which implements basic functionality: the inputs, outputs, and
@@ -188,13 +186,8 @@
return null;
}
- @Nullable
@Override
- public Iterable<Artifact> resolveInputsFromCache(
- ArtifactResolver artifactResolver,
- PackageRootResolver resolver,
- Collection<PathFragment> inputPaths)
- throws PackageRootResolutionException, InterruptedException {
+ public Iterable<Artifact> getAllowedDerivedInputs() {
throw new IllegalStateException(
"Method must be overridden for actions that may have unknown inputs.");
}
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 6985fa2..bee4774 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
@@ -18,10 +18,8 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ConditionallyThreadCompatible;
import com.google.devtools.build.lib.profiler.Describable;
import com.google.devtools.build.lib.vfs.Path;
-import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunction;
import java.io.IOException;
-import java.util.Collection;
import javax.annotation.Nullable;
/**
@@ -165,25 +163,15 @@
throws ActionExecutionException, InterruptedException;
/**
- * 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.
+ * Returns the set of artifacts that can possibly be inputs. It will be called iff inputsKnown()
+ * is false for the given action instance and there is a related cache entry in the action cache.
*
* <p>Method must be redefined for any action that may return inputsKnown() == false.
*
- * @param artifactResolver the artifact factory that can be used to manufacture artifacts
- * @param resolver object which helps to resolve some of the artifacts
- * @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.
- * @throws PackageRootResolutionException on failure to determine package roots of inputPaths
+ * <p>The method is allowed to return source artifacts. They are useless, though, since exec paths
+ * in the action cache referring to source artifacts are always resolved.
*/
- @Nullable
- Iterable<Artifact> resolveInputsFromCache(
- ArtifactResolver artifactResolver,
- PackageRootResolver resolver,
- Collection<PathFragment> inputPaths)
- throws PackageRootResolutionException, InterruptedException;
+ Iterable<Artifact> getAllowedDerivedInputs();
/**
* Informs the action that its inputs are {@code inputs}, and that its inputs are now known. Can
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 b97d323..4f2a33c 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
@@ -300,16 +300,57 @@
for (Artifact output : action.getOutputs()) {
outputs.add(output.getExecPath());
}
- List<PathFragment> inputs = new ArrayList<>();
+ List<PathFragment> inputExecPaths = new ArrayList<>();
for (String path : entry.getPaths()) {
PathFragment execPath = new PathFragment(path);
// Code assumes that action has only 1-2 outputs and ArrayList.contains() will be
// most efficient.
if (!outputs.contains(execPath)) {
- inputs.add(execPath);
+ inputExecPaths.add(execPath);
}
}
- return action.resolveInputsFromCache(artifactResolver, resolver, inputs);
+
+ // Note that this method may trigger a violation of the desirable invariant that getInputs()
+ // is a superset of getMandatoryInputs(). See bug about an "action not in canonical form"
+ // error message and the integration test test_crosstool_change_and_failure().
+ Map<PathFragment, Artifact> allowedDerivedInputsMap = new HashMap<>();
+ for (Artifact derivedInput : action.getAllowedDerivedInputs()) {
+ if (!derivedInput.isSourceArtifact()) {
+ allowedDerivedInputsMap.put(derivedInput.getExecPath(), derivedInput);
+ }
+ }
+
+ List<Artifact> inputArtifacts = new ArrayList<>();
+ List<PathFragment> unresolvedPaths = new ArrayList<>();
+ for (PathFragment execPath : inputExecPaths) {
+ Artifact artifact = allowedDerivedInputsMap.get(execPath);
+ if (artifact != null) {
+ inputArtifacts.add(artifact);
+ } else {
+ // Remember this execPath, we will try to resolve it as a source artifact.
+ unresolvedPaths.add(execPath);
+ }
+ }
+
+ Map<PathFragment, Artifact> resolvedArtifacts =
+ artifactResolver.resolveSourceArtifacts(unresolvedPaths, resolver);
+ if (resolvedArtifacts == null) {
+ // We are missing some dependencies. We need to rerun this update later.
+ return null;
+ }
+
+ for (PathFragment execPath : unresolvedPaths) {
+ Artifact artifact = resolvedArtifacts.get(execPath);
+ // If PathFragment cannot be resolved into the artifact, ignore it. This could happen if the
+ // rule has changed and the action no longer depends on, e.g., an additional source file in a
+ // separate package and that package is no longer referenced anywhere else. It is safe to
+ // ignore such paths because dependency checker would identify changes in inputs (ignored path
+ // was used before) and will force action execution.
+ if (artifact != null) {
+ inputArtifacts.add(artifact);
+ }
+ }
+ return inputArtifacts;
}
/**
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 b1e87d3..6465f6c 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
@@ -34,8 +34,6 @@
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionInfoSpecifier;
import com.google.devtools.build.lib.actions.Executor;
-import com.google.devtools.build.lib.actions.PackageRootResolutionException;
-import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.extra.CppCompileInfo;
import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
@@ -1081,50 +1079,6 @@
return variableBuilder.build();
}
- // Keep in sync with {@link ObjcCompileAction#resolveInputsFromCache}
- @Override
- public Iterable<Artifact> resolveInputsFromCache(
- ArtifactResolver artifactResolver,
- PackageRootResolver resolver,
- Collection<PathFragment> inputPaths)
- throws PackageRootResolutionException, InterruptedException {
- // Note that this method may trigger a violation of the desirable invariant that getInputs()
- // is a superset of getMandatoryInputs(). See bug about an "action not in canonical form"
- // error message and the integration test test_crosstool_change_and_failure().
- Map<PathFragment, Artifact> allowedDerivedInputsMap = getAllowedDerivedInputsMap();
- List<Artifact> inputs = new ArrayList<>();
- List<PathFragment> unresolvedPaths = new ArrayList<>();
- for (PathFragment execPath : inputPaths) {
- Artifact artifact = allowedDerivedInputsMap.get(execPath);
- if (artifact != null) {
- inputs.add(artifact);
- } else {
- // Remember this execPath, we will try to resolve it as a source artifact.
- unresolvedPaths.add(execPath);
- }
- }
-
- Map<PathFragment, Artifact> resolvedArtifacts =
- artifactResolver.resolveSourceArtifacts(unresolvedPaths, resolver);
- if (resolvedArtifacts == null) {
- // We are missing some dependencies. We need to rerun this update later.
- return null;
- }
-
- for (PathFragment execPath : unresolvedPaths) {
- Artifact artifact = resolvedArtifacts.get(execPath);
- // If PathFragment cannot be resolved into the artifact - ignore it. This could happen if
- // rule definition has changed and action no longer depends on, e.g., additional source file
- // in the separate package and that package is no longer referenced anywhere else.
- // It is safe to ignore such paths because dependency checker would identify change in inputs
- // (ignored path was used before) and will force action execution.
- if (artifact != null) {
- inputs.add(artifact);
- }
- }
- return inputs;
- }
-
@Override protected void setInputs(Iterable<Artifact> inputs) {
super.setInputs(inputs);
}
@@ -1137,6 +1091,11 @@
}
}
+ @Override
+ public Iterable<Artifact> getAllowedDerivedInputs() {
+ return getAllowedDerivedInputsMap().values();
+ }
+
protected Map<PathFragment, Artifact> getAllowedDerivedInputsMap() {
Map<PathFragment, Artifact> allowedDerivedInputMap = new HashMap<>();
addToMap(allowedDerivedInputMap, mandatoryInputs);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LTOBackendAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LTOBackendAction.java
index 9e7b856..0b71041 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LTOBackendAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LTOBackendAction.java
@@ -22,9 +22,6 @@
import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.ArtifactResolver;
-import com.google.devtools.build.lib.actions.PackageRootResolutionException;
-import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.analysis.actions.CommandLine;
@@ -175,15 +172,9 @@
inputsKnown = true;
}
- @Nullable
@Override
- public Iterable<Artifact> resolveInputsFromCache(
- ArtifactResolver artifactResolver,
- PackageRootResolver resolver,
- Collection<PathFragment> inputPaths)
- throws PackageRootResolutionException {
- Set<Artifact> bitcodeInputSet = computeBitcodeInputs(inputPaths);
- return createInputs(bitcodeInputSet, getMandatoryInputs());
+ public Iterable<Artifact> getAllowedDerivedInputs() {
+ return bitcodeFiles.values();
}
@Override
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 f9e4616..866e45e 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
@@ -15,8 +15,6 @@
package com.google.devtools.build.lib.rules.extra;
import com.google.common.base.Function;
-import com.google.common.base.Predicates;
-import com.google.common.collect.Collections2;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -25,11 +23,8 @@
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.ArtifactResolver;
import com.google.devtools.build.lib.actions.CompositeRunfilesSupplier;
import com.google.devtools.build.lib.actions.DelegateSpawn;
-import com.google.devtools.build.lib.actions.PackageRootResolutionException;
-import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnActionContext;
@@ -40,7 +35,6 @@
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
-import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.util.Collection;
import java.util.Map;
@@ -159,18 +153,9 @@
inputsKnown = true;
}
- @Nullable
@Override
- public Iterable<Artifact> resolveInputsFromCache(
- ArtifactResolver artifactResolver,
- PackageRootResolver resolver,
- Collection<PathFragment> inputPaths)
- throws PackageRootResolutionException, InterruptedException {
- // We update the inputs directly from the shadowed action.
- Set<PathFragment> extraActionPathFragments =
- ImmutableSet.copyOf(Artifact.asPathFragments(extraActionInputs));
- return shadowedAction.resolveInputsFromCache(artifactResolver, resolver,
- Collections2.filter(inputPaths, Predicates.in(extraActionPathFragments)));
+ public Iterable<Artifact> getAllowedDerivedInputs() {
+ return shadowedAction.getAllowedDerivedInputs();
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
index 49a741a..70509aa 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
@@ -28,8 +28,6 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactResolver;
import com.google.devtools.build.lib.actions.Executor;
-import com.google.devtools.build.lib.actions.PackageRootResolutionException;
-import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.actions.Spawn;
@@ -52,10 +50,7 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Collection;
import java.util.HashMap;
-import java.util.List;
import java.util.Map;
import javax.annotation.concurrent.GuardedBy;
@@ -181,49 +176,6 @@
return filterHeaderFiles();
}
- // Keep in sync with {@link CppCompileAction#resolveInputsFromCache}
- @Override
- public Iterable<Artifact> resolveInputsFromCache(
- ArtifactResolver artifactResolver,
- PackageRootResolver resolver,
- Collection<PathFragment> inputPaths)
- throws PackageRootResolutionException, InterruptedException {
- // Note that this method may trigger a violation of the desirable invariant that getInputs()
- // is a superset of getMandatoryInputs().
- Map<PathFragment, Artifact> allowedDerivedInputsMap = getAllowedDerivedInputsMap();
- List<Artifact> inputs = new ArrayList<>();
- List<PathFragment> unresolvedPaths = new ArrayList<>();
- for (PathFragment execPath : inputPaths) {
- Artifact artifact = allowedDerivedInputsMap.get(execPath);
- if (artifact != null) {
- inputs.add(artifact);
- } else {
- // Remember this execPath, we will try to resolve it as a source artifact.
- unresolvedPaths.add(execPath);
- }
- }
-
- Map<PathFragment, Artifact> resolvedArtifacts =
- artifactResolver.resolveSourceArtifacts(unresolvedPaths, resolver);
- if (resolvedArtifacts == null) {
- // We are missing some dependencies. We need to rerun this update later.
- return null;
- }
-
- for (PathFragment execPath : unresolvedPaths) {
- Artifact artifact = resolvedArtifacts.get(execPath);
- // If PathFragment cannot be resolved into the artifact - ignore it. This could happen if
- // rule definition has changed and action no longer depends on, e.g., additional source file
- // in the separate package and that package is no longer referenced anywhere else.
- // It is safe to ignore such paths because dependency checker would identify change in inputs
- // (ignored path was used before) and will force action execution.
- if (artifact != null) {
- inputs.add(artifact);
- }
- }
- return inputs;
- }
-
@Override
public synchronized void updateInputs(Iterable<Artifact> inputs) {
inputsKnown = true;
@@ -289,6 +241,11 @@
}
}
+ @Override
+ public Iterable<Artifact> getAllowedDerivedInputs() {
+ return getAllowedDerivedInputsMap().values();
+ }
+
private Map<PathFragment, Artifact> getAllowedDerivedInputsMap() {
Map<PathFragment, Artifact> allowedDerivedInputMap = new HashMap<>();
addToMapIfSource(allowedDerivedInputMap, getInputs());
