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
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 aaba224..4c9a273 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 @@
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.skyframe.SkyValue;
import java.io.IOException;
import java.util.Arrays;
+import java.util.Objects;
import javax.annotation.Nullable;
/**
@@ -204,7 +206,28 @@
@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 7637abd..87e4bd2 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 @@
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 10e9674..d221a77 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 @@
},
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)
