From ad74d5243917bb27a37e38d151a4a3c8a49947eb Mon Sep 17 00:00:00 2001 From: Brentley Jones Date: Mon, 9 May 2022 11:43:45 -0500 Subject: [PATCH] Fix checking remote cache for omitted files in buildevent file (#15405) Enabling both `--noremote_upload_local_results` and `--incompatible_remote_build_event_upload_respect_no_cache` caused ByteStreamBuildEventArtifactuploader not to check if some files already existed remotely because local files were not to be uploaded. When using remote execution some of these files might exist remotely, so we want to check remote cache for those files even if we would not upload them if missing. The test case included depicts this failure case. Closes #15280. PiperOrigin-RevId: 442798312 (cherry picked from commit 317375dd7c98ac9a6c83913089d256e832165618) Co-authored-by: Emil Kattainen --- .../ByteStreamBuildEventArtifactUploader.java | 65 +++++++++++++------ .../bazel/remote/remote_execution_test.sh | 29 +++++++++ 2 files changed, 74 insertions(+), 20 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 7c16c2973542f0..ae57825d11a2bb 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 @@ -107,12 +107,14 @@ private static final class PathMetadata { private final Digest digest; private final boolean directory; private final boolean remote; + private final boolean omitted; - PathMetadata(Path path, Digest digest, boolean directory, boolean remote) { + PathMetadata(Path path, Digest digest, boolean directory, boolean remote, boolean omitted) { this.path = path; this.digest = digest; this.directory = directory; this.remote = remote; + this.omitted = omitted; } public Path getPath() { @@ -130,6 +132,10 @@ public boolean isDirectory() { public boolean isRemote() { return remote; } + + public boolean isOmitted() { + return omitted; + } } /** @@ -138,22 +144,29 @@ public boolean isRemote() { */ private PathMetadata readPathMetadata(Path file) throws IOException { if (file.isDirectory()) { - return new PathMetadata(file, /* digest= */ null, /* directory= */ true, /* remote= */ false); - } - if (omittedFiles.contains(file)) { - return new PathMetadata(file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false); + return new PathMetadata( + file, + /* digest= */ null, + /* directory= */ true, + /* remote= */ false, + /* omitted= */ false); } - for (Path treeRoot : omittedTreeRoots) { - if (file.startsWith(treeRoot)) { - omittedFiles.add(file); - return new PathMetadata(file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false); + boolean omitted = false; + if (omittedFiles.contains(file)) { + omitted = true; + } else { + for (Path treeRoot : omittedTreeRoots) { + if (file.startsWith(treeRoot)) { + omittedFiles.add(file); + omitted = true; + } } } DigestUtil digestUtil = new DigestUtil(file.getFileSystem().getDigestFunction()); Digest digest = digestUtil.compute(file); - return new PathMetadata(file, digest, /* directory= */ false, isRemoteFile(file)); + return new PathMetadata(file, digest, /* directory= */ false, isRemoteFile(file), omitted); } private static void processQueryResult( @@ -166,14 +179,18 @@ private static void processQueryResult( } else { PathMetadata remotePathMetadata = new PathMetadata( - file.getPath(), file.getDigest(), file.isDirectory(), /* remote= */ true); + file.getPath(), + file.getDigest(), + file.isDirectory(), + /* remote= */ true, + file.isOmitted()); knownRemotePaths.add(remotePathMetadata); } } } private static boolean shouldUpload(PathMetadata path) { - return path.getDigest() != null && !path.isRemote() && !path.isDirectory(); + return path.getDigest() != null && !path.isRemote() && !path.isDirectory() && !path.isOmitted(); } private Single> queryRemoteCache( @@ -182,7 +199,8 @@ private Single> queryRemoteCache( List filesToQuery = new ArrayList<>(); Set digestsToQuery = new HashSet<>(); for (PathMetadata path : paths) { - if (shouldUpload(path)) { + // Query remote cache for files even if omitted from uploading + if (shouldUpload(path) || path.isOmitted()) { filesToQuery.add(path); digestsToQuery.add(path.getDigest()); } else { @@ -239,7 +257,8 @@ private Single> uploadLocalFiles( path.getPath(), /*digest=*/ null, path.isDirectory(), - path.isRemote())); + path.isRemote(), + path.isOmitted())); }); }) .collect(Collectors.toList()); @@ -265,13 +284,17 @@ private Single upload(Set files) { } catch (IOException e) { reporterUploadError(e); return new PathMetadata( - file, /*digest=*/ null, /*directory=*/ false, /*remote=*/ false); + file, + /*digest=*/ null, + /*directory=*/ false, + /*remote=*/ false, + /*omitted=*/ false); } }) .collect(Collectors.toList()) .flatMap(paths -> queryRemoteCache(remoteCache, context, paths)) .flatMap(paths -> uploadLocalFiles(remoteCache, context, paths)) - .map(paths -> new PathConverterImpl(remoteServerInstanceName, paths, omittedFiles)), + .map(paths -> new PathConverterImpl(remoteServerInstanceName, paths)), RemoteCache::release); } @@ -305,23 +328,25 @@ private static class PathConverterImpl implements PathConverter { private final Set skippedPaths; private final Set localPaths; - PathConverterImpl( - String remoteServerInstanceName, List uploads, Set localPaths) { + PathConverterImpl(String remoteServerInstanceName, List uploads) { Preconditions.checkNotNull(uploads); this.remoteServerInstanceName = remoteServerInstanceName; pathToDigest = new HashMap<>(uploads.size()); ImmutableSet.Builder skippedPaths = ImmutableSet.builder(); + ImmutableSet.Builder localPaths = ImmutableSet.builder(); for (PathMetadata pair : uploads) { Path path = pair.getPath(); Digest digest = pair.getDigest(); - if (digest != null) { + if (!pair.isRemote() && pair.isOmitted()) { + localPaths.add(path); + } else if (digest != null) { pathToDigest.put(path, digest); } else { skippedPaths.add(path); } } this.skippedPaths = skippedPaths.build(); - this.localPaths = localPaths; + this.localPaths = localPaths.build(); } @Override diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index c14f7b2a6c023a..d84afa55b4c5ea 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3498,6 +3498,35 @@ EOF [[ "$remote_cas_files" == 1 ]] || fail "Expected 1 remote cas entries, not $remote_cas_files" } +function test_remote_exec_convert_remote_file() { + mkdir -p a + cat > a/BUILD <<'EOF' +sh_test( + name = "test", + srcs = ["test.sh"], +) +EOF + cat > a/test.sh <<'EOF' +#!/bin/bash +set -e +echo \"foo\" +exit 0 +EOF + chmod 755 a/test.sh + + bazel test \ + --remote_executor=grpc://localhost:${worker_port} \ + --build_event_json_file=bep.json \ + --noremote_upload_local_results \ + --incompatible_remote_build_event_upload_respect_no_cache \ + //a:test >& $TEST_log || "Failed to test //a:test" + + cat bep.json > $TEST_log + expect_not_log "test\.log.*file://" || fail "remote files generated in remote execution are not converted" + expect_log "test\.log.*bytestream://" || fail "remote files generated in remote execution are not converted" +} + + function test_uploader_ignore_disk_cache_of_combined_cache() { mkdir -p a cat > a/BUILD <