Remove unnecessary tolerance for duplicate fileset entries.

Fileset symlinks are already deduplicated by `SkyframeFilesetManifestAction`, so we can use a preconditions check instead of tolerating them. PiperOrigin-RevId: 679795852 Change-Id: I3031de779731254e069d1e801010bcf595520b20
bazelbuild · Sep 28, 2024 · dfba96d · dfba96d
1 parent 32264b0
commit dfba96d
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 26 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetManifest.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetManifest.java
@@ -13,7 +13,9 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions;
 
-import com.google.common.base.Preconditions;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.common.base.Strings;
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
@@ -109,8 +111,12 @@ public static FilesetManifest constructFilesetManifest(
       String artifact = Strings.emptyToNull(outputSymlink.getTargetPath().getPathString());
       if (isRelativeSymlink(outputSymlink)) {
         addRelativeSymlinkEntry(artifact, fullLocation, relSymlinkBehavior, relativeLinks);
-      } else if (!entries.containsKey(fullLocation)) { // Keep consistent behavior: no overwriting.
-        entries.put(fullLocation, artifact);
+      } else {
+        // Symlinks are already deduplicated by name in SkyframeFilesetManifestAction.
+        checkState(
+            entries.put(fullLocation, artifact) == null,
+            "Duplicate fileset entry at %s",
+            fullLocation);
       }
       if (outputSymlink.getMetadata() instanceof FileArtifactValue) {
         artifactValues.put(artifact, (FileArtifactValue) outputSymlink.getMetadata());
@@ -135,11 +141,11 @@ private static void addRelativeSymlinkEntry(
       throws ForbiddenRelativeSymlinkException {
     switch (relSymlinkBehavior) {
       case ERROR -> throw new ForbiddenRelativeSymlinkException(artifact);
-      case RESOLVE, RESOLVE_FULLY -> {
-        if (!relativeLinks.containsKey(fullLocation)) { // Keep consistent behavior: no overwriting.
-          relativeLinks.put(fullLocation, artifact);
-        }
-      }
+      case RESOLVE, RESOLVE_FULLY ->
+          checkState(
+              relativeLinks.put(fullLocation, artifact) == null,
+              "Duplicate fileset entry at %s",
+              fullLocation);
       case IGNORE -> {
         // Do nothing.
       }
@@ -171,8 +177,8 @@ private static void fullyResolveRelativeSymlinks(
       for (Map.Entry<PathFragment, String> e : relativeLinks.entrySet()) {
         PathFragment location = e.getKey();
         Path locationPath = fs.getPath("/").getRelative(location);
-        PathFragment value = PathFragment.create(Preconditions.checkNotNull(e.getValue(), e));
-        Preconditions.checkState(!value.isAbsolute(), e);
+        PathFragment value = PathFragment.create(checkNotNull(e.getValue(), e));
+        checkState(!value.isAbsolute(), e);
 
         locationPath.getParentDirectory().createDirectoryAndParents();
         locationPath.createSymbolicLink(value);
@@ -219,8 +225,8 @@ private static void resolveRelativeSymlinks(
       for (Map.Entry<PathFragment, String> e : relativeLinks.entrySet()) {
         PathFragment location = e.getKey();
         String value = e.getValue();
-        String actual = Preconditions.checkNotNull(value, e);
-        Preconditions.checkState(!actual.startsWith("/"), e);
+        String actual = checkNotNull(value, e);
+        checkState(!actual.startsWith("/"), e);
         PathFragment actualLocation = location;
 
         // Recursively resolve relative symlinks.

diff --git a/src/test/java/com/google/devtools/build/lib/exec/FilesetManifestTest.java b/src/test/java/com/google/devtools/build/lib/exec/FilesetManifestTest.java
@@ -144,20 +144,6 @@ public void testManifestWithExecRootRelativePath() throws Exception {
       assertThat(manifest.getEntries())
           .containsExactly(PathFragment.create("out/foo/bar"), "foo/bar");
     }
-
-    /** Current behavior is first one wins. */
-    @Test
-    public void testDefactoBehaviorWithDuplicateEntries() throws Exception {
-      List<FilesetOutputSymlink> symlinks =
-          ImmutableList.of(filesetSymlink("bar", "/foo/bar"), filesetSymlink("bar", "/baz"));
-
-      FilesetManifest manifest =
-          FilesetManifest.constructFilesetManifest(
-              symlinks, PathFragment.create("out/foo"), behavior);
-
-      assertThat(manifest.getEntries())
-          .containsExactly(PathFragment.create("out/foo/bar"), "/foo/bar");
-    }
   }
 
   /** Manifest tests that apply to a specific relative symlink behavior. */