Repository: bazelbuild/bazel
Issue: runfiles conflicts print an ERROR message but don't fail the build
Labels: type: bug

```
$ cat rule.bzl
def _will_break(ctx):
    ctx.actions.write(ctx.outputs.executable, '')
    other_file = ctx.actions.declare_file('other-file')
    ctx.actions.write(other_file, '')
    return struct(runfiles=ctx.runfiles(symlinks={ctx.outputs.executable.short_path: other_file}))

will_break = rule(
    executable = True,
    implementation = _will_break,
)
$ cat BUILD
load(':rule.bzl', 'will_break')
will_break(name='x')
$ bazel  --batch build //:x
INFO: Analysed target //:x (4 packages loaded).
INFO: Found 1 target...
ERROR: /home/benjamin/repos/k/BUILD:2:1: overwrote runfile x, was symlink to /home/benjamin/.cache/bazel/_bazel_benjamin/c65d72cc374bbf6340b06c981e4234e9/execroot/__main__/bazel-out/k8-fastbuild/bin/other-file, now symlink to /home/benjamin/.cache/bazel/_bazel_benjamin/c65d72cc374bbf6340b06c981e4234e9/execroot/__main__/bazel-out/k8-fastbuild/bin/x
Target //:x up-to-date:
  bazel-bin/x
INFO: Elapsed time: 1.131s, Critical Path: 0.02s
INFO: 1 process, local.
INFO: Build completed successfully, 5 total actions
$ echo $?
0
```
Generally, I expect a non-zero exit code if an `ERROR` message is printed.


Issue comments:
Shall we reopen this, since 0e81f9aad49215fe4a13092e64384d9c0ef1ba76 reverted the fix?
+1 this was surprising to me that it wasn't failing the build
+1 this still happens in bazel-1.1.0
Could we re-open this issue?  We are hitting it in 3.1.0, and in our case it's definitely signals bugs.  Our of developers are using dependencies together they should not.  We have to put together some unpleasant checks to make sure these don't creep in, but they are slow and extra code to maintain.

What about adding this back in with a compatibility flag like `--experimental_no_runfile_conflicts` that we transition to in the future?  Similar to the legacy runfiles transition.
Ideally the case should not end up being a conflict. I'd expect bazel to either put `ctx.actions.symlink artifact` into runfiles.symlinks dict or handle the case above. 

I understand why bazel behaves the way it does right now but it should be improved. 

I see three things that could be done here to improve this weird edge case;

- Do not print an `ERROR` if a symlink points to same path eventually. Conflict checker would have follow the chain of symlinks in this case.

- Artifact created by the combination of `declare_file` / `declare_symlink` and `ctx.actions.symlink` should designate the declared artifact as symlink.  perhaps a new property can be added to `Artifact` class called `is_symlink` to determine whether its a symlink. similar to `is_source` `is_directory` 

- Artifact that is output of `SymlinkAction` should go into `symlinks` dict of runfiles.


+1 for `--experimental_no_runfile_conflicts`

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
index 3c2811eb981a83..70ea2d168bef79 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
@@ -360,12 +360,22 @@ public NestedSet<String> getEmptyFilenames() {
         .build();
   }
 
+  /** Returns the symlinks as a map from path fragment to artifact. */
+  public Map<PathFragment, Artifact> getSymlinksAsMap() {
+    try {
+      return entriesToMap(symlinks, null);
+    } catch (IOException e) {
+      throw new IllegalStateException("unexpected since we passed a null conflict checker", e);
+    }
+  }
+
   /**
    * Returns the symlinks as a map from path fragment to artifact.
    *
    * @param checker If not null, check for conflicts using this checker.
    */
