Make Artifacts be compared to each other based on their exec paths (and not their paths). This gives a predictable order in places where Artifacts are sorted by their natural order.

This works because exec paths of Artifacts are unique in any given build.

--
MOS_MIGRATED_REVID=90807141

3 files changed, 26 insertions, 14 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index 95bb3abbfa..4eb11cad0c 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Comparator;
 import java.util.List;
 
 import javax.annotation.Nullable;
@@ -68,7 +69,25 @@ import javax.annotation.Nullable;
 @SkylarkModule(name = "File",
     doc = "This type represents a file used by the build system. It can be "
         + "either a source file or a derived file produced by a rule.")
-public class Artifact implements FileType.HasFilename, Comparable<Artifact>, ActionInput {
+public class Artifact implements FileType.HasFilename, ActionInput {
+
+  /**
+   * Compares artifact according to their exec paths. Sorts null values first.
+   */
+  public static final Comparator<Artifact> EXEC_PATH_COMPARATOR = new Comparator<Artifact>() {
+    @Override
+    public int compare(Artifact a, Artifact b) {
+      if (a == b) {
+        return 0;
+      } else if (a == null) {
+        return -1;
+      } else if (b == null) {
+        return -1;
+      } else {
+        return a.execPath.compareTo(b.execPath);
+      }
+    }
+  };
 
   /** An object that can expand middleman artifacts. */
   public interface MiddlemanExpander {
@@ -366,19 +385,12 @@ public class Artifact implements FileType.HasFilename, Comparable<Artifact>, Act
       return false;
     }
     // We don't bother to check root in the equivalence relation, because we
-    // assume that 'root' is an ancestor of 'path', and that all possible roots
-    // are disjoint, so unless things are really screwed up, it's ok.
+    // assume that no root is an ancestor of another one.
     Artifact that = (Artifact) other;
     return this.path.equals(that.path);
   }
 
   @Override
-  public final int compareTo(Artifact o) {
-    // The artifact factory ensures that there is a unique artifact for a given path.
-    return this.path.compareTo(o.path);
-  }
-
-  @Override
   public final int hashCode() {
     return path.hashCode();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
index 921e4e7164..cc076e00b8 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java
@@ -167,7 +167,7 @@ public class CachingAnalysisEnvironment implements AnalysisEnvironment {
     }
     // The order of the artifacts.entrySet iteration is unspecified - we use a TreeMap here to
     // guarantee that the return value of this method is deterministic.
-    Map<Artifact, String> orphanArtifacts = new TreeMap<>();
+    Map<Artifact, String> orphanArtifacts = new TreeMap<>(Artifact.EXEC_PATH_COMPARATOR);
     for (Map.Entry<Artifact, String> entry : artifacts.entrySet()) {
       Artifact a = entry.getKey();
       if (!a.isSourceArtifact() && !artifactsWithActions.contains(a)) {
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
index 0ba34cb057..c4b3f53d28 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
@@ -85,10 +85,10 @@ public class ArtifactTest {
     PathFragment bPath = new PathFragment("src/b");
     Artifact aArtifact = new Artifact(aPath, rootDir);
     Artifact bArtifact = new Artifact(bPath, rootDir);
-    assertEquals(-1, aArtifact.compareTo(bArtifact));
-    assertEquals(0, aArtifact.compareTo(aArtifact));
-    assertEquals(0, bArtifact.compareTo(bArtifact));
-    assertEquals(1, bArtifact.compareTo(aArtifact));
+    assertEquals(-1, Artifact.EXEC_PATH_COMPARATOR.compare(aArtifact, bArtifact));
+    assertEquals(0, Artifact.EXEC_PATH_COMPARATOR.compare(aArtifact, aArtifact));
+    assertEquals(0, Artifact.EXEC_PATH_COMPARATOR.compare(bArtifact, bArtifact));
+    assertEquals(1, Artifact.EXEC_PATH_COMPARATOR.compare(bArtifact, aArtifact));
   }
 
   @Test