Fix RegularFileArtifactValue.equals

If two SkyValue objects are equal, then Skyframe caches and returns the old one, instead of the new one. That's a problem for the detection of file changes, which uses the FileContentsProxy, which wasn't part of the equals check before this change.

Progress on #3360.

PiperOrigin-RevId: 183834897

3 files changed, 45 insertions, 4 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
index aaba2245dc..4c9a273139 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.skyframe;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
+import com.google.common.io.BaseEncoding;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.FileStateType;
 import com.google.devtools.build.lib.actions.cache.DigestUtils;
@@ -28,6 +29,7 @@ import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.Objects;
 import javax.annotation.Nullable;
 
 /**
@@ -204,7 +206,28 @@ public abstract class FileArtifactValue implements SkyValue, Metadata {
 
     @Override
     public String toString() {
-      return MoreObjects.toStringHelper(this).add("digest", digest).add("size", size).toString();
+      return MoreObjects.toStringHelper(this)
+          .add("digest", BaseEncoding.base16().lowerCase().encode(digest))
+          .add("size", size)
+          .add("proxy", proxy).toString();
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
+      if (!(o instanceof RegularFileArtifactValue)) {
+        return false;
+      }
+      RegularFileArtifactValue r = (RegularFileArtifactValue) o;
+      return Arrays.equals(digest, r.digest) && Objects.equals(proxy, r.proxy) && size == r.size;
+    }
+
+    @Override
+    public int hashCode() {
+      return (proxy != null ? 127 * proxy.hashCode() : 0)
+          + 37 * Long.hashCode(getSize()) + Arrays.hashCode(getDigest());
     }
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
index 7637abd3ba..87e4bd2a6d 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
@@ -92,15 +92,31 @@ public class FileArtifactValueTest {
     Path dir3 = scratchDir("/dir3", 1L);
 
     new EqualsTester()
-        .addEqualityGroup(create(path1), create(path2), create(mtimePath))
+        // We check for ctime and inode equality for paths.
+        .addEqualityGroup(create(path1))
+        .addEqualityGroup(create(path2))
+        .addEqualityGroup(create(mtimePath))
         .addEqualityGroup(create(digestPath))
-        .addEqualityGroup(create(empty1), create(empty2), create(empty3))
+        .addEqualityGroup(create(empty1))
+        .addEqualityGroup(create(empty2))
+        .addEqualityGroup(create(empty3))
+        // We check for mtime equality for directories.
         .addEqualityGroup(create(dir1))
         .addEqualityGroup(create(dir2), create(dir3))
         .testEquals();
   }
 
   @Test
+  public void testCtimeInEquality() throws Exception {
+    Path path = scratchFile("/dir/artifact1", 0L, "content");
+    FileArtifactValue before = create(path);
+    clock.advanceMillis(1);
+    path.chmod(0777);
+    FileArtifactValue after = create(path);
+    assertThat(before).isNotEqualTo(after);
+  }
+
+  @Test
   public void testNoMtimeIfNonemptyFile() throws Exception {
     Path path = scratchFile("/root/non-empty", 1L, "abc");
     FileArtifactValue value = create(path);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java
index 10e9674146..d221a772b3 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java
@@ -510,7 +510,9 @@ public class SkyframeAwareActionTest extends TimestampBuilderTestCase {
         },
         ChangeArtifact.CHANGE_MTIME,
         Callables.<Void>returning(null),
-        ExpectActionIs.DIRTIED_BUT_VERIFIED_CLEAN);
+        unconditionalExecution
+            ? ExpectActionIs.REEXECUTED
+            : ExpectActionIs.DIRTIED_BUT_VERIFIED_CLEAN);
   }
 
   public void testActionWithNonChangingInput(final boolean unconditionalExecution)