Stop uploading the Action proto to the CAS.

The Action proto describes the contents of an AC entry. The spec doesn't require it to be also stored in the CAS. Also rename variables for clarity. PiperOrigin-RevId: 567845929 Change-Id: I249c9cc817051713e65143fca98ac5f09b1f0828
bazelbuild · Sep 23, 2023 · 4482d6c · 4482d6c
1 parent 92a6cdf
commit 4482d6c
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 52 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java b/src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java
@@ -84,8 +84,8 @@ public class UploadManifest {
   private final boolean followSymlinks;
   private final boolean allowDanglingSymlinks;
   private final boolean allowAbsoluteSymlinks;
-  private final Map<Digest, Path> digestToFile = new HashMap<>();
-  private final Map<Digest, ByteString> digestToBlobs = new HashMap<>();
+  private final Map<Digest, Path> casDigestToFile = new HashMap<>();
+  private final Map<Digest, ByteString> casDigestToBlob = new HashMap<>();
   @Nullable private ActionKey actionKey;
   private Digest stderrDigest;
   private Digest stdoutDigest;
@@ -172,11 +172,11 @@ public UploadManifest(
   private void setStdoutStderr(FileOutErr outErr) throws IOException {
     if (outErr.getErrorPath().exists()) {
       stderrDigest = digestUtil.compute(outErr.getErrorPath());
-      digestToFile.put(stderrDigest, outErr.getErrorPath());
+      casDigestToFile.put(stderrDigest, outErr.getErrorPath());
     }
     if (outErr.getOutputPath().exists()) {
       stdoutDigest = digestUtil.compute(outErr.getOutputPath());
-      digestToFile.put(stdoutDigest, outErr.getOutputPath());
+      casDigestToFile.put(stdoutDigest, outErr.getOutputPath());
     }
   }
 
@@ -255,22 +255,21 @@ void addFiles(Collection<Path> files) throws ExecException, IOException {
   private void addAction(RemoteCacheClient.ActionKey actionKey, Action action, Command command) {
     Preconditions.checkState(this.actionKey == null, "Already added an action");
     this.actionKey = actionKey;
-    digestToBlobs.put(actionKey.getDigest(), action.toByteString());
-    digestToBlobs.put(action.getCommandDigest(), command.toByteString());
+    casDigestToBlob.put(action.getCommandDigest(), command.toByteString());
   }
 
   /** Map of digests to file paths to upload. */
-  public Map<Digest, Path> getDigestToFile() {
-    return digestToFile;
+  public Map<Digest, Path> getCasDigestToFile() {
+    return casDigestToFile;
   }
 
   /**
    * Map of digests to chunkers to upload. When the file is a regular, non-directory file it is
-   * transmitted through {@link #getDigestToFile()}. When it is a directory, it is transmitted as a
-   * {@link Tree} protobuf message through {@link #getDigestToBlobs()}.
+   * transmitted through {@link #getCasDigestToFile()}. When it is a directory, it is transmitted as
+   * a {@link Tree} protobuf message through {@link #getCasDigestToBlob()}.
    */
-  public Map<Digest, ByteString> getDigestToBlobs() {
-    return digestToBlobs;
+  public Map<Digest, ByteString> getCasDigestToBlob() {
+    return casDigestToBlob;
   }
 
   @Nullable
@@ -311,7 +310,7 @@ private void addFile(Digest digest, Path file) throws IOException {
         // The permission of output file is changed to 0555 after action execution
         .setIsExecutable(true);
 
-    digestToFile.put(digest, file);
+    casDigestToFile.put(digest, file);
   }
 
   // Field numbers of the 'root' and 'directory' fields in the Tree message.
@@ -347,7 +346,7 @@ private void addDirectory(Path dir) throws ExecException, IOException {
           .setIsTopologicallySorted(true);
     }
 
-    digestToBlobs.put(digest, data);
+    casDigestToBlob.put(digest, data);
   }
 
   private ByteString computeDirectory(Path path, Set<ByteString> directories)
@@ -379,7 +378,7 @@ private ByteString computeDirectory(Path path, Set<ByteString> directories)
         if (statFollow.isFile() && !statFollow.isSpecialFile()) {
           Digest digest = digestUtil.compute(child);
           b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable());
-          digestToFile.put(digest, child);
+          casDigestToFile.put(digest, child);
         } else if (statFollow.isDirectory()) {
           ByteString dir = computeDirectory(child, directories);
           b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir.toByteArray()));
@@ -389,7 +388,7 @@ private ByteString computeDirectory(Path path, Set<ByteString> directories)
       } else if (dirent.getType() == Dirent.Type.FILE) {
         Digest digest = digestUtil.compute(child);
         b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable());
-        digestToFile.put(digest, child);
+        casDigestToFile.put(digest, child);
       } else {
         illegalOutput(child);
       }
