Skip to content

Commit

Permalink
Stop uploading the Action proto to the CAS.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@tjgq @copybara-github
tjgq authored and copybara-github committed Sep 23, 2023
1 parent 92a6cdf commit 4482d6c
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 52 deletions.
39 changes: 19 additions & 20 deletions src/main/java/com/google/devtools/build/lib/remote/UploadManifest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()));
Expand All @@ -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);
}
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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();

Expand Down
5 changes: 2 additions & 3 deletions src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down
43 changes: 23 additions & 20 deletions src/test/java/com/google/devtools/build/lib/remote/UploadManifestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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()
Expand Down Expand Up @@ -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);
Expand All @@ -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()
Expand Down Expand Up @@ -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);
Expand All @@ -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()
Expand Down Expand Up @@ -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");
Expand All @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
16 changes: 8 additions & 8 deletions src/test/shell/bazel/remote/remote_build_event_uploader_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand All @@ -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() {
Expand Down
3 changes: 2 additions & 1 deletion src/test/shell/bazel/remote/remote_execution_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down

0 comments on commit 4482d6c

Please sign in to comment.