From 2b83de53f19f6b834e04cf995103e9169e027743 Mon Sep 17 00:00:00 2001 From: Tyler Williams Date: Mon, 24 Jul 2023 22:09:22 -0700 Subject: [PATCH] Ensure namedSetOfFiles URIs specify blob type --- .../ByteStreamBuildEventArtifactUploader.java | 87 ++++++++++++++----- 1 file changed, 66 insertions(+), 21 deletions(-) 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; } } }