Encore of commit 41c2a26eef89167e807cbc9f33487dc66bb757d3 that removed AbstractAction#getInputFilesForExtraAction().

Turns out, we didn't add *mandatory* inputs of the shadowed action to the extra action, thus, breakage.

Original description:
Remove AbstractAction#getInputFilesForExtraAction().

This method was used to return the discovered inputs for extra actions, but it turns out that we can use #discoverInputs() just as well.

Note that this makes it possible for #discoverInputs() to be called more than once per action instance (once for the action and once for each extra action), but this appears to work. A followup change may be able to dispense with that, but let's take baby steps for now.

Also note that this introduces synchronization between an action and its associated extra action.

--
PiperOrigin-RevId: 148429641
MOS_MIGRATED_REVID=148429641

4 files changed, 23 insertions, 59 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..a8b460e823 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
@@ -174,7 +174,8 @@ public abstract class AbstractAction implements Action, SkylarkValue {
   }
 
   @Override
-  public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
+  public synchronized Iterable<Artifact> discoverInputs(
+      ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
     throw new IllegalStateException("discoverInputs cannot be called for " + this.prettyPrint()
         + " since it does not discover inputs");
@@ -495,27 +496,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 9936319435..3f8437087f 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
@@ -436,7 +436,8 @@ public class CppCompileAction extends AbstractAction
 
   @Nullable
   @Override
-  public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
+  public synchronized Iterable<Artifact> discoverInputs(
+      ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
     Executor executor = actionExecutionContext.getExecutor();
     Iterable<Artifact> initialResult;
@@ -1263,32 +1264,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/cpp/LTOBackendAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LTOBackendAction.java
index 0b71041ceb..e6d35fe128 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
@@ -117,7 +117,8 @@ public final class LTOBackendAction extends SpawnAction {
 
   @Nullable
   @Override
-  public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
+  public synchronized Iterable<Artifact> discoverInputs(
+      ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
     // Build set of files this LTO backend artifact will import from.
     HashSet<PathFragment> importSet = new HashSet<>();
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..a135ff75d5 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.Iterables;
 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,7 @@ public final class ExtraAction extends SpawnAction {
   private final boolean createDummyOutput;
   private final RunfilesSupplier runfilesSupplier;
   private final ImmutableSet<Artifact> extraActionInputs;
+
   // This can be read/written from multiple threads, and so accesses should be synchronized.
   @GuardedBy("this")
   private boolean inputsKnown;
@@ -82,7 +84,7 @@ public final class ExtraAction extends SpawnAction {
     super(
         shadowedAction.getOwner(),
         ImmutableList.<Artifact>of(),
-        createInputs(shadowedAction.getInputs(), extraActionInputs, runfilesSupplier),
+        createInputs(shadowedAction.getMandatoryInputs(), extraActionInputs, runfilesSupplier),
         outputs,
         AbstractAction.DEFAULT_RESOURCE_SET,
         argv,
@@ -114,18 +116,24 @@ public final class ExtraAction extends SpawnAction {
 
   @Nullable
   @Override
-  public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
+  public synchronized Iterable<Artifact> discoverInputs(
+      ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
+    // Note that discoverInputsStage2() is neither implemented nor called on the shadowed action.
+    // This means that the input files of extra actions and shadowed actions can be different, but
+    // it doesn't seem to be a problem in practice and we hope that we can eventually replace this
+    // awkward hardwired two-stage mechanism with something more principled, for example, being able
+    // to declare arbitrary Skyframe dependencies on actions and extra actions waiting for shadowed
+    // actions that make use of this facility to finish.
     Preconditions.checkState(discoversInputs(), this);
     // 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;
+    Iterable<Artifact> additionalInputs = shadowedAction.discoverInputs(actionExecutionContext);
+    updateInputs(createInputs(
+        Iterables.concat(shadowedAction.getMandatoryInputs(), additionalInputs),
+        extraActionInputs,
+        runfilesSupplier));
+    return additionalInputs;
   }
 
   @Override