Always decide whether to scrub an input by its effective path.

For runfiles, the effective path differs from the one reported by the ActionInput, and could even be in a different configuration prefix (e.g., an input bazel-out/xxx/foo may appear in a runfiles tree at bazel-out/yyy/bar.runfiles/foo). This is a more general version of #20926. Fixes #20926. PiperOrigin-RevId: 609327629 Change-Id: I8ff2eef626835f012091045a7a4adad52877c7e4
bazelbuild · Feb 22, 2024 · 4832332 · 4832332
1 parent 7bc54e2
commit 4832332
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 47 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD
@@ -260,7 +260,7 @@ java_library(
     srcs = ["Scrubber.java"],
     deps = [
         "//src/main/java/com/google/devtools/build/lib/actions",
-        "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
+        "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/protobuf:remote_scrubbing_java_proto",
         "//third_party:guava",
         "//third_party:jsr305",

diff --git a/src/main/java/com/google/devtools/build/lib/remote/Scrubber.java b/src/main/java/com/google/devtools/build/lib/remote/Scrubber.java
@@ -19,11 +19,10 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Spawn;
-import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.remote.RemoteScrubbing.Config;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.protobuf.TextFormat;
 import java.io.BufferedReader;
 import java.io.IOException;
@@ -148,14 +147,10 @@ private boolean matches(Spawn spawn) {
           && kindPattern.matcher(kind).matches();
     }
 
-    /** Whether the given input should be omitted from the cache key. */
-    public boolean shouldOmitInput(ActionInput input) {
-      if (input.equals(VirtualActionInput.EMPTY_MARKER)) {
-        return false;
-      }
-      String execPath = input.getExecPathString();
+    /** Whether an input with the given exec-relative path should be omitted from the cache key. */
+    public boolean shouldOmitInput(PathFragment execPath) {
       for (Pattern pattern : omittedInputPatterns) {
-        if (pattern.matcher(execPath).matches()) {
+        if (pattern.matcher(execPath.getPathString()).matches()) {
           return true;
         }
       }

diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java
@@ -243,9 +243,7 @@ private static <T> int build(
       PathFragment path = e.getKey();
       T input = e.getValue();
 
-      if (scrubber != null
-          && input instanceof ActionInput
-          && scrubber.shouldOmitInput((ActionInput) input)) {
+      if (scrubber != null && scrubber.shouldOmitInput(path)) {
         continue;
       }
 

diff --git a/src/test/java/com/google/devtools/build/lib/remote/ScrubberTest.java b/src/test/java/com/google/devtools/build/lib/remote/ScrubberTest.java
@@ -16,12 +16,11 @@
 
 import static com.google.common.truth.Truth.assertThat;
 
-import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.Spawn;
-import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.exec.util.SpawnBuilder;
 import com.google.devtools.build.lib.remote.RemoteScrubbing.Config;
 import com.google.devtools.build.lib.remote.Scrubber.SpawnScrubber;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -216,7 +215,7 @@ public void noOmittedInputs() {
         new Scrubber(Config.newBuilder().addRules(Config.Rule.getDefaultInstance()).build())
             .forSpawn(createSpawn());
 
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("foo/bar"))).isFalse();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("foo/bar"))).isFalse();
   }
 
   @Test
@@ -231,10 +230,9 @@ public void exactOmittedInput() {
                     .build())
             .forSpawn(createSpawn());
 
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("foo/bar"))).isTrue();
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("foo/bar/baz"))).isFalse();
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("bazel-out/foo/bar")))
-        .isFalse();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("foo/bar"))).isTrue();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("foo/bar/baz"))).isFalse();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("bazel-out/foo/bar"))).isFalse();
   }
 
   @Test
@@ -249,10 +247,9 @@ public void wildcardOmittedInput() {
                     .build())
             .forSpawn(createSpawn());
 
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("foo/bar"))).isTrue();
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("foo/bar/baz"))).isTrue();
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("bazel-out/foo/bar")))
-        .isFalse();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("foo/bar"))).isTrue();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("foo/bar/baz"))).isTrue();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("bazel-out/foo/bar"))).isFalse();
   }
 
   @Test
@@ -269,29 +266,12 @@ public void multipleOmittedInputs() {
                     .build())
             .forSpawn(createSpawn());
 
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("foo/bar"))).isTrue();
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("spam/eggs"))).isTrue();
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("foo/bar/baz"))).isFalse();
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("bazel-out/foo/bar")))
-        .isFalse();
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("spam/eggs/bacon")))
-        .isFalse();
-    assertThat(spawnScrubber.shouldOmitInput(ActionInputHelper.fromPath("bazel-out/spam/eggs")))
-        .isFalse();
-  }
-
-  @Test
-  public void doNotScrubEmptyMarker() {
-    var spawnScrubber =
-        new Scrubber(
-                Config.newBuilder()
-                    .addRules(
-                        Config.Rule.newBuilder()
-                            .setTransform(Config.Transform.newBuilder().addOmittedInputs(".*")))
-                    .build())
-            .forSpawn(createSpawn());
-
-    assertThat(spawnScrubber.shouldOmitInput(VirtualActionInput.EMPTY_MARKER)).isFalse();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("foo/bar"))).isTrue();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("spam/eggs"))).isTrue();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("foo/bar/baz"))).isFalse();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("bazel-out/foo/bar"))).isFalse();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("spam/eggs/bacon"))).isFalse();
+    assertThat(spawnScrubber.shouldOmitInput(PathFragment.create("bazel-out/spam/eggs"))).isFalse();
   }
 
   @Test