Simplify action input discovery by removing Action#getInputsWhenSkippingInputDiscovery().

This has the side effect of always allowing new inputs to be discovered when Action#discoversInputs() return true, but since those actions are very few and have mostly to do with C++, I think it's an acceptable tradeoff for th ecode being that much simpler.

--
PiperOrigin-RevId: 146098150
MOS_MIGRATED_REVID=146098150

6 files changed, 49 insertions, 145 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 a0dda9e7a1..4acc46a652 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
@@ -190,12 +190,6 @@ public abstract class AbstractAction implements Action, SkylarkValue {
 
   @Nullable
   @Override
-  public Iterable<Artifact> getInputsWhenSkippingInputDiscovery() {
-    return null;
-  }
-
-  @Nullable
-  @Override
   public Iterable<Artifact> resolveInputsFromCache(
       ArtifactResolver artifactResolver,
       PackageRootResolver resolver,
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 12316b7264..6985fa2a1d 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
@@ -165,32 +165,6 @@ public interface Action extends ActionExecutionMetadata, Describable {
       throws ActionExecutionException, InterruptedException;
 
   /**
-   * If an action does not know the exact set of inputs ahead of time, it will usually either do:
-   * <ul>
-   * <li> Execution time pruning: The action provides a superset set of inputs at action creation
-   *      time, and prunes inputs during {@link #execute}.
-   * <li> Input discovery: The action provides a subset of inputs at creation time, overwrites
-   *      {@link #discoverInputs} to return a superset of inputs depending on the execution context
-   *      and optionally prunes the inputs at the end of {@link #execute} to a required subset for
-   *      subsequent calls.
-   * </ul>
-   *
-   * This function allows an action that is set up for input discovery, and thus only provides a
-   * minimal set of inputs at creation time, to switch off input discovery depending on the
-   * execution context. To that end the action must:
-   * <ul>
-   * <li>Provide a subset of inputs at creation time
-   * <li>Return {@code null} from {@link #discoverInputs}, indicating no input discovery
-   * <li>Return a superset of inputs that might have been discovered otherwise from here;
-   *     this will usually be the set difference between the full set of inputs the action would get
-   *     when doing only execution time pruning and the minimal subset provided above.
-   * <li>Prune the set of inputs on execute
-   * </ul>
-   */
-  @Nullable
-  Iterable<Artifact> getInputsWhenSkippingInputDiscovery();
-
-  /**
    * 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.
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 ed88bebca5..150eced0fd 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
@@ -526,14 +526,21 @@ public class CppCompileAction extends AbstractAction
     }
 
     if (initialResult == null) {
-      // We will find inputs during execution. Store an empty list to show we did try to discover
-      // inputs and return null to inform the caller that inputs will be discovered later.
-      this.additionalInputs = ImmutableList.of();
-      return null;
+      NestedSetBuilder<Artifact> result = NestedSetBuilder.stableOrder();
+      if (useHeaderModules) {
+        // Here, we cannot really know what the top-level modules are, so we just mark all
+        // transitive modules as "top level".
+        topLevelModules = Sets.newLinkedHashSet(context.getTransitiveModules(usePic)
+            .toCollection());
+        result.addTransitive(context.getTransitiveModules(usePic));
+      }
+      result.addTransitive(includesExemptFromDiscovery);
+      additionalInputs = result.build();
+      return result.build();
     }
 
     Set<Artifact> initialResultSet = Sets.newLinkedHashSet(initialResult);
-    Iterables.addAll(initialResultSet, includesExemptFromDiscovery);
+
     if (shouldPruneModules) {
       usedModules = Sets.newLinkedHashSet();
       topLevelModules = null;
@@ -609,20 +616,6 @@ public class CppCompileAction extends AbstractAction
   }
 
   @Override
-  public Iterable<Artifact> getInputsWhenSkippingInputDiscovery() {
-    NestedSetBuilder<Artifact> result = NestedSetBuilder.stableOrder();
-    if (useHeaderModules) {
-      // Here, we cannot really know what the top-level modules are, so we just mark all
-      // transitive modules as "top level".
-      topLevelModules = Sets.newLinkedHashSet(context.getTransitiveModules(usePic).toCollection());
-      result.addTransitive(context.getTransitiveModules(usePic));
-    }
-    result.addTransitive(includesExemptFromDiscovery);
-    additionalInputs = result.build();
-    return result.build();
-  }
-
-  @Override
   public Artifact getPrimaryInput() {
     return getSourceFile();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
index 5d513a516d..886da30f05 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
@@ -30,7 +30,6 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
-import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction.SpecialInputsHandler;
@@ -282,9 +281,7 @@ public class CppCompileActionBuilder {
 
     NestedSet<Artifact> realMandatoryInputs = realMandatoryInputsBuilder.build();
 
-    NestedSet<Artifact> includesExemptFromDiscovery = shouldScanIncludes
-        ? NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER)
-        : context.getDeclaredIncludeSrcs();
+    NestedSet<Artifact> includesExemptFromDiscovery = context.getDeclaredIncludeSrcs();
 
     // Copying the collections is needed to make the builder reusable.
     if (fake) {
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 4eb7a6927e..49a741ac98 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
@@ -178,11 +178,6 @@ public class ObjcCompileAction extends SpawnAction {
 
   @Override
   public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext) {
-    return null;
-  }
-
-  @Override
-  public Iterable<Artifact> getInputsWhenSkippingInputDiscovery() {
     return filterHeaderFiles();
   }
 
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 1f9ad4ee6d..7398cd3dec 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
@@ -38,7 +38,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.util.BlazeClock;
-import com.google.devtools.build.lib.util.LoggingUtil;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
@@ -60,7 +59,6 @@ import java.util.Map.Entry;
 import java.util.Set;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.atomic.AtomicReference;
-import java.util.logging.Level;
 import javax.annotation.Nullable;
 
 /**
@@ -389,54 +387,39 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
           try {
             state.discoveredInputs = skyframeActionExecutor.discoverInputs(action,
                 perActionFileCache, metadataHandler, env);
+            Preconditions.checkState(state.discoveredInputs != null);
           } catch (MissingDepException e) {
             Preconditions.checkState(env.valuesMissing(), action);
             return null;
           }
         }
-        // state.discoveredInputs can be null even after include scanning if action discovers them
-        // during execution.
-        if (state.discoveredInputs != null) {
-          addDiscoveredInputs(state.inputArtifactData, state.discoveredInputs, env);
+        addDiscoveredInputs(state.inputArtifactData, state.discoveredInputs, env);
+        if (env.valuesMissing()) {
+          return null;
+        }
+        perActionFileCache = new PerActionFileCache(state.inputArtifactData);
+        metadataHandler =
+            new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get());
+
+        // Stage 1 finished, let's do stage 2. The stage 1 of input discovery will have added some
+        // files with addDiscoveredInputs() and then have waited for those files to be available
+        // by returning null if env.valuesMissing() returned true. So stage 2 can now access those
+        // inputs to discover even more inputs and then potentially also wait for those to be
+        // available.
+        if (state.discoveredInputsStage2 == null) {
+          state.discoveredInputsStage2 = action.discoverInputsStage2(env);
+        }
+        if (state.discoveredInputsStage2 != null) {
+          addDiscoveredInputs(state.inputArtifactData, state.discoveredInputsStage2, env);
           if (env.valuesMissing()) {
             return null;
           }
           perActionFileCache = new PerActionFileCache(state.inputArtifactData);
           metadataHandler =
               new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get());
-
-          // Stage 1 finished, let's do stage 2. The stage 1 of input discovery will have added some
-          // files with addDiscoveredInputs() and then have waited for those files to be available
-          // by returning null if env.valuesMissing() returned true. So stage 2 can now access those
-          // inputs to discover even more inputs and then potentially also wait for those to be
-          // available.
-          if (state.discoveredInputsStage2 == null) {
-            state.discoveredInputsStage2 = action.discoverInputsStage2(env);
-          }
-          if (state.discoveredInputsStage2 != null) {
-            addDiscoveredInputs(state.inputArtifactData, state.discoveredInputsStage2, env);
-            if (env.valuesMissing()) {
-              return null;
-            }
-            perActionFileCache = new PerActionFileCache(state.inputArtifactData);
-            metadataHandler =
-                new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get());
-          }
-        } else {
-          // The action generally tries to discover its inputs during execution. If there are any
-          // additional inputs necessary to execute the action, make sure they are available now.
-          Iterable<Artifact> requiredInputs = action.getInputsWhenSkippingInputDiscovery();
-          if (requiredInputs != null) {
-            addDiscoveredInputs(state.inputArtifactData, requiredInputs, env);
-            if (env.valuesMissing()) {
-              return null;
-            }
-            perActionFileCache = new PerActionFileCache(state.inputArtifactData);
-            metadataHandler =
-                new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get());
-          }
         }
       }
+
       actionExecutionContext =
           skyframeActionExecutor.getContext(perActionFileCache,
               metadataHandler, state.expandedArtifacts);
@@ -458,55 +441,23 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
           filterKnownInputs(action.getInputs(), state.inputArtifactData.keySet());
       Map<SkyKey, SkyValue> metadataFoundDuringActionExecution =
           env.getValues(toKeys(newInputs, action.getMandatoryInputs()));
-      if (state.discoveredInputs == null) {
-        // Include scanning didn't find anything beforehand -- these are the definitive discovered
-        // inputs.
-        state.discoveredInputs = newInputs;
-        if (env.valuesMissing()) {
-          return null;
-        }
-        if (!Iterables.isEmpty(newInputs)) {
-          // We are in the interesting case of an action that discovered its inputs during
-          // execution, and found some new ones, but the new ones were already present in the graph.
-          // We must therefore cache the metadata for those new ones.
-          Map<Artifact, FileArtifactValue> inputArtifactData = new HashMap<>();
-          inputArtifactData.putAll(state.inputArtifactData);
-          for (Map.Entry<SkyKey, SkyValue> entry : metadataFoundDuringActionExecution.entrySet()) {
-            inputArtifactData.put(
-                ArtifactSkyKey.artifact(entry.getKey()), (FileArtifactValue) entry.getValue());
-          }
-          state.inputArtifactData = inputArtifactData;
-          metadataHandler =
-              new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get());
-        }
-      } else if (!Iterables.isEmpty(newInputs)) {
-        // The action has run and discovered more inputs. This is a bug, probably the result of
-        // the action dynamically executing locally instead of remotely, and a discrepancy between
-        // our include scanning and the action's compiler. Fail the build so that the user notices,
-        // and also report the issue.
-        String errorMessageStart =
-            action.prettyPrint()
-                + " discovered unexpected inputs. This indicates a mismatch between the build"
-                + " system and the action's compiler. Please report this issue. The ";
-        String errorMessageEnd = "";
-        int artifactPrinted = 0;
-        for (Artifact extraArtifact : newInputs) {
-          if (artifactPrinted >= 10) {
-            errorMessageStart += "first ten ";
-            break;
-          }
-          if (artifactPrinted > 0) {
-            errorMessageEnd += ", ";
-          }
-          artifactPrinted++;
-          errorMessageEnd += extraArtifact.prettyPrint();
+      state.discoveredInputs = newInputs;
+      if (env.valuesMissing()) {
+        return null;
+      }
+      if (!Iterables.isEmpty(newInputs)) {
+        // We are in the interesting case of an action that discovered its inputs during
+        // execution, and found some new ones, but the new ones were already present in the graph.
+        // We must therefore cache the metadata for those new ones.
+        Map<Artifact, FileArtifactValue> inputArtifactData = new HashMap<>();
+        inputArtifactData.putAll(state.inputArtifactData);
+        for (Map.Entry<SkyKey, SkyValue> entry : metadataFoundDuringActionExecution.entrySet()) {
+          inputArtifactData.put(
+              ArtifactSkyKey.artifact(entry.getKey()), (FileArtifactValue) entry.getValue());
         }
-        errorMessageStart += "additional inputs found were: " + errorMessageEnd;
-        ActionExecutionException exception =
-            new ActionExecutionException(errorMessageStart, action, /*catastrophe=*/ false);
-        LoggingUtil.logToRemote(Level.SEVERE, errorMessageStart, exception);
-        throw skyframeActionExecutor.processAndThrow(
-            exception, action, actionExecutionContext.getFileOutErr());
+        state.inputArtifactData = inputArtifactData;
+        metadataHandler =
+            new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get());
       }
     }
     Preconditions.checkState(!env.valuesMissing(), action);