Automated rollback of commit 4465dae23de989f1452e93d0a88ac2a289103dd9.

*** Reason for rollback ***

The no-cache tag is not respected (see b/77857812) and thus this breaks remote caching for all projects with symlink outputs.

*** Original change description ***

Only allow regular files and directories spawn outputs to be uploaded to a remote cache.

The remote cache protocol only knows about regular files and
directories. Currently, during action output upload, symlinks are
resolved into regular files. This means cached "executions" of an
action may have different output file types than the original
execution, which can be a footgun. This CL bans symlinks from cachable
spawn outputs and fixes http...

***

PiperOrigin-RevId: 193338629

3 files changed, 16 insertions, 87 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
index 42de712e40..e9731210d3 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.testutil.TestSpec;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import java.util.Collection;
 import java.util.List;
 import org.junit.Before;
 import org.junit.Test;
@@ -146,7 +147,7 @@ public class AbstractSpawnStrategyTest {
 
     // Must only be called exactly once.
     verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionPolicy.class));
-    verify(entry).store(eq(spawnResult));
+    verify(entry).store(eq(spawnResult), any(Collection.class));
   }
 
   @SuppressWarnings("unchecked")
@@ -177,6 +178,6 @@ public class AbstractSpawnStrategyTest {
     }
     // Must only be called exactly once.
     verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionPolicy.class));
-    verify(entry).store(eq(result));
+    verify(entry).store(eq(result), any(Collection.class));
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java
deleted file mode 100644
index b4092a043e..0000000000
--- a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java
+++ /dev/null
@@ -1,71 +0,0 @@
-// Copyright 2018 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.remote;
-
-import static com.google.common.truth.Truth.assertThat;
-import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
-
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.actions.ExecException;
-import com.google.devtools.build.lib.clock.JavaClock;
-import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.UploadManifest;
-import com.google.devtools.build.lib.remote.util.DigestUtil;
-import com.google.devtools.build.lib.vfs.FileSystem;
-import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
-import com.google.devtools.build.lib.vfs.Path;
-import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
-import com.google.devtools.remoteexecution.v1test.ActionResult;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
-/** Tests for {@link AbstractRemoteActionCache}. */
-@RunWith(JUnit4.class)
-public class AbstractRemoteActionCacheTests {
-
-  private FileSystem fs;
-  private Path execRoot;
-  private final DigestUtil digestUtil = new DigestUtil(HashFunction.SHA256);
-
-  @Before
-  public void setUp() throws Exception {
-    fs = new InMemoryFileSystem(new JavaClock(), HashFunction.SHA256);
-    execRoot = fs.getPath("/execroot");
-    execRoot.createDirectory();
-  }
-
-  @Test
-  public void uploadSymlinkAsFile() throws Exception {
-    ActionResult.Builder result = ActionResult.newBuilder();
-    UploadManifest um = new UploadManifest(digestUtil, result, execRoot);
-    Path link = fs.getPath("/execroot/link");
-    link.createSymbolicLink(fs.getPath("/execroot/target"));
-    assertThat(assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(link))))
-        .hasMessageThat()
-        .contains("Only regular files and directories may be uploaded to a remote cache.");
-  }
-
-  @Test
-  public void uploadSymlinkInDirectory() throws Exception {
-    ActionResult.Builder result = ActionResult.newBuilder();
-    UploadManifest um = new UploadManifest(digestUtil, result, fs.getPath("/execroot"));
-    Path dir = fs.getPath("/execroot/dir");
-    dir.createDirectory();
-    fs.getPath("/execroot/dir/link").createSymbolicLink(fs.getPath("/execroot/target"));
-    assertThat(assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(dir))))
-        .hasMessageThat()
-        .contains("Only regular files and directories may be uploaded to a remote cache.");
-  }
-}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
index a41f9c4f96..0374e3dc38 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -165,7 +165,7 @@ public class RemoteSpawnCacheTest {
             ImmutableMap.of("VARIABLE", "value"),
             /*executionInfo=*/ ImmutableMap.<String, String>of(),
             /*inputs=*/ ImmutableList.of(ActionInputHelper.fromPath("input")),
-            /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath("/random/file")),
+            /*outputs=*/ ImmutableList.<ActionInput>of(),
             ResourceSet.ZERO);
 
     Path stdout = fs.getPath("/tmp/stdout");
@@ -267,7 +267,7 @@ public class RemoteSpawnCacheTest {
             })
         .when(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
-    entry.store(result);
+    entry.store(result, outputFiles);
     verify(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
   }
@@ -277,15 +277,14 @@ public class RemoteSpawnCacheTest {
     // Checks that spawns that have mayBeCached false are not looked up in the remote cache,
     // and also that their result is not uploaded to the remote cache. The artifacts, however,
     // are uploaded.
-    SimpleSpawn uncacheableSpawn =
-        new SimpleSpawn(
-            new FakeOwner("foo", "bar"),
-            /*arguments=*/ ImmutableList.of(),
-            /*environment=*/ ImmutableMap.of(),
-            ImmutableMap.of(ExecutionRequirements.NO_CACHE, ""),
-            /*inputs=*/ ImmutableList.of(),
-            /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath("/random/file")),
-            ResourceSet.ZERO);
+    SimpleSpawn uncacheableSpawn = new SimpleSpawn(
+        new FakeOwner("foo", "bar"),
+        /*arguments=*/ ImmutableList.of(),
+        /*environment=*/ ImmutableMap.of(),
+        ImmutableMap.of(ExecutionRequirements.NO_CACHE, ""),
+        /*inputs=*/ ImmutableList.of(),
+        /*outputs=*/ ImmutableList.<ActionInput>of(),
+        ResourceSet.ZERO);
     CacheHandle entry = cache.lookup(uncacheableSpawn, simplePolicy);
     verify(remoteCache, never())
         .getCachedActionResult(any(ActionKey.class));
@@ -296,8 +295,8 @@ public class RemoteSpawnCacheTest {
             .setStatus(Status.SUCCESS)
             .setRunnerName("test")
             .build();
-    entry.store(result);
     ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
+    entry.store(result, outputFiles);
     verify(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false));
   }
@@ -316,7 +315,7 @@ public class RemoteSpawnCacheTest {
             .setRunnerName("test")
             .build();
     ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
-    entry.store(result);
+    entry.store(result, outputFiles);
     verify(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false));
   }
@@ -337,7 +336,7 @@ public class RemoteSpawnCacheTest {
         .when(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
 
-    entry.store(result);
+    entry.store(result, outputFiles);
     verify(remoteCache)
         .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));