@@ -439,12 +438,12 @@ public ActionResult upload(
 
   private Completable upload(
       RemoteActionExecutionContext context, RemoteCache remoteCache, Digest digest) {
-    Path file = digestToFile.get(digest);
+    Path file = casDigestToFile.get(digest);
     if (file != null) {
       return toCompletable(() -> remoteCache.uploadFile(context, digest, file), directExecutor());
     }
 
-    ByteString blob = digestToBlobs.get(digest);
+    ByteString blob = casDigestToBlob.get(digest);
     if (blob == null) {
       String message = "FindMissingBlobs call returned an unknown digest: " + digest;
       return Completable.error(new IOException(message));
@@ -487,8 +486,8 @@ public Single<ActionResult> uploadAsync(
       RemoteCache remoteCache,
       ExtendedEventHandler reporter) {
     Collection<Digest> digests = new ArrayList<>();
-    digests.addAll(digestToFile.keySet());
-    digests.addAll(digestToBlobs.keySet());
+    digests.addAll(casDigestToFile.keySet());
+    digests.addAll(casDigestToBlob.keySet());
 
     ActionExecutionMetadata action = context.getSpawnOwner();
 

diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java
@@ -620,8 +620,7 @@ public void findMissingBlobs(
               FindMissingBlobsRequest request,
               StreamObserver<FindMissingBlobsResponse> responseObserver) {
             assertThat(request.getBlobDigestsList())
-                .containsExactly(
-                    cmdDigest, actionDigest, fooDigest, barDigest, stdoutDigest, stderrDigest);
+                .containsExactly(cmdDigest, fooDigest, barDigest, stdoutDigest, stderrDigest);
             // Nothing is missing.
             responseObserver.onNext(FindMissingBlobsResponse.getDefaultInstance());
             responseObserver.onCompleted();
@@ -725,7 +724,7 @@ public void updateActionResult(
         .setDigest(barDigest)
         .setIsExecutable(true);
     assertThat(result).isEqualTo(expectedResult.build());
-    assertThat(numGetMissingCalls.get()).isEqualTo(4);
+    assertThat(numGetMissingCalls.get()).isEqualTo(3);
   }
 
   @Test

diff --git a/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java b/src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java
@@ -214,7 +214,7 @@ public void actionResult_followSymlinks_absoluteFileSymlinkAsFile() throws Excep
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(link));
     Digest digest = digestUtil.compute(target);
-    assertThat(um.getDigestToFile()).containsExactly(digest, link);
+    assertThat(um.getCasDigestToFile()).containsExactly(digest, link);
 
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest).setIsExecutable(true);
@@ -241,7 +241,7 @@ public void actionResult_followSymlinks_absoluteDirectorySymlinkAsDirectory() th
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(link));
     Digest digest = digestUtil.compute(foo);
-    assertThat(um.getDigestToFile()).containsExactly(digest, execRoot.getRelative("link/foo"));
+    assertThat(um.getCasDigestToFile()).containsExactly(digest, execRoot.getRelative("link/foo"));
 
     Tree tree =
         Tree.newBuilder()
@@ -278,7 +278,7 @@ public void actionResult_noFollowSymlinks_absoluteFileSymlinkAsFile() throws Exc
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(link));
     Digest digest = digestUtil.compute(target);
-    assertThat(um.getDigestToFile()).containsExactly(digest, link);
+    assertThat(um.getCasDigestToFile()).containsExactly(digest, link);
 
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest).setIsExecutable(true);
@@ -305,7 +305,7 @@ public void actionResult_noFollowSymlinks_absoluteDirectorySymlinkAsDirectory()
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(link));
     Digest digest = digestUtil.compute(foo);
-    assertThat(um.getDigestToFile()).containsExactly(digest, execRoot.getRelative("link/foo"));
+    assertThat(um.getCasDigestToFile()).containsExactly(digest, execRoot.getRelative("link/foo"));
 
     Tree tree =
         Tree.newBuilder()
@@ -342,7 +342,7 @@ public void actionResult_followSymlinks_relativeFileSymlinkAsFile() throws Excep
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(link));
     Digest digest = digestUtil.compute(target);
-    assertThat(um.getDigestToFile()).containsExactly(digest, link);
+    assertThat(um.getCasDigestToFile()).containsExactly(digest, link);
 
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputFilesBuilder().setPath("link").setDigest(digest).setIsExecutable(true);
@@ -369,7 +369,7 @@ public void actionResult_followSymlinks_relativeDirectorySymlinkAsDirectory() th
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(link));
     Digest digest = digestUtil.compute(foo);
-    assertThat(um.getDigestToFile()).containsExactly(digest, execRoot.getRelative("link/foo"));
+    assertThat(um.getCasDigestToFile()).containsExactly(digest, execRoot.getRelative("link/foo"));
 
     Tree tree =
         Tree.newBuilder()
