Fix extra_actions with sandboxing.

The ExtraAction code assumed that it didn't need to list the runfiles
of its tools when running locally, but this isn't true with sandboxing.

I don't think fixing this will negatively affect anybody's performance
because they probably don't have any runfiles because they currently
can't use them, unless they're running actions remotely in which case
this change has no effect.

--
Change-Id: Ibeb3db9d31321912a7163d1bce0edf5f6288ea3e
Reviewed-on: https://bazel-review.googlesource.com/#/c/2520/
MOS_MIGRATED_REVID=112670586

2 files changed, 17 insertions, 24 deletions

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 88e51e8a03..2e0f25bf6d 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
@@ -27,8 +27,6 @@ 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.DelegateSpawn;
-import com.google.devtools.build.lib.actions.ExecException;
-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.Spawn;
@@ -133,16 +131,13 @@ public final class ExtraAction extends SpawnAction {
   public Collection<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
     Preconditions.checkState(discoversInputs(), this);
-    if (getContext(actionExecutionContext.getExecutor()).isRemotable(getMnemonic(),
-        isRemotable())) {
-      // If we're running remotely, 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));
-        return ImmutableSet.copyOf(additionalInputs);
-      }
+    // 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));
+      return ImmutableSet.copyOf(additionalInputs);
     }
     return null;
   }
@@ -192,19 +187,9 @@ public final class ExtraAction extends SpawnAction {
   @Override
   public void execute(ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
-    Executor executor = actionExecutionContext.getExecutor();
-
     // PHASE 2: execution of extra_action.
 
-    if (getContext(executor).isRemotable(getMnemonic(), isRemotable())) {
-      try {
-        getContext(executor).exec(getExtraActionSpawn(), actionExecutionContext);
-      } catch (ExecException e) {
-        throw e.toActionExecutionException(this);
-      }
-    } else {
-      super.execute(actionExecutionContext);
-    }
+    super.execute(actionExecutionContext);
 
     // PHASE 3: create dummy output.
     // If the user didn't specify output, we need to create dummy output
@@ -231,7 +216,8 @@ public final class ExtraAction extends SpawnAction {
    * before the SpawnAction so should not be listed as one of its outputs.
    */
   // TODO(bazel-team): Add more tests that execute this code path!
-  private Spawn getExtraActionSpawn() {
+  @Override
+  public Spawn getSpawn() {
     final Spawn base = super.getSpawn();
     return new DelegateSpawn(base) {
       @Override public ImmutableMap<PathFragment, Artifact> getRunfilesManifests() {
diff --git a/src/test/shell/bazel/bazel_rules_test.sh b/src/test/shell/bazel/bazel_rules_test.sh
index 7f18159997..2547ef57d2 100755
--- a/src/test/shell/bazel/bazel_rules_test.sh
+++ b/src/test/shell/bazel/bazel_rules_test.sh
@@ -63,6 +63,10 @@ function test_extra_action() {
   # a program that parses the proto here.
   cat > mypkg/echoer.sh <<EOF
 #!/bin/bash
+if [[ ! -e \$0.runfiles/mypkg/runfile ]]; then
+  echo "Runfile not found" >&2
+  exit 1
+fi
 echo EXTRA ACTION FILE: \$1
 EOF
   chmod +x mypkg/echoer.sh
@@ -75,6 +79,8 @@ public class Hello {
 }
 EOF
 
+  touch mypkg/runfile
+
   cat > mypkg/BUILD <<EOF
 package(default_visibility = ["//visibility:public"])
 
@@ -93,6 +99,7 @@ action_listener(
 sh_binary(
     name = "echoer",
     srcs = ["echoer.sh"],
+    data = ["runfile"],
 )
 
 java_library(