Remove the "overwrote runfile" warning.

At HEAD, this is reported on the console as an error but doesn't actually cause a build error and this has been the case for a long time. This means that this particular cat is out of the bag and we can't prohibit this behavior anymore and therefore it's better to not emit spam.

RELNOTES: None.
PiperOrigin-RevId: 720983277
Change-Id: I2f62247b7bb7a81e30281ab578a8b93f17cef476

Author: lberki

src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java

@@ -51,7 +51,6 @@
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 import java.util.UUID;
 import java.util.function.BiConsumer;
@@ -356,7 +355,7 @@ public BiConsumer<ConflictType, String> eventRunfilesConflictReceiver(
       EventKind kind =
           switch (conflictType) {
             case NESTED_RUNFILES_TREE -> EventKind.ERROR;
-            case ROOT_SYMLINK, PREFIX_CONFLICT ->
+            case PREFIX_CONFLICT ->
                 conflictPolicy == ConflictPolicy.ERROR ? EventKind.ERROR : EventKind.WARNING;
             default ->
                 switch (conflictPolicy) {
@@ -386,7 +385,6 @@ public Map<PathFragment, Artifact> getRunfilesInputs(
         conflictPolicy == ConflictPolicy.IGNORE
             ? EnumSet.of(
                 ConflictType.NESTED_RUNFILES_TREE,
-                ConflictType.ROOT_SYMLINK,
                 ConflictType.PREFIX_CONFLICT)
             : EnumSet.allOf(ConflictType.class);
 
@@ -401,7 +399,7 @@ private Map<PathFragment, Artifact> getRunfilesInputs(
     Map<PathFragment, Artifact> manifest = getSymlinksAsMap(checker);
     // Add artifacts (committed to inclusion on construction of runfiles).
     for (Artifact artifact : artifacts.toList()) {
-      checker.put(manifest, artifact.getRunfilesPath(), artifact, ConflictType.OTHER);
+      checker.put(manifest, artifact.getRunfilesPath(), artifact);
     }
 
     manifest =
@@ -412,7 +410,7 @@ private Map<PathFragment, Artifact> getRunfilesInputs(
 
     // TODO(bazel-team): Create /dev/null-like Artifact to avoid nulls?
     for (PathFragment extraPath : emptyFilesSupplier.getExtraPaths(manifest.keySet())) {
-      checker.put(manifest, extraPath, null, ConflictType.OTHER);
+      checker.put(manifest, extraPath, null);
     }
 
     // Copy manifest map to another manifest map, prepending the workspace name to every path.
@@ -423,11 +421,7 @@ private Map<PathFragment, Artifact> getRunfilesInputs(
     builder.addUnderWorkspace(manifest, checker);
     builder.addRootSymlinks(getRootSymlinksAsMap(checker), checker);
     if (repoMappingManifest != null) {
-      checker.put(
-          builder.manifest,
-          REPO_MAPPING_PATH_FRAGMENT,
-          repoMappingManifest,
-          ConflictType.ROOT_SYMLINK);
+      checker.put(builder.manifest, REPO_MAPPING_PATH_FRAGMENT, repoMappingManifest);
     }
     return builder.build();
   }
@@ -463,15 +457,13 @@ void addUnderWorkspace(Map<PathFragment, Artifact> inputManifest, ConflictChecke
         PathFragment path = entry.getKey();
         if (isUnderWorkspace(path)) {
           sawWorkspaceName = true;
-          checker.put(
-              manifest, workspaceName.getRelative(path), entry.getValue(), ConflictType.OTHER);
+          checker.put(manifest, workspaceName.getRelative(path), entry.getValue());
         } else {
           if (legacyExternalRunfiles) {
-            checker.put(
-                manifest, getLegacyExternalPath(path), entry.getValue(), ConflictType.OTHER);
+            checker.put(manifest, getLegacyExternalPath(path), entry.getValue());
           }
           // Always add the non-legacy .runfiles/repo/whatever path.
-          checker.put(manifest, getExternalPath(path), entry.getValue(), ConflictType.OTHER);
+          checker.put(manifest, getExternalPath(path), entry.getValue());
         }
       }
     }
@@ -480,11 +472,7 @@ void addUnderWorkspace(Map<PathFragment, Artifact> inputManifest, ConflictChecke
     public void addRootSymlinks(
         Map<PathFragment, Artifact> inputManifest, ConflictChecker checker) {
       for (Map.Entry<PathFragment, Artifact> entry : inputManifest.entrySet()) {
-        checker.put(
-            manifest,
-            checkForWorkspace(entry.getKey()),
-            entry.getValue(),
-            ConflictType.ROOT_SYMLINK);
+        checker.put(manifest, checkForWorkspace(entry.getKey()), entry.getValue());
       }
     }
 
@@ -606,7 +594,7 @@ private static Map<PathFragment, Artifact> entriesToMap(
     Map<PathFragment, Artifact> map = new LinkedHashMap<>();
     for (SymlinkEntry entry : entrySet.toList()) {
       // ConflictType does not matter, we ignore conflicts here
-      checker.put(map, entry.getPath(), entry.getArtifact(), ConflictType.OTHER);
+      checker.put(map, entry.getPath(), entry.getArtifact());
     }
     return map;
   }
@@ -627,8 +615,6 @@ public Runfiles setConflictPolicy(ConflictPolicy conflictPolicy) {
   public enum ConflictType {
     NESTED_RUNFILES_TREE, // A runfiles tree artifact in a runfiles tree
     PREFIX_CONFLICT, // An entry is the prefix of another
-    ROOT_SYMLINK, // A root symlink conflicts with something else
-    OTHER // Everything else
   };
 
   /** Checks for conflicts between entries in a runfiles tree while putting them in a map. */
@@ -655,11 +641,7 @@ public ConflictChecker(
      * @param path Path fragment to use as key in map.
      * @param artifact Artifact to store in map. This may be null to indicate an empty file.
      */
-    void put(
-        Map<PathFragment, Artifact> map,
-        PathFragment path,
-        Artifact artifact,
-        ConflictType conflictType) {
+    void put(Map<PathFragment, Artifact> map, PathFragment path, Artifact artifact) {
       if (artifact != null && artifact.isMiddlemanArtifact()) {
         if (conflictsToReport.contains(ConflictType.NESTED_RUNFILES_TREE)) {
           receiver.accept(
@@ -669,23 +651,6 @@ void put(
         return;
       }
 
-      if (map.containsKey(path) && conflictsToReport.contains(conflictType)) {
-        // Previous and new entry might have value of null
-        Artifact previous = map.get(path);
-        if (!Objects.equals(previous, artifact)) {
-          String previousStr =
-              (previous == null) ? "empty file" : previous.getExecPath().toString();
-          String artifactStr =
-              (artifact == null) ? "empty file" : artifact.getExecPath().toString();
-          if (!previousStr.equals(artifactStr)) {
-            String message =
-                String.format(
-                    "overwrote runfile %s, was symlink to %s, now symlink to %s",
-                    path.getSafePathString(), previousStr, artifactStr);
-            receiver.accept(conflictType, message);
-          }
-        }
-      }
       map.put(path, artifact);
     }
   }

src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java

@@ -175,22 +175,6 @@ public void testFilterListForObscuringSymlinksNoObscurers() {
     assertNoEvents();
   }
 
-  private void checkConflictWarning() {
-    assertContainsEvent("overwrote runfile");
-    assertWithMessage("ConflictChecker.put should have warned once")
-        .that(eventCollector.count())
-        .isEqualTo(1);
-    assertThat(Iterables.getOnlyElement(eventCollector).getKind()).isEqualTo(EventKind.WARNING);
-  }
-
-  private void checkConflictError() {
-    assertContainsEvent("overwrote runfile");
-    assertWithMessage("ConflictChecker.put should have errored once")
-        .that(eventCollector.count())
-        .isEqualTo(1);
-    assertThat(Iterables.getOnlyElement(eventCollector).getKind()).isEqualTo(EventKind.ERROR);
-  }
-
   private static final class SimpleActionLookupKey implements ActionLookupKey {
     private final String name;
 
@@ -217,7 +201,7 @@ public BuildConfigurationKey getConfigurationKey() {
   }
 
   @Test
-  public void testPutDerivedArtifactWithDifferentOwnerDoesNotConflict() throws Exception {
+  public void testPutDerivedArtifactWithDifferentOwner() throws Exception {
     ArtifactRoot root =
         ArtifactRoot.asDerivedRoot(scratch.dir("/workspace"), RootType.Output, "out");
     PathFragment path = PathFragment.create("src/foo.cc");
@@ -230,36 +214,13 @@ public void testPutDerivedArtifactWithDifferentOwnerDoesNotConflict() throws Exc
     Map<PathFragment, Artifact> map = new LinkedHashMap<>();
 
     Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.WARN);
-    checker.put(map, path, artifact1, ConflictType.OTHER);
+    checker.put(map, path, artifact1);
     assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(path, artifact1));
-    checker.put(map, path, artifact2, ConflictType.OTHER);
+    checker.put(map, path, artifact2);
     assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(path, artifact2));
     assertNoEvents();
   }
 
-  @Test
-  public void testPutDerivedArtifactWithDifferentPathConflicts() throws Exception {
-    ArtifactRoot root =
-        ArtifactRoot.asDerivedRoot(scratch.dir("/workspace"), RootType.Output, "out");
-    PathFragment path = PathFragment.create("src/foo.cc");
-    PathFragment path2 = PathFragment.create("src/bar.cc");
-
-    SimpleActionLookupKey owner1 = new SimpleActionLookupKey("//owner1");
-    SimpleActionLookupKey owner2 = new SimpleActionLookupKey("//owner2");
-    Artifact artifact1 = DerivedArtifact.create(root, root.getExecPath().getRelative(path), owner1);
-    Artifact artifact2 =
-        DerivedArtifact.create(root, root.getExecPath().getRelative(path2), owner2);
-
-    Map<PathFragment, Artifact> map = new LinkedHashMap<>();
-
-    Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.WARN);
-    checker.put(map, path, artifact1, ConflictType.OTHER);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(path, artifact1));
-    checker.put(map, path, artifact2, ConflictType.OTHER);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(path, artifact2));
-    checkConflictWarning();
-  }
-
   private BiConsumer<ConflictType, String> eventConflictReceiver(EventKind eventKind) {
     return (conflictType, message) -> reporter.handle(Event.of(eventKind, message));
   }
@@ -277,127 +238,6 @@ private Runfiles.ConflictChecker eventConflictChecker(Runfiles.ConflictPolicy co
   }
   ;
 
-  @Test
-  public void testPutCatchesConflict() {
-    ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
-    PathFragment pathA = PathFragment.create("a");
-    Artifact artifactB = ActionsTestUtil.createArtifact(root, "b");
-    Artifact artifactC = ActionsTestUtil.createArtifact(root, "c");
-    Map<PathFragment, Artifact> map = new LinkedHashMap<>();
-
-    Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.WARN);
-    checker.put(map, pathA, artifactB, ConflictType.OTHER);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(pathA, artifactB));
-    checker.put(map, pathA, artifactC, ConflictType.OTHER);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(pathA, artifactC));
-    checkConflictWarning();
-  }
-
-  @Test
-  public void testPutReportsError() {
-    ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
-    PathFragment pathA = PathFragment.create("a");
-    Artifact artifactB = ActionsTestUtil.createArtifact(root, "b");
-    Artifact artifactC = ActionsTestUtil.createArtifact(root, "c");
-    Map<PathFragment, Artifact> map = new LinkedHashMap<>();
-
-    // Same as above but with ERROR not WARNING
-    Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.ERROR);
-    checker.put(map, pathA, artifactB, ConflictType.OTHER);
-    reporter.removeHandler(failFastHandler); // So it doesn't throw AssertionError
-    checker.put(map, pathA, artifactC, ConflictType.OTHER);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(pathA, artifactC));
-    checkConflictError();
-  }
-
-  @Test
-  public void testPutCatchesConflictBetweenNullAndNotNull() {
-    ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
-    PathFragment pathA = PathFragment.create("a");
-    Artifact artifactB = ActionsTestUtil.createArtifact(root, "b");
-    Map<PathFragment, Artifact> map = new LinkedHashMap<>();
-
-    Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.WARN);
-    checker.put(map, pathA, null, ConflictType.OTHER);
-    checker.put(map, pathA, artifactB, ConflictType.OTHER);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(pathA, artifactB));
-    checkConflictWarning();
-  }
-
-  @Test
-  public void testPutCatchesConflictBetweenNotNullAndNull() {
-    ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
-    PathFragment pathA = PathFragment.create("a");
-    Artifact artifactB = ActionsTestUtil.createArtifact(root, "b");
-    Map<PathFragment, Artifact> map = new LinkedHashMap<>();
-
-    // Same as above but opposite order
-    Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.WARN);
-    checker.put(map, pathA, artifactB, ConflictType.OTHER);
-    checker.put(map, pathA, null, ConflictType.OTHER);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(pathA, null));
-    checkConflictWarning();
-  }
-
-  @Test
-  public void testPutIgnoresConflict() {
-    ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
-    PathFragment pathA = PathFragment.create("a");
-    Artifact artifactB = ActionsTestUtil.createArtifact(root, "b");
-    Artifact artifactC = ActionsTestUtil.createArtifact(root, "c");
-    Map<PathFragment, Artifact> map = new LinkedHashMap<>();
-
-    Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.IGNORE);
-    checker.put(map, pathA, artifactB, ConflictType.OTHER);
-    checker.put(map, pathA, artifactC, ConflictType.OTHER);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(pathA, artifactC));
-    assertNoEvents();
-  }
-
-  @Test
-  public void testPutIgnoresConflictWithIgnorePolicy() {
-    ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
-    PathFragment pathA = PathFragment.create("a");
-    Artifact artifactB = ActionsTestUtil.createArtifact(root, "b");
-    Artifact artifactC = ActionsTestUtil.createArtifact(root, "c");
-    Map<PathFragment, Artifact> map = new LinkedHashMap<>();
-
-    Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.IGNORE);
-    checker.put(map, pathA, artifactB, ConflictType.OTHER);
-    checker.put(map, pathA, artifactC, ConflictType.OTHER);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(pathA, artifactC));
-    assertNoEvents();
-  }
-
-  @Test
-  public void testPutIgnoresSameArtifact() {
-    ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
-    PathFragment pathA = PathFragment.create("a");
-    Artifact artifactB = ActionsTestUtil.createArtifact(root, "b");
-    Artifact artifactB2 = ActionsTestUtil.createArtifact(root, "b");
-    assertThat(artifactB2).isEqualTo(artifactB);
-    Map<PathFragment, Artifact> map = new LinkedHashMap<>();
-
-    Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.WARN);
-    checker.put(map, pathA, artifactB, ConflictType.OTHER);
-    checker.put(map, pathA, artifactB2, ConflictType.OTHER);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(pathA, artifactB2));
-    assertNoEvents();
-  }
-
-  @Test
-  public void testPutIgnoresNullAndNull() {
-    PathFragment pathA = PathFragment.create("a");
-    Map<PathFragment, Artifact> map = new LinkedHashMap<>();
-
-    Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.WARN);
-    checker.put(map, pathA, null, ConflictType.OTHER);
-    // Add it again
-    checker.put(map, pathA, null, ConflictType.OTHER);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(pathA, null));
-    assertNoEvents();
-  }
-
   @Test
   public void testPutNoConflicts() {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
@@ -409,11 +249,11 @@ public void testPutNoConflicts() {
     Map<PathFragment, Artifact> map = new LinkedHashMap<>();
 
     Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.WARN);
-    checker.put(map, pathA, artifactA, ConflictType.OTHER);
+    checker.put(map, pathA, artifactA);
     // Add different artifact under different path
-    checker.put(map, pathB, artifactB, ConflictType.OTHER);
+    checker.put(map, pathB, artifactB);
     // Add artifact again under different path
-    checker.put(map, pathC, artifactA, ConflictType.OTHER);
+    checker.put(map, pathC, artifactA);
     assertThat(map.entrySet())
         .containsExactly(
             Maps.immutableEntry(pathA, artifactA),
@@ -495,31 +335,6 @@ public void testRunfileAdded() {
     assertNoEvents();
   }
 
-  // TODO(kchodorow): remove this once the default workspace name is always set.
-  @Test
-  public void testConflictWithExternal() {
-    ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
-    PathFragment pathB = PathFragment.create("repo/b");
-    PathFragment externalLegacyPath = LabelConstants.EXTERNAL_PATH_PREFIX.getRelative(pathB);
-    PathFragment externalRunfilesPathB =
-        LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX.getRelative(pathB);
-    Artifact artifactB = ActionsTestUtil.createArtifactWithRootRelativePath(root, pathB);
-    Artifact artifactExternalB =
-        ActionsTestUtil.createArtifactWithRootRelativePath(root, externalLegacyPath);
-
-    Runfiles.ManifestBuilder builder = new Runfiles.ManifestBuilder(
-        PathFragment.EMPTY_FRAGMENT, false);
-
-    Map<PathFragment, Artifact> inputManifest =
-        ImmutableMap.of(pathB, artifactB, externalRunfilesPathB, artifactExternalB);
-    Runfiles.ConflictChecker checker = eventConflictChecker(Runfiles.ConflictPolicy.WARN);
-    builder.addUnderWorkspace(inputManifest, checker);
-
-    assertThat(builder.build().entrySet()).containsExactly(
-        Maps.immutableEntry(PathFragment.create("repo/b"), artifactExternalB));
-    checkConflictWarning();
-  }
-
   @Test
   public void testMergeWithSymlinks() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));