Make runfiles conflicts fail the build.
Previously, runfiles conflicts would print an ERROR message and then allow the build to continue, resolving the conflict in an arbitrary way.
Most of this CL is churn to allow the runfiles mapping computation methods to raise ExecException. The only interesting changes are in Runfiles.java, RunfilesTest.java, and SkylarkRuleImplementationFunctionsTest.java.
Fixes https://github.com/bazelbuild/bazel/issues/4965.
Closes #5814.
PiperOrigin-RevId: 211437751
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 3c2811e..70ea2d1 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 @@
.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 @@
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 @@
}
}
- /**
- * 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 @@
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 @@
* 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 @@
*
* @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 @@
/** 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 @@
}
this.eventHandler = eventHandler;
this.location = location;
- this.eventKind = (policy == ConflictPolicy.ERROR) ? EventKind.ERROR : EventKind.WARNING;
}
/**
@@ -708,7 +726,8 @@
* @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 @@
(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 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());
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 54d215a..51b54be 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 @@
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 @@
}
@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 @@
}
@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 @@
}
@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 @@
}
@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 @@
}
@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 @@
}
@Test
- public void testPutIgnoresNullAndNull() {
+ public void testPutIgnoresNullAndNull() throws Exception {
PathFragment pathA = PathFragment.create("a");
Map<PathFragment, Artifact> map = new LinkedHashMap<>();
@@ -270,7 +263,7 @@
}
@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 @@
}
@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 @@
}
@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 @@
// 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 @@
}
@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 @@
.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 d98c16f..264346b 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;
@@ -915,7 +916,7 @@
@Test
public void testRunfilesSymlinkConflict() throws Exception {
- // Two different artifacts mapped to same path in runfiles
+ // Two different artifacts mapped to same path in runfiles.
Object result =
evalRuleContextCode(
"artifacts = ruleContext.files.srcs",
@@ -924,9 +925,10 @@
"root_symlinks = {prefix + 'sym1': artifacts[0]},",
"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");
+ reporter.removeHandler(failFastHandler); // So it doesn't throw an exception.
+ assertThat(assertThrows(IOException.class, () -> runfiles.getRunfilesInputs(reporter, null)))
+ .hasMessageThat()
+ .matches("runfile [\\w_]+/sym1 mapped to both foo/b.img and foo/a.txt");
}
private Iterable<Artifact> getRunfileArtifacts(Object runfiles) {
