Allow banning symlink action outputs from being uploaded to a remote cache.

This is mostly a roll-forward of 4465dae23de989f1452e93d0a88ac2a289103dd9, which
was reverted by fa36d2f48965b127e8fd397348d16e991135bfb6. The main difference is
that the new behavior is now gated behind the --noremote_allow_symlink_upload
flag.

https://docs.google.com/document/d/1gnOYszitgrLVet3sQk-TKGqIcpkkDsc6aw-izoo-d64
is a design proposal to support symlinks in the remote cache, which would render
this change moot. I'd like to be able to prevent incorrect cache behavior until
that change is implemented, though.

This fixes https://github.com/bazelbuild/bazel/issues/4840 (again).

Closes #5122.

Change-Id: I2136cfe82c2e1a8a9f5856e12a37d42cabd0e299
PiperOrigin-RevId: 195261827

2 files changed, 29 insertions, 50 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
index d8c42ff140..6745b1a96e 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
@@ -41,7 +41,6 @@ import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
 import java.time.Duration;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.SortedMap;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -95,8 +94,7 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte
           // Actual execution.
           spawnResult = spawnRunner.exec(spawn, context);
           if (cacheHandle.willStore()) {
-            cacheHandle.store(
-                spawnResult, listExistingOutputFiles(spawn, actionExecutionContext.getExecRoot()));
+            cacheHandle.store(spawnResult);
           }
         }
       }
@@ -144,19 +142,6 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte
     return ImmutableList.of(spawnResult);
   }
 
-  private List<Path> listExistingOutputFiles(Spawn spawn, Path execRoot) {
-    ArrayList<Path> outputFiles = new ArrayList<>();
-    for (ActionInput output : spawn.getOutputFiles()) {
-      Path outputPath = execRoot.getRelative(output.getExecPathString());
-      // TODO(ulfjack): Store the actual list of output files in SpawnResult and use that instead
-      // of statting the files here again.
-      if (outputPath.exists()) {
-        outputFiles.add(outputPath);
-      }
-    }
-    return outputFiles;
-  }
-
   private final class SpawnExecutionContextImpl implements SpawnExecutionContext {
     private final Spawn spawn;
     private final ActionExecutionContext actionExecutionContext;
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
index afc9d9376b..48bdaf5c81 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
@@ -19,10 +19,8 @@ import com.google.devtools.build.lib.actions.ExecutionStrategy;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
-import com.google.devtools.build.lib.vfs.Path;
 import java.io.Closeable;
 import java.io.IOException;
-import java.util.Collection;
 import java.util.NoSuchElementException;
 
 /**
@@ -33,32 +31,31 @@ import java.util.NoSuchElementException;
  */
 public interface SpawnCache extends ActionContext {
   /** A no-op implementation that has no result, and performs no upload. */
-  public static CacheHandle NO_RESULT_NO_STORE = new CacheHandle() {
-    @Override
-    public boolean hasResult() {
-      return false;
-    }
-
-    @Override
-    public SpawnResult getResult() {
-      throw new NoSuchElementException();
-    }
-
-    @Override
-    public boolean willStore() {
-      return false;
-    }
-
-    @Override
-    public void store(SpawnResult result, Collection<Path> files)
-        throws InterruptedException, IOException {
-      // Do nothing.
-    }
-
-    @Override
-    public void close() {
-    }
-  };
+  public static CacheHandle NO_RESULT_NO_STORE =
+      new CacheHandle() {
+        @Override
+        public boolean hasResult() {
+          return false;
+        }
+
+        @Override
+        public SpawnResult getResult() {
+          throw new NoSuchElementException();
+        }
+
+        @Override
+        public boolean willStore() {
+          return false;
+        }
+
+        @Override
+        public void store(SpawnResult result) throws InterruptedException, IOException {
+          // Do nothing.
+        }
+
+        @Override
+        public void close() {}
+      };
 
   /**
    * Helper method to create a {@link CacheHandle} from a successful {@link SpawnResult} instance.
@@ -81,14 +78,12 @@ public interface SpawnCache extends ActionContext {
       }
 
       @Override
-      public void store(SpawnResult result, Collection<Path> files)
-          throws InterruptedException, IOException {
+      public void store(SpawnResult result) throws InterruptedException, IOException {
         throw new IllegalStateException();
       }
 
       @Override
-      public void close() {
-      }
+      public void close() {}
     };
   }
 
@@ -148,8 +143,7 @@ public interface SpawnCache extends ActionContext {
      * <p>If the current thread is interrupted, then this method should return as quickly as
      * possible with an {@link InterruptedException}.
      */
-    void store(SpawnResult result, Collection<Path> files)
-        throws InterruptedException, IOException;
+    void store(SpawnResult result) throws ExecException, InterruptedException, IOException;
   }
 
   /**