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

Commit

Permalink
Make sure we log what we set in the RuleKeyBuilder
Browse files Browse the repository at this point in the history
Summary:
Turns out that absolute paths were being properly encoded in the rulekey,
but not being reported correctly in the logs. yay.

Test Plan:
Run multiple builds from separate locations and watch rule keys match.
buck test --all
  • Loading branch information
shs96c authored and sdwilsh committed Nov 11, 2015
1 parent 5dbe934 commit fab3812
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 18 deletions.
51 changes: 37 additions & 14 deletions src/com/facebook/buck/rules/RuleKeyBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,22 @@ protected RuleKeyBuilder setSourcePath(SourcePath sourcePath) {
return setSingleValue(buildRule.get());
} else {
// The original version of this expected the path to be relative, however, sometimes the
// deprecated method returned an absolute path, which is obviously less than ideal. Maintain
// the old behaviour until we root out the places where things aren't working as expected.
Path path = resolver.deprecatedGetPath(sourcePath);
return setSingleValue(path);
// deprecated method returned an absolute path, which is obviously less than ideal. If we can,
// grab the relative path to the output. We also need to hash the contents of the absolute
// path no matter what.
Path ideallyRelative;
try {
ideallyRelative = resolver.getRelativePath(sourcePath);
} catch (IllegalStateException e) {
// Expected relative path was absolute. Yay.
ideallyRelative = resolver.getAbsolutePath(sourcePath);
}
Path absolutePath = resolver.getAbsolutePath(sourcePath);
try {
return setPath(absolutePath, ideallyRelative);
} catch (IOException e) {
throw new RuntimeException(e);
}
}
}

Expand Down Expand Up @@ -211,26 +223,37 @@ public RuleKeyBuilder setReflectively(String key, @Nullable Object val) {
}
}

public RuleKeyBuilder setPath(Path path) throws IOException {
return setPath(path, path);
}

// Paths get added as a combination of the file name and file hash. If the path is absolute
// then we only include the file name (assuming that it represents a tool of some kind
// that's being used for compilation or some such). This does mean that if a user renames a
// file without changing the contents, we have a cache miss. We're going to assume that this
// doesn't happen that often in practice.
public RuleKeyBuilder setPath(Path path) throws IOException {
HashCode sha1 = hashCache.get(path);
private RuleKeyBuilder setPath(Path absolutePath, Path ideallyRelative) throws IOException {
// TODO(simons): Enable this precondition once setPath(Path) has been removed.
// Preconditions.checkState(absolutePath.isAbsolute());
HashCode sha1 = hashCache.get(absolutePath);
if (sha1 == null) {
throw new RuntimeException("No SHA for " + path);
}
if (logElms != null) {
logElms.add(String.format("path(%s:%s):", path, sha1));
throw new RuntimeException("No SHA for " + absolutePath);
}
if (path.isAbsolute()) {

Path addToKey;
if (ideallyRelative.isAbsolute()) {
logger.warn(
"Attempting to add absolute path to rule key. Only using file name: %s", path);
feed(path.getFileName().toString().getBytes());
"Attempting to add absolute path to rule key. Only using file name: %s", ideallyRelative);
addToKey = ideallyRelative.getFileName();
} else {
feed(path.toString().getBytes());
addToKey = ideallyRelative;
}

if (logElms != null) {
logElms.add(String.format("path(%s:%s):", addToKey, sha1));
}

feed(addToKey.toString().getBytes());
feed(sha1.toString().getBytes());
return this;
}
Expand Down
21 changes: 17 additions & 4 deletions test/com/facebook/buck/testutil/FakeFileHashCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,16 @@

package com.facebook.buck.testutil;

import com.facebook.buck.io.ProjectFilesystem;
import com.facebook.buck.util.cache.FileHashCache;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.common.hash.HashCode;

import java.io.IOException;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.HashMap;
import java.util.Map;

/**
Expand All @@ -39,11 +40,23 @@ public FakeFileHashCache(Map<Path, HashCode> pathsToHashes) {
}

public static FakeFileHashCache createFromStrings(Map<String, String> pathsToHashes) {
ImmutableMap.Builder<Path, HashCode> builder = ImmutableMap.builder();
return createFromStrings(new FakeProjectFilesystem(), pathsToHashes);
}

public static FakeFileHashCache createFromStrings(
ProjectFilesystem filesystem,
Map<String, String> pathsToHashes) {
Map<Path, HashCode> cachedValues = new HashMap<>();
for (Map.Entry<String, String> entry : pathsToHashes.entrySet()) {
builder.put(Paths.get(entry.getKey()), HashCode.fromString(entry.getValue()));
// Retain the original behaviour
cachedValues.put(Paths.get(entry.getKey()), HashCode.fromString(entry.getValue()));

// And ensure that the absolute path is also present.
if (!entry.getKey().startsWith("/")) {
cachedValues.put(filesystem.resolve(entry.getKey()), HashCode.fromString(entry.getValue()));
}
}
return new FakeFileHashCache(builder.build());
return new FakeFileHashCache(cachedValues);
}

@Override
Expand Down

0 comments on commit fab3812

Please sign in to comment.