From 82452c7c372fb28485b0b5e0a98b471648f0dfd0 Mon Sep 17 00:00:00 2001
From: Gowroji Sunil <48122892+sgowroji@users.noreply.github.com>
Date: Thu, 4 Aug 2022 19:05:24 +0530
Subject: [PATCH] =?UTF-8?q?Fix=20an=20issue=20that=20`incompatible=5Fremot?=
 =?UTF-8?q?e=5Fbuild=5Fevent=5Fupload=5Frespect=5Fno=5F=E2=80=A6=20(#16045?=
 =?UTF-8?q?)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Fix an issue that `incompatible_remote_build_event_upload_respect_no_…

…cache` doesn't work with BwtB.

The root cause is we use `Path` as key to check which files are omitted, but it also compares the underlying filesystem. When BwtB is enabled, the filesystem for the file is different so we failed to mark the file as omitted.

This change fixes that by using `PathFragment` as key.

Fixes #15527.

Closes #16023.

PiperOrigin-RevId: 465284879
Change-Id: I49bbd7a6055e0f234b4ac86a224886bd85797f71

* Update ByteStreamBuildEventArtifactUploader changes

Removing the import as it is throwing the error

Co-authored-by: Chi Wang <chiwang@google.com>
---
 .../ByteStreamBuildEventArtifactUploader.java | 16 +++++++------
 .../bazel/remote/remote_execution_test.sh     | 23 +++++++++++++++++++
 2 files changed, 32 insertions(+), 7 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 0e3b79cedf070f..15c12bbb1b093e 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
@@ -34,6 +34,7 @@
 import com.google.devtools.build.lib.remote.util.DigestUtil;
 import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import io.netty.util.AbstractReferenceCounted;
 import io.netty.util.ReferenceCounted;
 import io.reactivex.rxjava3.core.Flowable;
@@ -67,8 +68,8 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
   private final AtomicBoolean shutdown = new AtomicBoolean();
   private final Scheduler scheduler;
 
-  private final Set<Path> omittedFiles = Sets.newConcurrentHashSet();
-  private final Set<Path> omittedTreeRoots = Sets.newConcurrentHashSet();
+  private final Set<PathFragment> omittedFiles = Sets.newConcurrentHashSet();
+  private final Set<PathFragment> omittedTreeRoots = Sets.newConcurrentHashSet();
 
   ByteStreamBuildEventArtifactUploader(
       Executor executor,
@@ -89,11 +90,11 @@ class ByteStreamBuildEventArtifactUploader extends AbstractReferenceCounted
   }
 
   public void omitFile(Path file) {
-    omittedFiles.add(file);
+    omittedFiles.add(file.asFragment());
   }
 
   public void omitTree(Path treeRoot) {
-    omittedTreeRoots.add(treeRoot);
+    omittedTreeRoots.add(treeRoot.asFragment());
   }
 
   /** Returns {@code true} if Bazel knows that the file is stored on a remote system. */
@@ -153,13 +154,14 @@ private PathMetadata readPathMetadata(Path file) throws IOException {
           /* omitted= */ false);
     }
 
+    PathFragment filePathFragment = file.asFragment();
     boolean omitted = false;
-    if (omittedFiles.contains(file)) {
+    if (omittedFiles.contains(filePathFragment)) {
       omitted = true;
     } else {
-      for (Path treeRoot : omittedTreeRoots) {
+      for (PathFragment treeRoot : omittedTreeRoots) {
         if (file.startsWith(treeRoot)) {
-          omittedFiles.add(file);
+          omittedFiles.add(filePathFragment);
           omitted = true;
         }
       }
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index f45f93dbb2f361..e79b2dfe4f4d49 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -3536,6 +3536,29 @@ EOF
   expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
 }
 
+function test_uploader_respect_no_cache_minimal() {
+  mkdir -p a
+  cat > a/BUILD <<EOF
+genrule(
+  name = 'foo',
+  outs = ["foo.txt"],
+  cmd = "echo \"foo bar\" > \$@",
+  tags = ["no-cache"],
+)
+EOF
+
+  bazel build \
+      --remote_cache=grpc://localhost:${worker_port} \
+      --remote_download_minimal \
+      --incompatible_remote_build_event_upload_respect_no_cache \
+      --build_event_json_file=bep.json \
+      //a:foo >& $TEST_log || fail "Failed to build"
+
+  cat bep.json > $TEST_log
+  expect_not_log "a:foo.*bytestream://" || fail "local files are converted"
+  expect_log "command.profile.gz.*bytestream://" || fail "should upload profile data"
+}
+
 function test_uploader_alias_action_respect_no_cache() {
   mkdir -p a
   cat > a/BUILD <<EOF