Remove AbtractAction#getInputsForExtraAction() (the third time).

This time, if the action discovers inputs, the extra action is made to depend on the outputs of the action so that by the time the extra action runs, the inputs of the original action are discovered. This avoids us having to think about the state the shadowed action may keep.

Yes, actions should not keep state, but they do. Such is life.

--
PiperOrigin-RevId: 148627715
MOS_MIGRATED_REVID=148627715

5 files changed, 13 insertions, 66 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 7919529da0..80f6d7df6b 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
@@ -495,27 +495,6 @@ public abstract class AbstractAction implements Action, SkylarkValue {
     return ImmutableSet.of();
   }
 
-  /**
-   * Returns input files that need to be present to allow extra_action rules to shadow this action
-   * correctly when run remotely. This is at least the normal inputs of the action, but may include
-   * other files as well. For example C(++) compilation may perform include file header scanning.
-   * This needs to be mirrored by the extra_action rule. Called by
-   * {@link com.google.devtools.build.lib.rules.extra.ExtraAction} at execution time.
-   *
-   * <p>As this method is called from the ExtraAction, make sure it is ok to call
-   * this method from a different thread than the one this action is executed on.
-   *
-   * @param actionExecutionContext Services in the scope of the action, like the Out/Err streams.
-   * @throws ActionExecutionException only when code called from this method
-   *     throws that exception.
-   * @throws InterruptedException if interrupted
-   */
-  public Iterable<Artifact> getInputFilesForExtraAction(
-      ActionExecutionContext actionExecutionContext)
-      throws ActionExecutionException, InterruptedException {
-    return getInputs();
-  }
-
   @SkylarkCallable(
       name = "inputs",
       doc = "A set of the input files of this action.",
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 1814e2516c..afc77d23c3 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
@@ -1265,32 +1265,6 @@ public class CppCompileAction extends AbstractAction
     }
   }
 
-  /**
-   * Provides list of include files needed for performing extra actions on this action when run
-   * remotely. The list of include files is created by performing a header scan on the known input
-   * files.
-   */
-  @Override
-  public Iterable<Artifact> getInputFilesForExtraAction(
-      ActionExecutionContext actionExecutionContext)
-      throws ActionExecutionException, InterruptedException {
-    Iterable<Artifact> scannedIncludes;
-    try {
-      scannedIncludes = actionExecutionContext.getExecutor().getContext(actionContext)
-          .findAdditionalInputs(this, actionExecutionContext,  cppSemantics.getIncludeProcessing());
-    } catch (ExecException e) {
-      throw e.toActionExecutionException(this);
-    }
-
-    if (scannedIncludes == null) {
-      return ImmutableList.of();
-    }
-
-    // Use a set to eliminate duplicates.
-    ImmutableSet.Builder<Artifact> result = ImmutableSet.builder();
-    return result.addAll(getInputs()).addAll(scannedIncludes).build();
-  }
-
   @Override
   public String getMnemonic() { return "CppCompile"; }
 
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 866e45e20a..a293b00497 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
@@ -18,6 +18,7 @@ import com.google.common.base.Function;
 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.AbstractAction;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
@@ -51,6 +52,8 @@ public final class ExtraAction extends SpawnAction {
   private final boolean createDummyOutput;
   private final RunfilesSupplier runfilesSupplier;
   private final ImmutableSet<Artifact> extraActionInputs;
+  private final Iterable<Artifact> originalShadowedActionInputs;
+
   // This can be read/written from multiple threads, and so accesses should be synchronized.
   @GuardedBy("this")
   private boolean inputsKnown;
@@ -95,6 +98,7 @@ public final class ExtraAction extends SpawnAction {
         mnemonic,
         false,
         null);
+    this.originalShadowedActionInputs = shadowedAction.getInputs();
     this.shadowedAction = shadowedAction;
     this.runfilesSupplier = runfilesSupplier;
     this.createDummyOutput = createDummyOutput;
@@ -117,15 +121,15 @@ public final class ExtraAction extends SpawnAction {
   public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
     Preconditions.checkState(discoversInputs(), this);
+    // We depend on the outputs of actions doing input discovery and they should know their inputs
+    // after having been executed
+    Preconditions.checkState(shadowedAction.inputsKnown());
+
     // We need to update our inputs to take account of any additional
     // inputs the shadowed action may need to do its work.
-    if (shadowedAction.discoversInputs() && shadowedAction instanceof AbstractAction) {
-      Iterable<Artifact> additionalInputs =
-          ((AbstractAction) shadowedAction).getInputFilesForExtraAction(actionExecutionContext);
-      updateInputs(createInputs(additionalInputs, extraActionInputs, runfilesSupplier));
-      return ImmutableSet.copyOf(additionalInputs);
-    }
-    return null;
+    updateInputs(createInputs(shadowedAction.getInputs(), extraActionInputs, runfilesSupplier));
+    return Sets.difference(ImmutableSet.copyOf(shadowedAction.getInputs()),
+        ImmutableSet.copyOf(originalShadowedActionInputs));
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionSpec.java b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionSpec.java
index 81f1a8683d..db3569a370 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionSpec.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionSpec.java
@@ -85,9 +85,10 @@ public final class ExtraActionSpec implements TransitiveInfoProvider {
     NestedSetBuilder<Artifact> extraActionInputs = NestedSetBuilder.stableOrder();
 
     Label ownerLabel = owningRule.getLabel();
-    if (requiresActionOutput) {
+    if (requiresActionOutput || actionToShadow.discoversInputs()) {
       extraActionInputs.addAll(actionToShadow.getOutputs());
     }
+
     extraActionInputs.addAll(resolvedTools);
     extraActionInputs.addAll(resolvedData);
 
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 f3d185cc01..8164cc3c85 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
@@ -387,17 +387,6 @@ public class ObjcCompileAction extends SpawnAction {
   }
 
   @Override
-  public Iterable<Artifact> getInputFilesForExtraAction(
-      ActionExecutionContext actionExecutionContext)
-      throws ActionExecutionException, InterruptedException {
-    // Use a set to eliminate duplicates.
-    return ImmutableSet.<Artifact>builder()
-        .addAll(getInputs())
-        .addAll(discoverInputs(actionExecutionContext))
-        .build();
-  }
-
-  @Override
   protected SpawnInfo getExtraActionSpawnInfo() {
     SpawnInfo.Builder info = SpawnInfo.newBuilder(super.getExtraActionSpawnInfo());
     if (!inputsKnown()) {