Don't treat empty files specially with respect to mtime/digest.
RELNOTES: Bazel no longer regards an empty file as changed if its mtime has changed.
--
MOS_MIGRATED_REVID=127328552
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java b/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java
index 377ea43..c61e943 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java
@@ -40,17 +40,6 @@
private DigestUtils() {}
/**
- * Returns true iff using MD5 digests is appropriate for an artifact.
- *
- * @param isFile whether or not Artifact is a file versus a directory, isFile() on its stat.
- * @param size size of Artifact on filesystem in bytes, getSize() on its stat.
- */
- public static boolean useFileDigest(boolean isFile, long size) {
- // Use timestamps for directories and empty files. Use digests for everything else.
- return isFile && size != 0;
- }
-
- /**
* Obtain file's MD5 metadata using synchronized method, ensuring that system
* is not overloaded in case when multiple threads are requesting MD5
* calculations and underlying file system cannot provide it via extended
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
index a72ef48..f6a0224 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -24,7 +24,6 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.cache.Digest;
-import com.google.devtools.build.lib.actions.cache.DigestUtils;
import com.google.devtools.build.lib.actions.cache.Metadata;
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
import com.google.devtools.build.lib.skyframe.TreeArtifactValue.TreeArtifactException;
@@ -253,10 +252,9 @@
throw new FileNotFoundException(artifact.prettyPrint() + " does not exist");
}
if (!artifact.hasParent()) {
- // Artifacts may use either the "real" digest or the mtime, if the file is size 0.
+ // Artifacts may use either the "real" digest or the mtime, if the file is a directory.
boolean isFile = data.isFile();
- boolean useDigest = DigestUtils.useFileDigest(isFile, isFile ? data.getSize() : 0);
- if (useDigest && data.getDigest() != null) {
+ if (isFile && data.getDigest() != null) {
// We do not need to store the FileArtifactValue separately -- the digest is in the
// file value and that is all that is needed for this file's metadata.
return new Metadata(data.getDigest());
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 b55639e..5125764 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
@@ -33,9 +33,6 @@
* </li><li>
* a directory, in which case we would expect to see an mtime;
* </li><li>
- * an empty file corresponding to an Artifact, where we would expect to see a size (=0), mtime,
- * and digest;
- * </li><li>
* an intentionally omitted file which the build system is aware of but doesn't actually exist,
* where all access methods are unsupported;
* </li><li>
@@ -107,13 +104,10 @@
if (isFile && digest == null) {
digest = DigestUtils.getDigestOrFail(artifact.getPath(), size);
}
- if (!DigestUtils.useFileDigest(isFile, size)) {
- // In this case, we need to store the mtime because the action cache uses mtime to determine
- // if this artifact has changed. This is currently true for empty files and directories. We
- // do not optimize for this code path (by storing the mtime in a FileValue) because we do not
- // like it and may remove this special-casing for empty files in the future. We want this code
- // path to go away somehow too for directories (maybe by implementing FileSet
- // in Skyframe)
+ if (!isFile) {
+ // In this case, we need to store the mtime because the action cache uses mtime for
+ // directories to determine if this artifact has changed. We want this code path to go away
+ // somehow (maybe by implementing FileSet in Skyframe).
return new FileArtifactValue(digest, artifact.getPath().getLastModifiedTime(), size);
}
Preconditions.checkState(digest != null, artifact);
@@ -158,11 +152,7 @@
return size;
}
- /**
- * Gets last modified time of file. Should only be called if {@link DigestUtils#useFileDigest} was
- * false for this artifact -- namely, either it is a directory or an empty file. Note that since
- * we store directory sizes as 0, all files for which this method can be called have size 0.
- */
+ /** Gets last modified time of file. Should only be called if this is a directory. */
long getModifiedTime() {
Preconditions.checkState(size == 0, "%s %s %s", digest, mtime, size);
return mtime;
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 789e2b1..84e455c 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -263,8 +263,7 @@
assertEquals(1L, value.getModifiedTime());
}
- // Empty files need to store their mtimes, so touching an empty file
- // can be used to trigger rebuilds.
+ // Empty files are the same as normal files -- mtime is not stored.
@Test
public void testEmptyFile() throws Exception {
Artifact artifact = createDerivedArtifact("empty");
@@ -273,7 +272,7 @@
path.setLastModifiedTime(1L);
FileArtifactValue value = create(artifact);
assertArrayEquals(path.getMD5Digest(), value.getDigest());
- assertEquals(1L, value.getModifiedTime());
+ assertEquals(-1L, value.getModifiedTime());
assertEquals(0L, value.getSize());
}
@@ -322,8 +321,7 @@
EqualsTester equalsTester = new EqualsTester();
equalsTester
.addEqualityGroup(create(artifact1), create(artifact2), create(diffMtime))
- .addEqualityGroup(create(empty1))
- .addEqualityGroup(create(empty2), create(empty3))
+ .addEqualityGroup(create(empty1), create(empty2), create(empty3))
.addEqualityGroup(create(dir1))
.addEqualityGroup(create(dir2), create(dir3))
.testEquals();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderMediumTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderMediumTest.java
index dba91d6..3c274f9 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderMediumTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderMediumTest.java
@@ -134,7 +134,8 @@
buildArtifacts(persistentBuilder(cache), goodbye);
assertFalse(button.pressed); // not rebuilt
- FileSystemUtils.touchFile(hello.getPath());
+ hello.getPath().setWritable(true);
+ FileSystemUtils.writeContentAsLatin1(hello.getPath(), "new content");
button.pressed = false;
buildArtifacts(persistentBuilder(cache), goodbye);
@@ -364,7 +365,8 @@
buildArtifacts(persistentBuilder(cache), hello);
assertFalse(button.pressed); // not rebuilt
- BlazeTestUtils.changeModtime(hello.getPath());
+ hello.getPath().setWritable(true);
+ FileSystemUtils.writeContentAsLatin1(hello.getPath(), "new content");
button.pressed = false;
buildArtifacts(persistentBuilder(cache), hello);
@@ -394,7 +396,8 @@
buildArtifacts(persistentBuilder(cache), hello);
assertFalse(button.pressed); // not rebuilt
- BlazeTestUtils.changeModtime(hello.getPath());
+ hello.getPath().setWritable(true);
+ FileSystemUtils.writeContentAsLatin1(hello.getPath(), "new content");
button.pressed = false;
buildArtifacts(persistentBuilder(cache), hello);
@@ -440,7 +443,8 @@
buildArtifacts(persistentBuilder(cache), hello);
assertFalse(button.pressed); // not rebuilt
- BlazeTestUtils.changeModtime(hello.getPath());
+ hello.getPath().setWritable(true);
+ FileSystemUtils.writeContentAsLatin1(hello.getPath(), "new content");
button.pressed = false;
buildArtifacts(persistentBuilder(cache), hello);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTest.java
index 55f1784..ea10d33 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTest.java
@@ -169,8 +169,8 @@
buildArtifacts(cachingBuilder(), goodbye);
assertFalse(button.pressed); // not rebuilt
- // inMemoryMetadataCache.useFileDigest is false, so new timestamp is enough to force a rebuild.
- FileSystemUtils.touchFile(hello.getPath());
+ hello.getPath().setWritable(true);
+ FileSystemUtils.writeContentAsLatin1(hello.getPath(), "new content");
button.pressed = false;
buildArtifacts(cachingBuilder(), goodbye);
@@ -231,10 +231,10 @@
buildArtifacts(cachingBuilder(), hello);
assertFalse(button.pressed); // not rebuilt
- // Touching the *output* file 'hello' causes 'action' to re-execute, to
- // make things consistent again; this is not what Make would do, but it is
- // correct according to this Builder.
- BlazeTestUtils.changeModtime(hello.getPath());
+ // Changing the *output* file 'hello' causes 'action' to re-execute, to make things consistent
+ // again.
+ hello.getPath().setWritable(true);
+ FileSystemUtils.writeContentAsLatin1(hello.getPath(), "new content");
button.pressed = false;
buildArtifacts(cachingBuilder(), hello);
@@ -251,7 +251,8 @@
Artifact hello = createSourceArtifact("hello");
BlazeTestUtils.makeEmptyFile(hello.getPath());
Artifact wazuup = createDerivedArtifact("wazuup");
- Button button1 = createActionButton(Sets.newHashSet(hello), Sets.newHashSet(wazuup));
+ Button button1 = new Button();
+ registerAction(new CopyingAction(button1, hello, wazuup));
Artifact goodbye = createDerivedArtifact("goodbye");
Button button2 = createActionButton(Sets.newHashSet(wazuup), Sets.newHashSet(goodbye));
@@ -275,7 +276,8 @@
assertFalse(button1.pressed); // wazuup not rebuilt
assertFalse(button2.pressed); // goodbye not rebuilt
- FileSystemUtils.touchFile(hello.getPath());
+ hello.getPath().setWritable(true);
+ FileSystemUtils.writeContentAsLatin1(hello.getPath(), "new content");
button1.pressed = button2.pressed = false;
buildArtifacts(cachingBuilder(), goodbye);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
index 51fa9cfa..2c1b7e6 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
@@ -19,6 +19,7 @@
import com.google.common.base.Predicates;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Range;
import com.google.common.collect.Sets;
@@ -26,6 +27,8 @@
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionCacheChecker;
+import com.google.devtools.build.lib.actions.ActionExecutionContext;
+import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.ActionExecutionStatusReporter;
import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator;
import com.google.devtools.build.lib.actions.Artifact;
@@ -54,6 +57,7 @@
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.CycleInfo;
@@ -71,6 +75,7 @@
import org.junit.Before;
+import java.io.IOException;
import java.io.PrintStream;
import java.util.ArrayList;
import java.util.Collection;
@@ -357,6 +362,26 @@
}
}
+ /** {@link TestAction} that copies its single input to its single output. */
+ protected static class CopyingAction extends TestAction {
+ CopyingAction(Runnable effect, Artifact input, Artifact output) {
+ super(effect, ImmutableSet.of(input), ImmutableSet.of(output));
+ }
+
+ @Override
+ public void execute(ActionExecutionContext actionExecutionContext)
+ throws ActionExecutionException {
+ super.execute(actionExecutionContext);
+ try {
+ FileSystemUtils.copyFile(
+ Iterables.getOnlyElement(getInputs()).getPath(),
+ Iterables.getOnlyElement(getOutputs()).getPath());
+ } catch (IOException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+ }
+
protected static class InMemoryActionCache implements ActionCache {
private final Map<String, Entry> actionCache = new HashMap<>();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
index 0d6d172..b68a86c 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
@@ -260,11 +260,7 @@
assertTrue(buttonTwo.pressed);
}
- /**
- * TreeArtifacts don't care about mtime, even when the file is empty.
- * However, actions taking input non-Tree artifacts still care about mtime
- * (although this behavior should go away).
- */
+ /** TreeArtifacts don't care about mtime, even when the file is empty. */
@Test
public void testMTimeForTreeArtifactsDoesNotMatter() throws Exception {
// For this test, we only touch the input file.
@@ -291,9 +287,8 @@
buttonOne.pressed = buttonTwo.pressed = false;
touchFile(in);
buildArtifact(outTwo);
- // Per existing behavior, mtime matters for empty file Artifacts.
- assertTrue(buttonOne.pressed);
- // But this should be cached.
+ // mtime does not matter.
+ assertFalse(buttonOne.pressed);
assertFalse(buttonTwo.pressed);
// None of the below following should result in anything being built.
