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 bazelbuild#20926. Fixes bazelbuild#20926. PiperOrigin-RevId: 609327629 Change-Id: I8ff2eef626835f012091045a7a4adad52877c7e4
uber-common · Jul 7, 2024 · 6c4f2bc · 6c4f2bc
1 parent 24a9ef4
commit 6c4f2bc
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 45 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
@@ -255,7 +255,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,10 +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;
@@ -142,10 +142,10 @@ private boolean matches(Spawn spawn) {
           && labelPattern.matcher(label).matches();
     }
 
-    /** Whether the given input should be omitted from the cache key. */
-    public boolean shouldOmitInput(String path) {
+    /** 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(path).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
@@ -262,11 +262,8 @@ private static <T> int build(
       PathFragment path = e.getKey();
       T input = e.getValue();
 
-      if (scrubber != null && input instanceof ActionInput) {
-        String inputPath = input.equals(VirtualActionInput.EMPTY_MARKER) ? path.getPathString() : ((ActionInput)input).getExecPathString();
-        if (scrubber.shouldOmitInput(inputPath)) {
-          continue;
-        }
+      if (scrubber != null && scrubber.shouldOmitInput(path)) {
+        continue;
       }
 
       if (input instanceof DerivedArtifact && ((DerivedArtifact) input).isTreeArtifact()) {

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;
@@ -164,7 +163,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
@@ -179,10 +178,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
@@ -197,10 +195,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
@@ -217,29 +214,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