Make the semantics of runfiles trees saner by always using the entry that comes last in the iteration order for a particular path.

Before, if there were three entries for the same path with the middle one being different, the first one would be overridden by the middle one, and the last one would be omitted because it is .equals() to the first.

--
MOS_MIGRATED_REVID=88377253

1 files changed, 65 insertions, 22 deletions

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 6c06a186fc..5c0537c127 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
@@ -19,7 +19,6 @@ import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.Constants;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
@@ -58,11 +57,11 @@ import javax.annotation.concurrent.Immutable;
  */
 @Immutable
 public final class Runfiles {
-  private static final Function<Map.Entry<PathFragment, Artifact>, Artifact> TO_ARTIFACT =
-      new Function<Map.Entry<PathFragment, Artifact>, Artifact>() {
+  private static final Function<SymlinkEntry, Artifact> TO_ARTIFACT =
+      new Function<SymlinkEntry, Artifact>() {
         @Override
-        public Artifact apply(Map.Entry<PathFragment, Artifact> input) {
-          return input.getValue();
+        public Artifact apply(SymlinkEntry input) {
+          return input.getArtifact();
         }
       };
 
@@ -75,6 +74,46 @@ public final class Runfiles {
         }
       };
 
+  /**
+   * An entry in the runfiles map.
+   */
+  //
+  // O intrepid fixer or bugs and implementor of features, dare not to add a .equals() method
+  // to this class, lest you condemn yourself, or a fellow other developer to spending two
+  // delightful hours in a fancy hotel on a Chromebook that is utterly unsuitable for Java
+  // development to figure out what went wrong, just like I just did.
+  //
+  // The semantics of the symlinks nested set dictates that later entries overwrite earlier
+  // ones. However, the semantics of nested sets dictate that if there are duplicate entries, they
+  // are only returned once in the iterator.
+  //
+  // These two things, innocent when taken alone, result in the effect that when there are three
+  // entries for the same path, the first one and the last one the same, and the middle one
+  // different, the *middle* one will take effect: the middle one overrides the first one, and the
+  // first one prevents the last one from appearing on the iterator.
+  //
+  // The lack of a .equals() method prevents this by making the first entry in the above case not
+  // equals to the third one if they are not the same instance (which they almost never are)
+  //
+  // Goodnight, prince(ss)?, and sweet dreams.
+  private static final class SymlinkEntry {
+    private final PathFragment path;
+    private final Artifact artifact;
+
+    private SymlinkEntry(PathFragment path, Artifact artifact) {
+      this.path = path;
+      this.artifact = artifact;
+    }
+
+    public PathFragment getPath() {
+      return path;
+    }
+
+    public Artifact getArtifact() {
+      return artifact;
+    }
+  }
+
   // It is important to declare this *after* the DUMMY_SYMLINK_EXPANDER to avoid NPEs
   public static final Runfiles EMPTY = new Builder().build();
 
@@ -103,14 +142,14 @@ public final class Runfiles {
    *
    * <p>This may include runfiles symlinks from the root of the runfiles tree.
    */
-  private final NestedSet<Map.Entry<PathFragment, Artifact>> symlinks;
+  private final NestedSet<SymlinkEntry> symlinks;
 
   /**
    * A map of symlinks that should be present above the runfiles directory. These are useful for
    * certain rule types like AppEngine apps which have root level config files outside of the
    * regular source tree.
    */
-  private final NestedSet<Map.Entry<PathFragment, Artifact>> rootSymlinks;
+  private final NestedSet<SymlinkEntry> rootSymlinks;
 
   /**
    * A function to generate extra manifest entries.
@@ -170,8 +209,8 @@ public final class Runfiles {
 
   private Runfiles(String suffix,
       NestedSet<Artifact> artifacts,
-      NestedSet<Map.Entry<PathFragment, Artifact>> symlinks,
-      NestedSet<Map.Entry<PathFragment, Artifact>> rootSymlinks,
+      NestedSet<SymlinkEntry> symlinks,
+      NestedSet<SymlinkEntry> rootSymlinks,
       NestedSet<PruningManifest> pruningManifests,
       Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>> expander) {
     this.suffix = suffix == null ? Constants.DEFAULT_RUNFILES_PREFIX : suffix;
@@ -232,7 +271,7 @@ public final class Runfiles {
   /**
    * Returns the symlinks.
    */
-  public NestedSet<Map.Entry<PathFragment, Artifact>> getSymlinks() {
+  public NestedSet<SymlinkEntry> getSymlinks() {
     return symlinks;
   }
 
@@ -345,7 +384,7 @@ public final class Runfiles {
   /**
    * Returns the root symlinks.
    */
-  public NestedSet<Map.Entry<PathFragment, Artifact>> getRootSymlinks() {
+  public NestedSet<SymlinkEntry> getRootSymlinks() {
     return rootSymlinks;
   }
 
@@ -413,10 +452,10 @@ public final class Runfiles {
         pruningManifests.isEmpty();
   }
 
-  private static <K, V> Map<K, V> entriesToMap(Iterable<Map.Entry<K, V>> entrySet) {
-    Map<K, V> map = new LinkedHashMap<>();
-    for (Map.Entry<K, V> entry : entrySet) {
-      map.put(entry.getKey(), entry.getValue());
+  private static Map<PathFragment, Artifact> entriesToMap(Iterable<SymlinkEntry> entrySet) {
+    Map<PathFragment, Artifact> map = new LinkedHashMap<>();
+    for (SymlinkEntry entry : entrySet) {
+      map.put(entry.getPath(), entry.getArtifact());
     }
     return map;
   }
@@ -434,9 +473,9 @@ public final class Runfiles {
      */
     private NestedSetBuilder<Artifact> artifactsBuilder =
         NestedSetBuilder.compileOrder();
-    private NestedSetBuilder<Map.Entry<PathFragment, Artifact>> symlinksBuilder =
+    private NestedSetBuilder<SymlinkEntry> symlinksBuilder =
         NestedSetBuilder.stableOrder();
-    private NestedSetBuilder<Map.Entry<PathFragment, Artifact>> rootSymlinksBuilder =
+    private NestedSetBuilder<SymlinkEntry> rootSymlinksBuilder =
         NestedSetBuilder.stableOrder();
     private NestedSetBuilder<PruningManifest> pruningManifestsBuilder =
         NestedSetBuilder.stableOrder();
@@ -497,7 +536,7 @@ public final class Runfiles {
     public Builder addSymlink(PathFragment link, Artifact target) {
       Preconditions.checkNotNull(link);
       Preconditions.checkNotNull(target);
-      symlinksBuilder.add(Maps.immutableEntry(link, target));
+      symlinksBuilder.add(new SymlinkEntry(link, target));
       return this;
     }
 
@@ -505,14 +544,16 @@ public final class Runfiles {
      * Adds several symlinks.
      */
     public Builder addSymlinks(Map<PathFragment, Artifact> symlinks) {
-      symlinksBuilder.addAll(symlinks.entrySet());
+      for (Map.Entry<PathFragment, Artifact> symlink : symlinks.entrySet()) {
+        symlinksBuilder.add(new SymlinkEntry(symlink.getKey(), symlink.getValue()));
+      }
       return this;
     }
 
     /**
      * Adds several symlinks as a NestedSet.
      */
-    public Builder addSymlinks(NestedSet<Map.Entry<PathFragment, Artifact>> symlinks) {
+    public Builder addSymlinks(NestedSet<SymlinkEntry> symlinks) {
       symlinksBuilder.addTransitive(symlinks);
       return this;
     }
@@ -521,14 +562,16 @@ public final class Runfiles {
      * Adds several root symlinks.
      */
     public Builder addRootSymlinks(Map<PathFragment, Artifact> symlinks) {
-      rootSymlinksBuilder.addAll(symlinks.entrySet());
+      for (Map.Entry<PathFragment, Artifact> symlink : symlinks.entrySet()) {
+        rootSymlinksBuilder.add(new SymlinkEntry(symlink.getKey(), symlink.getValue()));
+      }
       return this;
     }
 
     /**
      * Adds several root symlinks as a NestedSet.
      */
-    public Builder addRootSymlinks(NestedSet<Map.Entry<PathFragment, Artifact>> symlinks) {
+    public Builder addRootSymlinks(NestedSet<SymlinkEntry> symlinks) {
       rootSymlinksBuilder.addTransitive(symlinks);
       return this;
     }