-  public Map<PathFragment, Artifact> getSymlinksAsMap(@Nullable ConflictChecker checker) {
+  public Map<PathFragment, Artifact> getSymlinksAsMap(@Nullable ConflictChecker checker)
+      throws IOException {
     return entriesToMap(symlinks, checker);
   }
 
@@ -502,11 +512,9 @@ public ManifestBuilder(
       this.sawWorkspaceName = legacyExternalRunfiles;
     }
 
-    /**
-     * Adds a map under the workspaceName.
-     */
+    /** Adds a map under the workspaceName. */
     public void addUnderWorkspace(
-        Map<PathFragment, Artifact> inputManifest, ConflictChecker checker) {
+        Map<PathFragment, Artifact> inputManifest, ConflictChecker checker) throws IOException {
       for (Map.Entry<PathFragment, Artifact> entry : inputManifest.entrySet()) {
         PathFragment path = entry.getKey();
         if (isUnderWorkspace(path)) {
@@ -522,10 +530,9 @@ public void addUnderWorkspace(
       }
     }
 
-    /**
-     * Adds a map to the root directory.
-     */
-    public void add(Map<PathFragment, Artifact> inputManifest, ConflictChecker checker) {
+    /** Adds a map to the root directory. */
+    public void add(Map<PathFragment, Artifact> inputManifest, ConflictChecker checker)
+        throws IOException {
       for (Map.Entry<PathFragment, Artifact> entry : inputManifest.entrySet()) {
         checker.put(manifest, checkForWorkspace(entry.getKey()), entry.getValue());
       }
@@ -571,12 +578,22 @@ public NestedSet<SymlinkEntry> getRootSymlinks() {
     return rootSymlinks;
   }
 
+  /** Returns the root symlinks as a map from path fragment to artifact. */
+  public Map<PathFragment, Artifact> getRootSymlinksAsMap() {
+    try {
+      return entriesToMap(rootSymlinks, null);
+    } catch (IOException e) {
+      throw new IllegalStateException("unexpected since we passed a null conflict checker", e);
+    }
+  }
+
   /**
    * Returns the root symlinks as a map from path fragment to artifact.
    *
    * @param checker If not null, check for conflicts using this checker.
    */
-  public Map<PathFragment, Artifact> getRootSymlinksAsMap(@Nullable ConflictChecker checker) {
+  public Map<PathFragment, Artifact> getRootSymlinksAsMap(@Nullable ConflictChecker checker)
+      throws IOException {
     return entriesToMap(rootSymlinks, checker);
   }
 
@@ -585,7 +602,12 @@ public Map<PathFragment, Artifact> getRootSymlinksAsMap(@Nullable ConflictChecke
    * account.
    */
   public Map<PathFragment, Artifact> asMapWithoutRootSymlinks() {
-    Map<PathFragment, Artifact> result = entriesToMap(symlinks, null);
+    Map<PathFragment, Artifact> result;
+    try {
+      result = entriesToMap(symlinks, null);
+    } catch (IOException e) {
+      throw new IllegalStateException("unexpected since we passed a null conflict checker", e);
+    }
     // If multiple artifacts have the same root-relative path, the last one in the list will win.
     // That is because the runfiles tree cannot contain the same artifact for different
     // configurations, because it only uses root-relative paths.
@@ -645,11 +667,11 @@ public boolean isEmpty() {
    *
    * @param entrySet Sequence of entries to add.
    * @param checker If not null, check for conflicts with this checker, otherwise silently allow
-   *    entries to overwrite previous entries.
+   *     entries to overwrite previous entries.
    * @return Map<PathFragment, Artifact> Map of runfile entries.
    */
   private static Map<PathFragment, Artifact> entriesToMap(
-      Iterable<SymlinkEntry> entrySet, @Nullable ConflictChecker checker) {
+      Iterable<SymlinkEntry> entrySet, @Nullable ConflictChecker checker) throws IOException {
     checker = (checker != null) ? checker : ConflictChecker.IGNORE_CHECKER;
     Map<PathFragment, Artifact> map = new LinkedHashMap<>();
     for (SymlinkEntry entry : entrySet) {
@@ -686,9 +708,6 @@ public static final class ConflictChecker {
     /** Location for eventHandler warnings. Ignored if eventHandler is null. */
     private final Location location;
 
-    /** Type of event to emit */
-    private final EventKind eventKind;
-
     /** Construct a ConflictChecker for the given reporter with the given behavior */
     public ConflictChecker(ConflictPolicy policy, EventHandler eventHandler, Location location) {
       if (eventHandler == null) {
@@ -698,7 +717,6 @@ public ConflictChecker(ConflictPolicy policy, EventHandler eventHandler, Locatio
       }
       this.eventHandler = eventHandler;
       this.location = location;
-      this.eventKind = (policy == ConflictPolicy.ERROR) ? EventKind.ERROR : EventKind.WARNING;
     }
 
     /**
@@ -708,7 +726,8 @@ public ConflictChecker(ConflictPolicy policy, EventHandler eventHandler, Locatio
      * @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.
      */
-    public void put(Map<PathFragment, Artifact> map, PathFragment path, Artifact artifact) {
+    public void put(Map<PathFragment, Artifact> map, PathFragment path, Artifact artifact)
+        throws IOException {
       Preconditions.checkArgument(
           artifact == null || !artifact.isMiddlemanArtifact(), "%s", artifact);
       if (policy != ConflictPolicy.IGNORE && map.containsKey(path)) {
@@ -719,13 +738,18 @@ public void put(Map<PathFragment, Artifact> map, PathFragment path, Artifact art
               (previous == null) ? "empty file" : previous.getExecPath().toString();
           String artifactStr =
               (artifact == null) ? "empty file" : artifact.getExecPath().toString();
-          String message =
-              String.format(
-                  "overwrote runfile %s, was symlink to %s, now symlink to %s",
-                  path.getSafePathString(),
-                  previousStr,
-                  artifactStr);
-          eventHandler.handle(Event.of(eventKind, location, message));
+          if (policy == ConflictPolicy.WARN) {
+            String message =
+                String.format(
+                    "overwrote runfile %s, was symlink to %s, now symlink to %s",
+                    path.getSafePathString(), previousStr, artifactStr);
+            eventHandler.handle(Event.of(EventKind.WARNING, location, message));
+          } else {
+            throw new IOException(
+                String.format(
+                    "runfile %s mapped to both %s and %s",
+                    path.getSafePathString(), previousStr, artifactStr));
+          }
         }
       }
       map.put(path, artifact);
@@ -1169,13 +1193,13 @@ public Runfiles merge(RunfilesApi other) {
   public void fingerprint(Fingerprint fp) {
     fp.addBoolean(getLegacyExternalRunfiles());
     fp.addPath(getSuffix());
-    Map<PathFragment, Artifact> symlinks = getSymlinksAsMap(null);
+    Map<PathFragment, Artifact> symlinks = getSymlinksAsMap();
     fp.addInt(symlinks.size());
     for (Map.Entry<PathFragment, Artifact> symlink : symlinks.entrySet()) {
       fp.addPath(symlink.getKey());
       fp.addPath(symlink.getValue().getExecPath());
     }
-    Map<PathFragment, Artifact> rootSymlinks = getRootSymlinksAsMap(null);
+    Map<PathFragment, Artifact> rootSymlinks = getRootSymlinksAsMap();
     fp.addInt(rootSymlinks.size());
     for (Map.Entry<PathFragment, Artifact> rootSymlink : rootSymlinks.entrySet()) {
       fp.addPath(rootSymlink.getKey());


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java
index 54d215a31f7454..51b54bec83a592 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java
@@ -15,6 +15,7 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -27,6 +28,7 @@
 import com.google.devtools.build.lib.testutil.FoundationTestCase;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
+import java.io.IOException;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.Map;
@@ -132,16 +134,8 @@ private void checkConflictWarning() {
     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);
-  }
-
   @Test
-  public void testPutCatchesConflict() {
+  public void testPutCatchesConflict() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     PathFragment pathA = PathFragment.create("a");
     Artifact artifactB = new Artifact(PathFragment.create("b"), root);
@@ -158,25 +152,24 @@ public void testPutCatchesConflict() {
   }
 
   @Test
-  public void testPutReportsError() {
+  public void testPutReportsError() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     PathFragment pathA = PathFragment.create("a");
     Artifact artifactB = new Artifact(PathFragment.create("b"), root);
     Artifact artifactC = new Artifact(PathFragment.create("c"), root);
     Map<PathFragment, Artifact> map = new LinkedHashMap<>();
 
-    // Same as above but with ERROR not WARNING
     Runfiles.ConflictChecker checker =
         new Runfiles.ConflictChecker(Runfiles.ConflictPolicy.ERROR, reporter, null);
     checker.put(map, pathA, artifactB);
     reporter.removeHandler(failFastHandler); // So it doesn't throw AssertionError
-    checker.put(map, pathA, artifactC);
-    assertThat(map.entrySet()).containsExactly(Maps.immutableEntry(pathA, artifactC));
-    checkConflictError();
+    assertThat(assertThrows(IOException.class, () -> checker.put(map, pathA, artifactC)))
+        .hasMessageThat()
+        .isEqualTo("runfile a mapped to both b and c");
   }
 
   @Test
-  public void testPutCatchesConflictBetweenNullAndNotNull() {
+  public void testPutCatchesConflictBetweenNullAndNotNull() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     PathFragment pathA = PathFragment.create("a");
     Artifact artifactB = new Artifact(PathFragment.create("b"), root);
@@ -191,7 +184,7 @@ public void testPutCatchesConflictBetweenNullAndNotNull() {
   }
 
   @Test
-  public void testPutCatchesConflictBetweenNotNullAndNull() {
+  public void testPutCatchesConflictBetweenNotNullAndNull() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     PathFragment pathA = PathFragment.create("a");
     Artifact artifactB = new Artifact(PathFragment.create("b"), root);
@@ -207,7 +200,7 @@ public void testPutCatchesConflictBetweenNotNullAndNull() {
   }
 
   @Test
-  public void testPutIgnoresConflict() {
+  public void testPutIgnoresConflict() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     PathFragment pathA = PathFragment.create("a");
     Artifact artifactB = new Artifact(PathFragment.create("b"), root);
@@ -223,7 +216,7 @@ public void testPutIgnoresConflict() {
   }
 
   @Test
-  public void testPutIgnoresConflictNoListener() {
+  public void testPutIgnoresConflictNoListener() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     PathFragment pathA = PathFragment.create("a");
     Artifact artifactB = new Artifact(PathFragment.create("b"), root);
@@ -239,7 +232,7 @@ public void testPutIgnoresConflictNoListener() {
   }
 
   @Test
-  public void testPutIgnoresSameArtifact() {
+  public void testPutIgnoresSameArtifact() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     PathFragment pathA = PathFragment.create("a");
     Artifact artifactB = new Artifact(PathFragment.create("b"), root);
@@ -256,7 +249,7 @@ public void testPutIgnoresSameArtifact() {
   }
 
   @Test
-  public void testPutIgnoresNullAndNull() {
+  public void testPutIgnoresNullAndNull() throws Exception {
     PathFragment pathA = PathFragment.create("a");
     Map<PathFragment, Artifact> map = new LinkedHashMap<>();
 
@@ -270,7 +263,7 @@ public void testPutIgnoresNullAndNull() {
   }
 
   @Test
-  public void testPutNoConflicts() {
+  public void testPutNoConflicts() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     PathFragment pathA = PathFragment.create("a");
     PathFragment pathB = PathFragment.create("b");
@@ -324,7 +317,7 @@ public void testBuilderMergeConflictPolicyInheritStrictest() {
   }
 
   @Test
-  public void testLegacyRunfilesStructure() {
+  public void testLegacyRunfilesStructure() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     PathFragment workspaceName = PathFragment.create("wsname");
     PathFragment pathB = PathFragment.create("external/repo/b");
@@ -345,7 +338,7 @@ public void testLegacyRunfilesStructure() {
   }
 
   @Test
-  public void testRunfileAdded() {
+  public void testRunfileAdded() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     PathFragment workspaceName = PathFragment.create("wsname");
     PathFragment pathB = PathFragment.create("external/repo/b");
@@ -368,7 +361,7 @@ public void testRunfileAdded() {
 
   // TODO(kchodorow): remove this once the default workspace name is always set.
   @Test
-  public void testConflictWithExternal() {
+  public void testConflictWithExternal() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     PathFragment pathB = PathFragment.create("repo/b");
     PathFragment externalPathB = Label.EXTERNAL_PACKAGE_NAME.getRelative(pathB);
@@ -392,7 +385,7 @@ public void testConflictWithExternal() {
   }
 
   @Test
-  public void testMergeWithSymlinks() {
+  public void testMergeWithSymlinks() throws Exception {
     ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
     Artifact artifactA = new Artifact(PathFragment.create("a/target"), root);
     Artifact artifactB = new Artifact(PathFragment.create("b/target"), root);
@@ -406,8 +399,8 @@ public void testMergeWithSymlinks() {
         .build();
 
     Runfiles runfilesC = runfilesA.merge(runfilesB);
-    assertThat(runfilesC.getSymlinksAsMap(null).get(sympathA)).isEqualTo(artifactA);
-    assertThat(runfilesC.getSymlinksAsMap(null).get(sympathB)).isEqualTo(artifactB);
+    assertThat(runfilesC.getSymlinksAsMap().get(sympathA)).isEqualTo(artifactA);
+    assertThat(runfilesC.getSymlinksAsMap().get(sympathB)).isEqualTo(artifactB);
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
index d98c16f4bc8543..3326125cb85c89 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
@@ -65,6 +65,7 @@
 import com.google.devtools.build.lib.testutil.MoreAsserts;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.OsUtils;
+import java.io.IOException;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
@@ -925,8 +926,10 @@ public void testRunfilesSymlinkConflict() throws Exception {
             "symlinks = {'sym1': artifacts[1]})");
     Runfiles runfiles = (Runfiles) result;
     reporter.removeHandler(failFastHandler); // So it doesn't throw exception
-    runfiles.getRunfilesInputs(reporter, null);
-    assertContainsEvent("ERROR <no location>: overwrote runfile");
+    assertThat(
+        assertThrows(IOException.class, () -> runfiles.getRunfilesInputs(reporter, null)))
+        .hasMessageThat()
+        .isEqualTo("runfile __main__/sym1 mapped to both foo/b.img and foo/a.txt");
   }
 
   private Iterable<Artifact> getRunfileArtifacts(Object runfiles) {