Fix checking remote cache for omitted files in buildevent file

exoson · copybara-github · commit 317375dd7c98 · 2022-04-19T06:46:54.000-07:00
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 bazelbuild#15280. PiperOrigin-RevId: 442798312
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
@@ -112,12 +112,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() {
@@ -135,6 +137,10 @@ public boolean isDirectory() {
     public boolean isRemote() {
       return remote;
     }
+
+    public boolean isOmitted() {
+      return omitted;
+    }
   }
 
   /**
@@ -143,22 +149,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(xattrProvider, 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(
@@ -171,14 +184,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<List<PathMetadata>> queryRemoteCache(
@@ -187,7 +204,8 @@ private Single<List<PathMetadata>> queryRemoteCache(
     List<PathMetadata> filesToQuery = new ArrayList<>();
     Set<Digest> 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 {
@@ -244,7 +262,8 @@ private Single<List<PathMetadata>> uploadLocalFiles(
                                 path.getPath(),
                                 /*digest=*/ null,
                                 path.isDirectory(),
-                                path.isRemote()));
+                                path.isRemote(),
+                                path.isOmitted()));
                       });
             })
         .collect(Collectors.toList());
@@ -271,13 +290,17 @@ private Single<PathConverter> upload(Set<Path> 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);
   }
 
@@ -311,23 +334,25 @@ private static class PathConverterImpl implements PathConverter {
     private final Set<Path> skippedPaths;
     private final Set<Path> localPaths;
 
-    PathConverterImpl(
-        String remoteServerInstanceName, List<PathMetadata> uploads, Set<Path> localPaths) {
+    PathConverterImpl(String remoteServerInstanceName, List<PathMetadata> uploads) {
       Preconditions.checkNotNull(uploads);
       this.remoteServerInstanceName = remoteServerInstanceName;
       pathToDigest = new HashMap<>(uploads.size());
       ImmutableSet.Builder<Path> skippedPaths = ImmutableSet.builder();
+      ImmutableSet.Builder<Path> 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
@@ -3606,6 +3606,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 <<EOF