@@ -405,7 +405,7 @@ public void actionResult_noFollowSymlinks_relativeFileSymlinkAsSymlink() throws
             /*allowDanglingSymlinks=*/ false,
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(link));
-    assertThat(um.getDigestToFile()).isEmpty();
+    assertThat(um.getCasDigestToFile()).isEmpty();
 
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputFileSymlinksBuilder().setPath("link").setTarget("target");
@@ -432,7 +432,7 @@ public void actionResult_noFollowSymlinks_relativeDirectorySymlinkAsSymlink() th
             /*allowDanglingSymlinks=*/ false,
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(link));
-    assertThat(um.getDigestToFile()).isEmpty();
+    assertThat(um.getCasDigestToFile()).isEmpty();
 
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputDirectorySymlinksBuilder().setPath("link").setTarget("dir");
@@ -503,7 +503,7 @@ public void actionResult_noFollowSymlinks_noAllowDanglingSymlinks_absoluteDangli
             /*allowDanglingSymlinks=*/ true,
             /*allowAbsoluteSymlinks=*/ true);
     um.addFiles(ImmutableList.of(link));
-    assertThat(um.getDigestToFile()).isEmpty();
+    assertThat(um.getCasDigestToFile()).isEmpty();
 
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputFileSymlinksBuilder().setPath("link").setTarget("/execroot/target");
@@ -550,7 +550,7 @@ public void actionResult_noFollowSymlinks_allowDanglingSymlinks_relativeDangling
             /*allowDanglingSymlinks=*/ true,
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(link));
-    assertThat(um.getDigestToFile()).isEmpty();
+    assertThat(um.getCasDigestToFile()).isEmpty();
 
     ActionResult.Builder expectedResult = ActionResult.newBuilder();
     expectedResult.addOutputFileSymlinksBuilder().setPath("link").setTarget("target");
@@ -578,7 +578,7 @@ public void actionResult_followSymlinks_absoluteFileSymlinkInDirectoryAsFile() t
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(dir));
     Digest digest = digestUtil.compute(target);
-    assertThat(um.getDigestToFile()).containsExactly(digest, link);
+    assertThat(um.getCasDigestToFile()).containsExactly(digest, link);
 
     Tree tree =
         Tree.newBuilder()
@@ -620,7 +620,8 @@ public void actionResult_followSymlinks_absoluteDirectorySymlinkInDirectoryAsDir
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(dir));
     Digest digest = digestUtil.compute(foo);
-    assertThat(um.getDigestToFile()).containsExactly(digest, execRoot.getRelative("dir/link/foo"));
+    assertThat(um.getCasDigestToFile())
+        .containsExactly(digest, execRoot.getRelative("dir/link/foo"));
 
     Directory barDir =
         Directory.newBuilder()
@@ -667,7 +668,7 @@ public void actionResult_noFollowSymlinks_absoluteFileSymlinkInDirectoryAsFile()
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(dir));
     Digest digest = digestUtil.compute(target);
-    assertThat(um.getDigestToFile()).containsExactly(digest, link);
+    assertThat(um.getCasDigestToFile()).containsExactly(digest, link);
 
     Tree tree =
         Tree.newBuilder()
@@ -709,7 +710,8 @@ public void actionResult_noFollowSymlinks_absoluteDirectorySymlinkInDirectoryAsD
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(dir));
     Digest digest = digestUtil.compute(foo);
-    assertThat(um.getDigestToFile()).containsExactly(digest, execRoot.getRelative("dir/link/foo"));
+    assertThat(um.getCasDigestToFile())
+        .containsExactly(digest, execRoot.getRelative("dir/link/foo"));
 
     Directory barDir =
         Directory.newBuilder()
@@ -755,7 +757,7 @@ public void actionResult_followSymlinks_relativeFileSymlinkInDirectoryAsFile() t
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(dir));
     Digest digest = digestUtil.compute(target);
-    assertThat(um.getDigestToFile()).containsExactly(digest, link);
+    assertThat(um.getCasDigestToFile()).containsExactly(digest, link);
 
     Tree tree =
         Tree.newBuilder()
@@ -797,7 +799,8 @@ public void actionResult_followSymlinks_relativeDirectorySymlinkInDirectoryAsDir
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(dir));
     Digest digest = digestUtil.compute(foo);
-    assertThat(um.getDigestToFile()).containsExactly(digest, execRoot.getRelative("dir/link/foo"));
+    assertThat(um.getCasDigestToFile())
+        .containsExactly(digest, execRoot.getRelative("dir/link/foo"));
 
     Directory barDir =
         Directory.newBuilder()
