Skip to content

Commit

Permalink
Fix an issue that `incompatible_remote_build_event_upload_respect_no_… (
Browse files Browse the repository at this point in the history
#16045)

* 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>
  • Loading branch information
sgowroji and coeuvre authored Aug 4, 2022
1 parent 9d57003 commit 82452c7
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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. */
Expand Down Expand Up @@ -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;
}
}
Expand Down
23 changes: 23 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 82452c7

Please sign in to comment.