From 49a01c46ea6abf1e92defeec197e059f80d28b01 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 22 Feb 2024 04:40:11 -0800 Subject: [PATCH] [7.1.0] 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 https://github.com/bazelbuild/bazel/pull/20926. Fixes https://github.com/bazelbuild/bazel/pull/20926. PiperOrigin-RevId: 609327629 Change-Id: I8ff2eef626835f012091045a7a4adad52877c7e4 --- .../google/devtools/build/lib/remote/BUILD | 2 +- .../devtools/build/lib/remote/Scrubber.java | 13 ++--- .../merkletree/DirectoryTreeBuilder.java | 4 +- .../build/lib/remote/ScrubberTest.java | 48 ++++++------------- 4 files changed, 20 insertions(+), 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 index 890f3e5dc24443..91fa5cde4a2729 100644 --- 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 index 886a681129935a..0b88edaddf34f8 100644 --- 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 index baaf104d7c6a34..22d352594d8dbe 100644 --- 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,9 +262,7 @@ private static 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 index f8f3cd8d71c403..383f3e31886a11 100644 --- 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