commit 8c007af50652a453a098d455e610d1af31095ec6
author ulfjack <ulfjack@google.com> Tue Feb 12 11:10:24 2019 -0800
committer Copybara-Service <copybara-piper@google.com> Tue Feb 12 11:24:48 2019 -0800
tree 51053b7d6b1260e492fb04cc0a592f7d33c2a563
parent 8f0ce21a5078b5924e8deee3430997127b1f6576

Correctly merge empty runfiles w/ non-empty runfiles

Since an empty runfiles object doesn't have a valid suffix, we cannot create a correct Runfiles.Builder from it.

PiperOrigin-RevId: 233641175

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

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 4ca156a..50a325f 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
@@ -1165,10 +1165,16 @@
 
   @Override
   public Runfiles merge(RunfilesApi other) {
-    Runfiles.Builder builder = new Runfiles.Builder(suffix, false);
-    builder.merge(this);
-    builder.merge((Runfiles) other);
-    return builder.build();
+    Runfiles o = (Runfiles) other;
+    if (isEmpty()) {
+      // This is not just a memory / performance optimization. The Builder requires a valid suffix,
+      // but the {@code Runfiles.EMPTY} singleton has an invalid one, which must not be used to
+      // construct a Runfiles.Builder.
+      return o;
+    } else if (o.isEmpty()) {
+      return this;
+    }
+    return new Runfiles.Builder(suffix, false).merge(this).merge(o).build();
   }
 
   /**

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

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 5a7cb89..26a6dbc 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
@@ -411,6 +411,15 @@
   }
 
   @Test
+  public void testMergeEmptyWithNonEmpty() {
+    ArtifactRoot root = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace")));
+    Artifact artifactA = new Artifact(PathFragment.create("a/target"), root);
+    Runfiles runfilesB = new Runfiles.Builder("TESTING").addArtifact(artifactA).build();
+    assertThat(Runfiles.EMPTY.merge(runfilesB)).isSameAs(runfilesB);
+    assertThat(runfilesB.merge(Runfiles.EMPTY)).isSameAs(runfilesB);
+  }
+
+  @Test
   public void testOnlyExtraMiddlemenNotConsideredEmpty() {
     ArtifactRoot root =
         ArtifactRoot.middlemanRoot(scratch.resolve("execroot"), scratch.resolve("execroot/out"));