diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java index 2b4fe613c98787..ecb6899db7e1e0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java @@ -19,7 +19,9 @@ import static com.google.devtools.build.lib.remote.util.Utils.grpcAwareErrorMessage; import build.bazel.remote.execution.v2.Digest; +import build.bazel.remote.execution.v2.DigestFunction; import build.bazel.remote.execution.v2.RequestMetadata; +import com.google.common.base.Ascii; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; @@ -135,6 +137,7 @@ private static final class PathMetadata { private final boolean symlink; private final boolean remote; private final boolean omitted; + private final DigestFunction.Value digestFunction; PathMetadata( Path path, @@ -142,13 +145,15 @@ private static final class PathMetadata { boolean directory, boolean symlink, boolean remote, - boolean omitted) { + boolean omitted, + DigestFunction.Value digestFunction) { this.path = path; this.digest = digest; this.directory = directory; this.symlink = symlink; this.remote = remote; this.omitted = omitted; + this.digestFunction = digestFunction; } public Path getPath() { @@ -174,6 +179,10 @@ public boolean isRemote() { public boolean isOmitted() { return omitted; } + + public DigestFunction.Value getDigestFunction() { + return digestFunction; + } } /** @@ -181,6 +190,8 @@ public boolean isOmitted() { * might do I/O. */ private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOException { + DigestUtil digestUtil = new DigestUtil(xattrProvider, path.getFileSystem().getDigestFunction()); + if (file.type == LocalFileType.OUTPUT_DIRECTORY || (file.type == LocalFileType.OUTPUT && path.isDirectory())) { return new PathMetadata( @@ -189,7 +200,8 @@ private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOExcept /* directory= */ true, /* symlink= */ false, /* remote= */ false, - /* omitted= */ false); + /* omitted= */ false, + /* digestFunction= */ digestUtil.getDigestFunction()); } if (file.type == LocalFileType.OUTPUT_SYMLINK) { return new PathMetadata( @@ -198,7 +210,8 @@ private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOExcept /* directory= */ false, /* symlink= */ true, /* remote= */ false, - /* omitted= */ false); + /* omitted= */ false, + /* digestFunction= */ digestUtil.getDigestFunction()); } PathFragment filePathFragment = path.asFragment(); @@ -214,10 +227,15 @@ private PathMetadata readPathMetadata(Path path, LocalFile file) throws IOExcept } } - DigestUtil digestUtil = new DigestUtil(xattrProvider, path.getFileSystem().getDigestFunction()); Digest digest = digestUtil.compute(path); return new PathMetadata( - path, digest, /* directory= */ false, /* symlink= */ false, isRemoteFile(path), omitted); + path, + digest, + /* directory= */ false, + /* symlink= */ false, + isRemoteFile(path), + omitted, + digestUtil.getDigestFunction()); } private static void processQueryResult( @@ -235,7 +253,8 @@ private static void processQueryResult( file.isDirectory(), file.isSymlink(), /* remote= */ true, - file.isOmitted()); + file.isOmitted(), + file.getDigestFunction()); knownRemotePaths.add(remotePathMetadata); } } @@ -336,7 +355,8 @@ private Single> uploadLocalFiles( // set remote to true so the PathConverter will use bytestream:// // scheme to convert the URI for this file /* remote= */ true, - path.isOmitted())) + path.isOmitted(), + path.getDigestFunction())) .onErrorResumeNext( error -> { reportUploadError(error, path.getPath(), path.getDigest()); @@ -375,7 +395,8 @@ private Single doUpload(Map files) { /* directory= */ false, /* symlink= */ false, /* remote= */ false, - /* omitted= */ false); + /* omitted= */ false, + DigestFunction.Value.SHA256); } }) .collect(Collectors.toList()) @@ -419,7 +440,7 @@ public ReferenceCounted touch(Object o) { private static class PathConverterImpl implements PathConverter { private final String remoteServerInstanceName; - private final Map pathToDigest; + private final Map pathToMetadata; private final Set skippedPaths; private final Set localPaths; @@ -429,18 +450,18 @@ private static class PathConverterImpl implements PathConverter { RemoteBuildEventUploadMode remoteBuildEventUploadMode) { Preconditions.checkNotNull(uploads); this.remoteServerInstanceName = remoteServerInstanceName; - pathToDigest = Maps.newHashMapWithExpectedSize(uploads.size()); + pathToMetadata = Maps.newHashMapWithExpectedSize(uploads.size()); ImmutableSet.Builder skippedPaths = ImmutableSet.builder(); ImmutableSet.Builder localPaths = ImmutableSet.builder(); - for (PathMetadata pair : uploads) { - Path path = pair.getPath(); - Digest digest = pair.getDigest(); + for (PathMetadata metadata : uploads) { + Path path = metadata.getPath(); + Digest digest = metadata.getDigest(); if (digest != null) { // Always use bytestream:// in MINIMAL mode if (remoteBuildEventUploadMode == RemoteBuildEventUploadMode.MINIMAL) { - pathToDigest.put(path, digest); - } else if (pair.isRemote()) { - pathToDigest.put(path, digest); + pathToMetadata.put(path, metadata); + } else if (metadata.isRemote()) { + pathToMetadata.put(path, metadata); } else { localPaths.add(path); } @@ -452,6 +473,14 @@ private static class PathConverterImpl implements PathConverter { this.localPaths = localPaths.build(); } + private static boolean isOldStyleDigestFunction(DigestFunction.Value digestFunction) { + // Old-style digest functions (SHA256, etc) are distinguishable by the length + // of their hash alone and do not require extra specification, but newer + // digest functions (which may have the same length hashes as the older + // functions!) must be explicitly specified in the upload resource name. + return digestFunction.getNumber() <= 7; + } + @Override @Nullable public String apply(Path path) { @@ -461,8 +490,8 @@ public String apply(Path path) { return String.format("file://%s", path.getPathString()); } - Digest digest = pathToDigest.get(path); - if (digest == null) { + PathMetadata metadata = pathToMetadata.get(path); + if (metadata == null) { if (skippedPaths.contains(path)) { return null; } @@ -470,9 +499,25 @@ public String apply(Path path) { throw new IllegalStateException( String.format("Illegal file reference: '%s'", path.getPathString())); } - return String.format( - "bytestream://%s/blobs/%s/%d", - remoteServerInstanceName, digest.getHash(), digest.getSizeBytes()); + + Digest digest = metadata.getDigest(); + DigestFunction.Value digestFunction = metadata.getDigestFunction(); + String out; + if (isOldStyleDigestFunction(digestFunction)) { + out = + String.format( + "bytestream://%s/blobs/%s/%d", + remoteServerInstanceName, digest.getHash(), digest.getSizeBytes()); + } else { + out = + String.format( + "bytestream://%s/blobs/%s/%s/%d", + remoteServerInstanceName, + Ascii.toLowerCase(digestFunction.getValueDescriptor().getName()), + digest.getHash(), + digest.getSizeBytes()); + } + return out; } } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 874d5a7500e8b9..3c26fab935f559 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -94,6 +94,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/util/io", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/main/java/com/google/devtools/build/lib/vfs/bazel", "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", "//src/main/java/com/google/devtools/common/options", "//src/main/protobuf:failure_details_java_proto", diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index 5323e4318a0b85..cc1a93a9047248 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assume.assumeNotNull; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; @@ -66,6 +67,7 @@ import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.SyscallCache; +import com.google.devtools.build.lib.vfs.bazel.BazelHashFunctions; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.common.options.Options; import io.grpc.Server; @@ -177,7 +179,7 @@ public void uploadsShouldWork() throws Exception { filesToUpload.put( file, new LocalFile( - file, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null)); + file, LocalFileType.OUTPUT, /* artifact= */ null, /* artifactMetadata= */ null)); } serviceRegistry.addService(new MaybeFailOnceUploadService(blobsByHash)); @@ -220,7 +222,7 @@ public void uploadsShouldWork_fewerPermitsThanUploads() throws Exception { filesToUpload.put( file, new LocalFile( - file, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null)); + file, LocalFileType.OUTPUT, /* artifact= */ null, /* artifactMetadata= */ null)); } serviceRegistry.addService(new MaybeFailOnceUploadService(blobsByHash)); @@ -310,6 +312,33 @@ public void testUnknown_uploadedIfFile() throws Exception { artifactUploader.release(); } + @Test + public void testUnknown_uploadedIfFileBlake3() throws Exception { + assumeNotNull(BazelHashFunctions.BLAKE3); + + FileSystem fs = new InMemoryFileSystem(new JavaClock(), BazelHashFunctions.BLAKE3); + Path file = fs.getPath("/file"); + file.getOutputStream().close(); + Map filesToUpload = new HashMap<>(); + filesToUpload.put( + file, + new LocalFile( + file, LocalFileType.OUTPUT, /* artifact= */ null, /* artifactMetadata= */ null)); + RemoteRetrier retrier = + TestUtils.newRemoteRetrier(() -> new FixedBackoff(1, 0), (e) -> true, retryService); + ReferenceCountedChannel refCntChannel = new ReferenceCountedChannel(channelConnectionFactory); + RemoteCache remoteCache = newRemoteCache(refCntChannel, retrier); + ByteStreamBuildEventArtifactUploader artifactUploader = newArtifactUploader(remoteCache); + + PathConverter pathConverter = artifactUploader.upload(filesToUpload).get(); + String hash = BaseEncoding.base16().lowerCase().encode(file.getDigest()); + long size = file.getFileSize(); + String conversion = pathConverter.apply(file); + assertThat(conversion) + .isEqualTo("bytestream://localhost/instance/blobs/blake3/" + hash + "/" + size); + artifactUploader.release(); + } + @Test public void testUnknown_notUploadedIfDirectory() throws Exception { Path dir = fs.getPath("/dir"); @@ -352,7 +381,7 @@ public void someUploadsFail_succeedsWithWarningMessages() throws Exception { filesToUpload.put( file, new LocalFile( - file, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null)); + file, LocalFileType.OUTPUT, /* artifact= */ null, /* artifactMetadata= */ null)); } String hashOfBlobThatShouldFail = blobsByHash.keySet().iterator().next().toString(); serviceRegistry.addService( @@ -437,7 +466,7 @@ public void remoteFileShouldNotBeUploaded_actionFs() throws Exception { assertThat(remotePath.getFileSystem()).isEqualTo(remoteFs); LocalFile file = new LocalFile( - remotePath, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null); + remotePath, LocalFileType.OUTPUT, /* artifact= */ null, /* artifactMetadata= */ null); // act @@ -488,10 +517,16 @@ public void remoteFileShouldNotBeUploaded_findMissingDigests() throws Exception ImmutableMap.of( remoteFile, new LocalFile( - remoteFile, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null), + remoteFile, + LocalFileType.OUTPUT, + /* artifact= */ null, + /* artifactMetadata= */ null), localFile, new LocalFile( - localFile, LocalFileType.OUTPUT, /*artifact=*/ null, /*artifactMetadata=*/ null)); + localFile, + LocalFileType.OUTPUT, + /* artifact= */ null, + /* artifactMetadata= */ null)); PathConverter pathConverter = artifactUploader.upload(files).get(); // assert @@ -549,11 +584,11 @@ private ByteStreamBuildEventArtifactUploader newArtifactUploader(RemoteCache rem return new ByteStreamBuildEventArtifactUploader( MoreExecutors.directExecutor(), reporter, - /*verboseFailures=*/ true, + /* verboseFailures= */ true, remoteCache, - /*remoteServerInstanceName=*/ "localhost/instance", - /*buildRequestId=*/ "none", - /*commandId=*/ "none", + /* remoteServerInstanceName= */ "localhost/instance", + /* buildRequestId= */ "none", + /* commandId= */ "none", SyscallCache.NO_CACHE, RemoteBuildEventUploadMode.ALL); }