Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Commit

Permalink
DefaultFileHashCache: invalidate children of directories
Browse files Browse the repository at this point in the history 
Summary:
When invalidating a directory, make sure to also invalidate any children
it has.  This fixes a bug where genrule's which generate output directories
don't properly invalidate all their outputs after they have been re-run.

Test Plan: added unittest

Reviewed By: k21

fb-gh-sync-id: e37b138
  • Loading branch information
@andrewjcg
andrewjcg authored and facebook-github-bot-3 committed Jan 22, 2016
1 parent cc398a3 commit 435cbc0
Show file tree
Hide file tree
Showing 7 changed files with 148 additions and 73 deletions.
43 changes: 24 additions & 19 deletions src/com/facebook/buck/hashing/PathHashing.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,48 +19,53 @@
import com.facebook.buck.io.MorePaths;
import com.facebook.buck.io.ProjectFilesystem;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.hash.Hasher;

import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Set;

public class PathHashing {
// Utility class, do not instantiate.
private PathHashing() { }

private static final Path EMPTY_PATH = Paths.get("");

/**
* Iterates recursively over all files under {@code paths}, sorts
* the filenames, and updates the {@link Hasher} with the names and
* contents of all files inside.
*/
public static void hashPaths(
public static ImmutableSet<Path> hashPath(
Hasher hasher,
FileHashLoader fileHashLoader,
ProjectFilesystem projectFilesystem,
Set<Path> paths) throws IOException {
Path root) throws IOException {
Preconditions.checkArgument(
!paths.contains(EMPTY_PATH),
"Paths to hash (%s) must not contain empty path",
paths
);
for (Path path : walkedPathsInSortedOrder(projectFilesystem, paths)) {
!root.equals(EMPTY_PATH),
"Path to hash (%s) must not be empty",
root);
ImmutableSet.Builder<Path> children = ImmutableSet.builder();
for (Path path : ImmutableSortedSet.copyOf(projectFilesystem.getFilesUnderPath(root))) {
StringHashing.hashStringAndLength(hasher, MorePaths.pathWithUnixSeparators(path));
if (!root.equals(path)) {
children.add(root.relativize(path));
}
hasher.putBytes(fileHashLoader.get(projectFilesystem.resolve(path)).asBytes());
}
return children.build();
}

private static ImmutableSortedSet<Path> walkedPathsInSortedOrder(
/**
* Iterates recursively over all files under {@code paths}, sorts
* the filenames, and updates the {@link Hasher} with the names and
* contents of all files inside.
*/
public static void hashPaths(
Hasher hasher,
FileHashLoader fileHashLoader,
ProjectFilesystem projectFilesystem,
Set<Path> pathsToWalk) throws IOException {
final ImmutableSortedSet.Builder<Path> walkedPaths = ImmutableSortedSet.naturalOrder();
for (Path pathToWalk : pathsToWalk) {
walkedPaths.addAll(projectFilesystem.getFilesUnderPath(pathToWalk));
ImmutableSortedSet<Path> paths) throws IOException {
for (Path path : paths) {
hashPath(hasher, fileHashLoader, projectFilesystem, path);
}
return walkedPaths.build();
}

}
7 changes: 6 additions & 1 deletion src/com/facebook/buck/rules/TargetGraphHashing.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.facebook.buck.util.HumanReadableException;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.hash.HashCode;
import com.google.common.hash.Hasher;
import com.google.common.hash.Hashing;
Expand Down Expand Up @@ -126,7 +127,11 @@ private void hashNode(final Hasher hasher, final TargetNode<?> node) throws IOEx
hasher.putBytes(targetRuleHashCode.asBytes());

// Hash the contents of all input files and directories.
PathHashing.hashPaths(hasher, fileHashLoader, projectFilesystem, node.getInputs());
PathHashing.hashPaths(
hasher,
fileHashLoader,
projectFilesystem,
ImmutableSortedSet.copyOf(node.getInputs()));

// We've already visited the dependencies (this is a depth-first traversal), so
// hash each dependency's build target and that build target's own hash.
Expand Down
44 changes: 35 additions & 9 deletions src/com/facebook/buck/util/cache/AbstractHashCodeAndFileType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,47 @@
package com.facebook.buck.util.cache;

import com.facebook.buck.util.immutables.BuckStyleImmutable;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.common.hash.HashCode;

import org.immutables.value.Value;

import java.nio.file.Path;

@Value.Immutable
@BuckStyleImmutable
interface AbstractHashCodeAndFileType {
static enum Type {
FILE,
DIRECTORY
};
abstract class AbstractHashCodeAndFileType {

public abstract Type getType();

public abstract HashCode getHashCode();

public abstract ImmutableSet<Path> getChildren();

@Value.Parameter
HashCode getHashCode();
@Value.Check
void check() {
Preconditions.checkState(getType() == Type.DIRECTORY || getChildren().isEmpty());
}

public static HashCodeAndFileType ofDirectory(HashCode hashCode, ImmutableSet<Path> children) {
return HashCodeAndFileType.builder()
.setType(Type.DIRECTORY)
.setGetHashCode(hashCode)
.setChildren(children)
.build();
}

public static HashCodeAndFileType ofFile(HashCode hashCode) {
return HashCodeAndFileType.builder()
.setType(Type.FILE)
.setGetHashCode(hashCode)
.build();
}

enum Type {
FILE,
DIRECTORY
};

@Value.Parameter
Type getType();
}
23 changes: 13 additions & 10 deletions src/com/facebook/buck/util/cache/DefaultFileHashCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,9 @@ public HashCodeAndFileType load(@Nonnull Path path) throws Exception {

private HashCodeAndFileType getHashCodeAndFileType(Path path) throws IOException {
if (projectFilesystem.isDirectory(path)) {
return HashCodeAndFileType.of(
getDirHashCode(path),
HashCodeAndFileType.Type.DIRECTORY);
return getDirHashCode(path);
} else {
return HashCodeAndFileType.of(
getFileHashCode(path),
HashCodeAndFileType.Type.FILE);
return HashCodeAndFileType.ofFile(getFileHashCode(path));
}
}

Expand All @@ -86,10 +82,11 @@ public InputStream openStream() throws IOException {
return source.hash(Hashing.sha1());
}

private HashCode getDirHashCode(Path path) throws IOException {
private HashCodeAndFileType getDirHashCode(Path path) throws IOException {
Hasher hasher = Hashing.sha1().newHasher();
PathHashing.hashPaths(hasher, this, projectFilesystem, ImmutableSet.of(path));
return hasher.hash();
ImmutableSet<Path> children =
PathHashing.hashPath(hasher, this, projectFilesystem, path);
return HashCodeAndFileType.ofDirectory(hasher.hash(), children);
}

@Override
Expand All @@ -105,7 +102,13 @@ public boolean willGet(Path path) {

@Override
public void invalidate(Path path) {
loadingCache.invalidate(path);
HashCodeAndFileType cached = loadingCache.getIfPresent(path);
if (cached != null) {
for (Path child : cached.getChildren()) {
loadingCache.invalidate(path.resolve(child));
}
loadingCache.invalidate(path);
}
}

@Override
Expand Down
44 changes: 34 additions & 10 deletions test/com/facebook/buck/hashing/PathHashingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import com.facebook.buck.util.cache.FileHashCache;
import com.facebook.buck.util.cache.NullFileHashCache;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.hash.HashCode;
import com.google.common.hash.Hasher;
import com.google.common.hash.Hashing;
Expand Down Expand Up @@ -65,7 +65,7 @@ public void emptyPathHasExpectedHash() throws IOException {
hasher,
new NullFileHashCache(),
emptyFilesystem,
ImmutableSet.<Path>of());
ImmutableSortedSet.<Path>of());
HashCode emptyStringHashCode = Hashing.sha1().newHasher().hash();
assertThat(
hasher.hash(),
Expand All @@ -82,10 +82,18 @@ public void sameContentsSameNameHaveSameHash() throws IOException {
filesystem2.touch(Paths.get("foo/bar.txt"));

Hasher hasher1 = Hashing.sha1().newHasher();
PathHashing.hashPaths(hasher1, fileHashCache, filesystem1, ImmutableSet.of(Paths.get("foo")));
PathHashing.hashPaths(
hasher1,
fileHashCache,
filesystem1,
ImmutableSortedSet.of(Paths.get("foo")));

Hasher hasher2 = Hashing.sha1().newHasher();
PathHashing.hashPaths(hasher2, fileHashCache, filesystem2, ImmutableSet.of(Paths.get("foo")));
PathHashing.hashPaths(
hasher2,
fileHashCache,
filesystem2,
ImmutableSortedSet.of(Paths.get("foo")));

assertThat(
hasher1.hash(),
Expand All @@ -102,10 +110,18 @@ public void sameContentsDifferentNameHaveDifferentHashes() throws IOException {
filesystem2.touch(Paths.get("foo/baz.txt"));

Hasher hasher1 = Hashing.sha1().newHasher();
PathHashing.hashPaths(hasher1, fileHashCache, filesystem1, ImmutableSet.of(Paths.get("foo")));
PathHashing.hashPaths(
hasher1,
fileHashCache,
filesystem1,
ImmutableSortedSet.of(Paths.get("foo")));

Hasher hasher2 = Hashing.sha1().newHasher();
PathHashing.hashPaths(hasher2, fileHashCache, filesystem2, ImmutableSet.of(Paths.get("foo")));
PathHashing.hashPaths(
hasher2,
fileHashCache,
filesystem2,
ImmutableSortedSet.of(Paths.get("foo")));

assertThat(
hasher1.hash(),
Expand All @@ -126,14 +142,14 @@ public void sameNameDifferentContentsHaveDifferentHashes() throws IOException {
hasher1,
fileHashCache,
filesystem1,
ImmutableSet.of(Paths.get("foo")));
ImmutableSortedSet.of(Paths.get("foo")));

Hasher hasher2 = Hashing.sha1().newHasher();
PathHashing.hashPaths(
hasher2,
modifiedFileHashCache,
filesystem2,
ImmutableSet.of(Paths.get("foo")));
ImmutableSortedSet.of(Paths.get("foo")));

assertThat(
hasher1.hash(),
Expand All @@ -154,10 +170,18 @@ public void hashDoesNotDependOnFilesystemIterationOrder() throws IOException {
filesystem2.touch(Paths.get("foo/foo.txt"));

Hasher hasher1 = Hashing.sha1().newHasher();
PathHashing.hashPaths(hasher1, fileHashCache, filesystem1, ImmutableSet.of(Paths.get("foo")));
PathHashing.hashPaths(
hasher1,
fileHashCache,
filesystem1,
ImmutableSortedSet.of(Paths.get("foo")));

Hasher hasher2 = Hashing.sha1().newHasher();
PathHashing.hashPaths(hasher2, fileHashCache, filesystem2, ImmutableSet.of(Paths.get("foo")));
PathHashing.hashPaths(
hasher2,
fileHashCache,
filesystem2,
ImmutableSortedSet.of(Paths.get("foo")));

assertThat(
hasher1.hash(),
Expand Down
38 changes: 29 additions & 9 deletions test/com/facebook/buck/util/cache/DefaultFileHashCacheTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

import com.facebook.buck.io.ProjectFilesystem;
Expand Down Expand Up @@ -48,9 +49,7 @@ public void whenPathIsPutCacheContainsPath() {
DefaultFileHashCache cache =
new DefaultFileHashCache(new FakeProjectFilesystem());
Path path = new File("SomeClass.java").toPath();
HashCodeAndFileType value = HashCodeAndFileType.of(
HashCode.fromInt(42),
HashCodeAndFileType.Type.FILE);
HashCodeAndFileType value = HashCodeAndFileType.ofFile(HashCode.fromInt(42));
cache.loadingCache.put(path, value);
assertTrue("Cache should contain path", cache.willGet(path));
}
Expand All @@ -60,9 +59,7 @@ public void whenPathIsPutPathGetReturnsHash() throws IOException {
DefaultFileHashCache cache =
new DefaultFileHashCache(new FakeProjectFilesystem());
Path path = new File("SomeClass.java").toPath();
HashCodeAndFileType value = HashCodeAndFileType.of(
HashCode.fromInt(42),
HashCodeAndFileType.Type.FILE);
HashCodeAndFileType value = HashCodeAndFileType.ofFile(HashCode.fromInt(42));
cache.loadingCache.put(path, value);
assertEquals("Cache should contain hash", value.getHashCode(), cache.get(path));
}
Expand All @@ -72,9 +69,7 @@ public void whenPathIsPutThenInvalidatedCacheDoesNotContainPath() {
DefaultFileHashCache cache =
new DefaultFileHashCache(new FakeProjectFilesystem());
Path path = new File("SomeClass.java").toPath();
HashCodeAndFileType value = HashCodeAndFileType.of(
HashCode.fromInt(42),
HashCodeAndFileType.Type.FILE);
HashCodeAndFileType value = HashCodeAndFileType.ofFile(HashCode.fromInt(42));
cache.loadingCache.put(path, value);
assertTrue("Cache should contain path", cache.willGet(path));
cache.invalidate(path);
Expand Down Expand Up @@ -119,4 +114,29 @@ public void whenPathsArePutThenInvalidateAllRemovesThem() throws IOException {

assertTrue(cache.loadingCache.asMap().isEmpty());
}

@Test
public void whenDirectoryIsPutThenInvalidatedCacheDoesNotContainPathOrChildren()
throws IOException {
ProjectFilesystem filesystem = new FakeProjectFilesystem();
DefaultFileHashCache cache = new DefaultFileHashCache(filesystem);

Path dir = filesystem.getRootPath().getFileSystem().getPath("dir");
filesystem.mkdirs(dir);
Path child1 = dir.resolve("child1");
filesystem.touch(child1);
Path child2 = dir.resolve("child2");
filesystem.touch(child2);

cache.get(dir);
assertTrue(cache.willGet(dir));
assertTrue(cache.willGet(child1));
assertTrue(cache.willGet(child2));

cache.invalidate(dir);
assertNull(cache.loadingCache.getIfPresent(dir));
assertNull(cache.loadingCache.getIfPresent(child1));
assertNull(cache.loadingCache.getIfPresent(child2));
}

}
Loading

0 comments on commit 435cbc0

Please sign in to comment.