@@ -843,7 +846,7 @@ public void actionResult_noFollowSymlinks_relativeFileSymlinkInDirectoryAsSymlin
             /*allowDanglingSymlinks=*/ false,
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(dir));
-    assertThat(um.getDigestToFile()).isEmpty();
+    assertThat(um.getCasDigestToFile()).isEmpty();
 
     Tree tree =
         Tree.newBuilder()
@@ -884,7 +887,7 @@ public void actionResult_noFollowSymlinks_relativeDirectorySymlinkInDirectoryAsS
             /*allowDanglingSymlinks=*/ false,
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(dir));
-    assertThat(um.getDigestToFile()).isEmpty();
+    assertThat(um.getCasDigestToFile()).isEmpty();
 
     Tree tree =
         Tree.newBuilder()
@@ -973,7 +976,7 @@ public void actionResult_noFollowSymlinks_relativeDirectorySymlinkInDirectoryAsS
             /*allowDanglingSymlinks=*/ false,
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(dir));
-    assertThat(um.getDigestToFile()).isEmpty();
+    assertThat(um.getCasDigestToFile()).isEmpty();
 
     Tree tree =
         Tree.newBuilder()
@@ -1012,7 +1015,7 @@ public void actionResult_noFollowSymlinks_relativeDirectorySymlinkInDirectoryAsS
             /*allowDanglingSymlinks=*/ true,
             /*allowAbsoluteSymlinks=*/ false);
     um.addFiles(ImmutableList.of(dir));
-    assertThat(um.getDigestToFile()).isEmpty();
+    assertThat(um.getCasDigestToFile()).isEmpty();
 
     Tree tree =
         Tree.newBuilder()

diff --git a/src/test/shell/bazel/remote/remote_build_event_uploader_test.sh b/src/test/shell/bazel/remote/remote_build_event_uploader_test.sh
@@ -224,11 +224,11 @@ EOF
 function test_upload_minimal_respect_no_upload_results_combined_cache() {
   local cache_dir="${TEST_TMPDIR}/disk_cache"
   mkdir -p a
-  cat > a/BUILD <<EOF
+  cat > a/BUILD <<'EOF'
 genrule(
   name = 'foo',
   outs = ["foo.txt"],
-  cmd = "echo \"foo bar\" > \$@",
+  cmd = "echo out && echo err 1>&2 && echo 'foo bar' > $@",
 )
 EOF
 
@@ -246,18 +246,18 @@ EOF
   remote_cas_files="$(count_remote_cas_files)"
   [[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote cas entries, not $remote_cas_files"
   disk_cas_files="$(count_disk_cas_files $cache_dir)"
-  # foo.txt, stdout and stderr for action 'foo'
-  [[ "$disk_cas_files" == 3 ]] || fail "Expected 3 disk cas entries, not $disk_cas_files"
+  # foo.txt, stdout, stderr, Command proto for action 'foo'
+  [[ "$disk_cas_files" == 4 ]] || fail "Expected 4 disk cas entries, not $disk_cas_files"
 }
 
 function test_upload_all_combined_cache() {
   local cache_dir="${TEST_TMPDIR}/disk_cache"
   mkdir -p a
-  cat > a/BUILD <<EOF
+  cat > a/BUILD <<'EOF'
 genrule(
   name = 'foo',
   outs = ["foo.txt"],
-  cmd = "echo \"foo bar\" > \$@",
+  cmd = "echo out && echo err 1>&2 && echo 'foo bar' > $@",
 )
 EOF
 
@@ -275,8 +275,8 @@ EOF
   remote_cas_files="$(count_remote_cas_files)"
   [[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote cas entries, not $remote_cas_files"
   disk_cas_files="$(count_disk_cas_files $cache_dir)"
-  # foo.txt, stdout and stderr for action 'foo'
-  [[ "$disk_cas_files" == 3 ]] || fail "Expected 3 disk cas entries, not $disk_cas_files"
+  # foo.txt, stdout, stderr, Command proto for action 'foo'
+  [[ "$disk_cas_files" == 4 ]] || fail "Expected 4 disk cas entries, not $disk_cas_files"
 }
 
 function test_upload_minimal_alias_action_doesnt_upload_missing_blobs() {

diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -1737,7 +1737,8 @@ EOF
   remote_ac_files="$(count_remote_ac_files)"
   [[ "$remote_ac_files" == 1 ]] || fail "Expected 1 remote action cache entries, not $remote_ac_files"
   remote_cas_files="$(count_remote_cas_files)"
-  [[ "$remote_cas_files" == 3 ]] || fail "Expected 3 remote cas entries, not $remote_cas_files"
+  # foo.txt + Command proto; stdout/stderr not uploaded because they're empty
+  [[ "$remote_cas_files" == 2 ]] || fail "Expected 2 remote cas entries, not $remote_cas_files"
 }
 
 function test_combined_cache_with_no_remote_cache_tag_remote_cache() {