Fix Fileset incrementality bug when Fileset consumes a generated file. The native skyframe implementation was actually quite incorrect in this case: It was adding a skyframe dependency on a FileValue for the output file. Without a transitive dependency on the source files and actions that determine the output file's state, this could never work (and explains why the incremental build would fail). Instead, we now depend on the Artifact corresponding to the output file instead.
This change updates the business logic RecursiveFilesystemTraversalFunction. This approach keeps the business logic of Fileset filesystem traversal centralized in RFTF.
To avoid making weird recursive Skyframe nodes in the output tree, we inline Skyframe dependencies and do direct filesystem operations over the output tree.
There are now three states we can be in when looking up a file:
1. Source file: As before, make a skyframe dep on the corresponding file
2. Top-level file of an output tree: Make a dep on the corresponding Artifact
3. Recursive file under an output directory: Do direct filesystem operations. It doesn't make sense to make Skyframe nodes corresponding to these files. In the future, I think we should consider failing fast on this case.
RELNOTES: None
PiperOrigin-RevId: 184556044
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
index 75e0da2..7569544b 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
@@ -23,6 +23,7 @@
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
import java.util.Set;
+import javax.annotation.Nullable;
/**
* Parameters of a filesystem traversal requested by a Fileset rule.
@@ -71,6 +72,13 @@
abstract class DirectTraversalRoot {
/**
+ * Returns the output Artifact corresponding to this traversal, if present. Only present when
+ * traversing a generated output.
+ */
+ @Nullable
+ public abstract Artifact getOutputArtifact();
+
+ /**
* Returns the root part of the full path.
*
* <p>This is typically the workspace root or some output tree's root (e.g. genfiles, binfiles).
@@ -94,15 +102,22 @@
@Override
public abstract int hashCode();
- static DirectTraversalRoot forPackage(Artifact buildFile) {
+ public static DirectTraversalRoot forPackage(Artifact buildFile) {
return new AutoValue_FilesetTraversalParams_DirectTraversalRoot(
+ null,
buildFile.getRoot().getRoot(), buildFile.getRootRelativePath().getParentDirectory());
}
- static DirectTraversalRoot forFileOrDirectory(Artifact fileOrDirectory) {
+ public static DirectTraversalRoot forFileOrDirectory(Artifact fileOrDirectory) {
return new AutoValue_FilesetTraversalParams_DirectTraversalRoot(
+ fileOrDirectory.isSourceArtifact() ? null : fileOrDirectory,
fileOrDirectory.getRoot().getRoot(), fileOrDirectory.getRootRelativePath());
}
+
+ public static DirectTraversalRoot forRootedPath(RootedPath newPath) {
+ return new AutoValue_FilesetTraversalParams_DirectTraversalRoot(null,
+ newPath.getRoot(), newPath.getRootRelativePath());
+ }
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java
index e003bdd..02f256f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java
@@ -238,7 +238,7 @@
Environment env, String errorInfo, DirectTraversal traversal)
throws MissingDepException, InterruptedException {
SkyKey depKey = RecursiveFilesystemTraversalValue.key(
- new RecursiveFilesystemTraversalValue.TraversalRequest(traversal.getRoot().asRootedPath(),
+ new RecursiveFilesystemTraversalValue.TraversalRequest(traversal.getRoot(),
traversal.isGenerated(), traversal.getPackageBoundaryMode(), traversal.isPackage(),
errorInfo));
RecursiveFilesystemTraversalValue v = (RecursiveFilesystemTraversalValue) env.getValue(depKey);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
index f6bdfc6..e46776d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
@@ -17,6 +17,7 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Verify;
import com.google.common.collect.Collections2;
+import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.events.Event;
@@ -24,9 +25,13 @@
import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.ResolvedFileFactory;
import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.TraversalRequest;
import com.google.devtools.build.lib.vfs.Dirent;
+import com.google.devtools.build.lib.vfs.FileStatus;
+import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
+import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
@@ -34,6 +39,8 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
@@ -58,7 +65,7 @@
String.format(
"Generated directory %s conflicts with package under the same path. "
+ "Additional info: %s",
- traversal.path.getRootRelativePath().getPathString(),
+ traversal.root.asRootedPath().getRootRelativePath().getPathString(),
traversal.errorInfo != null ? traversal.errorInfo : traversal.toString()));
}
}
@@ -126,14 +133,14 @@
if (!rootInfo.type.exists()) {
// May be a dangling symlink or a non-existent file. Handle gracefully.
if (rootInfo.type.isSymlink()) {
- return resultForDanglingSymlink(traversal.path, rootInfo);
+ return resultForDanglingSymlink(traversal.root.asRootedPath(), rootInfo);
} else {
return RecursiveFilesystemTraversalValue.EMPTY;
}
}
if (rootInfo.type.isFile()) {
- return resultForFileRoot(traversal.path, rootInfo);
+ return resultForFileRoot(traversal.root.asRootedPath(), rootInfo);
}
// Otherwise the root is a directory or a symlink to one.
@@ -150,7 +157,7 @@
String msg =
traversal.errorInfo
+ " crosses package boundary into package rooted at "
- + traversal.path.getRootRelativePath().getPathString();
+ + traversal.root.asRootedPath().getRootRelativePath().getPathString();
switch (traversal.crossPkgBoundaries) {
case CROSS:
// We are free to traverse the subpackage but we need to display a warning.
@@ -170,7 +177,8 @@
// We are free to traverse this directory.
Collection<SkyKey> dependentKeys = createRecursiveTraversalKeys(env, traversal);
- return resultForDirectory(traversal, rootInfo, traverseChildren(env, dependentKeys));
+ return resultForDirectory(traversal, rootInfo,
+ traverseChildren(env, dependentKeys, /*inline=*/traversal.isGenerated));
} catch (IOException e) {
throw new RecursiveFilesystemTraversalFunctionException(
new FileOperationException("Error while traversing fileset: " + e.getMessage()));
@@ -211,31 +219,82 @@
private static FileInfo lookUpFileInfo(Environment env, TraversalRequest traversal)
throws MissingDepException, IOException, InterruptedException {
- // Stat the file.
- FileValue fileValue =
- (FileValue) env.getValueOrThrow(FileValue.key(traversal.path), IOException.class);
+ if (traversal.isGenerated) {
+ byte[] digest = null;
+ if (traversal.root.getOutputArtifact() != null) {
+ Artifact artifact = traversal.root.getOutputArtifact();
+ SkyKey artifactKey = ArtifactSkyKey.key(artifact, true);
+ SkyValue value = env.getValue(artifactKey);
+ if (env.valuesMissing()) {
+ throw new MissingDepException();
+ }
- if (env.valuesMissing()) {
- throw new MissingDepException();
- }
- if (fileValue.exists()) {
- // If it exists, it may either be a symlink or a file/directory.
- PathFragment unresolvedLinkTarget = null;
- FileType type = null;
- if (fileValue.isSymlink()) {
- unresolvedLinkTarget = fileValue.getUnresolvedLinkTarget();
- type = fileValue.isDirectory() ? FileType.SYMLINK_TO_DIRECTORY : FileType.SYMLINK_TO_FILE;
- } else {
- type = fileValue.isDirectory() ? FileType.DIRECTORY : FileType.FILE;
+ if (value instanceof FileArtifactValue) {
+ FileArtifactValue fsVal = (FileArtifactValue) value;
+ digest = fsVal.getDigest();
+ } else {
+ return new FileInfo(
+ FileType.NONEXISTENT, null, null, null);
+ }
}
- return new FileInfo(type, fileValue.realFileStateValue(),
- fileValue.realRootedPath(), unresolvedLinkTarget);
+
+ // FileArtifactValue does not currently track symlinks. If it did, we could potentially remove
+ // some of the filesystem operations we're doing here.
+ Path path = traversal.root.asRootedPath().asPath();
+ FileStatus noFollowStat = path.stat(Symlinks.NOFOLLOW);
+ FileStatus followStat = path.statIfFound(Symlinks.FOLLOW);
+ FileType type;
+ PathFragment unresolvedLinkTarget = null;
+ RootedPath realPath = traversal.root.asRootedPath();
+ if (followStat == null) {
+ type = FileType.DANGLING_SYMLINK;
+ if (!noFollowStat.isSymbolicLink()) {
+ throw new IOException("Expected symlink for " + path + ", but got: " + noFollowStat);
+ }
+ unresolvedLinkTarget = path.readSymbolicLink();
+ } else if (noFollowStat.isFile()) {
+ type = FileType.FILE;
+ } else if (noFollowStat.isDirectory()) {
+ type = FileType.DIRECTORY;
+ } else {
+ unresolvedLinkTarget = path.readSymbolicLink();
+ realPath = RootedPath.toRootedPath(
+ Root.absoluteRoot(path.getFileSystem()),
+ path.resolveSymbolicLinks());
+ type = followStat.isFile() ? FileType.SYMLINK_TO_FILE : FileType.SYMLINK_TO_DIRECTORY;
+ }
+ return new FileInfo(type,
+ FileStateValue.createWithStatNoFollow(traversal.root.asRootedPath(),
+ new StatWithDigest(noFollowStat, digest), null),
+ realPath, unresolvedLinkTarget);
} else {
- // If it doesn't exist, or it's a dangling symlink, we still want to handle that gracefully.
- return new FileInfo(
- fileValue.isSymlink() ? FileType.DANGLING_SYMLINK : FileType.NONEXISTENT,
- fileValue.realFileStateValue(), null,
- fileValue.isSymlink() ? fileValue.getUnresolvedLinkTarget() : null);
+ // Stat the file.
+ FileValue fileValue =
+ (FileValue) env.getValueOrThrow(
+ FileValue.key(traversal.root.asRootedPath()), IOException.class);
+
+ if (env.valuesMissing()) {
+ throw new MissingDepException();
+ }
+ if (fileValue.exists()) {
+ // If it exists, it may either be a symlink or a file/directory.
+ PathFragment unresolvedLinkTarget = null;
+ FileType type;
+ if (fileValue.isSymlink()) {
+ unresolvedLinkTarget = fileValue.getUnresolvedLinkTarget();
+ type = fileValue.isDirectory() ? FileType.SYMLINK_TO_DIRECTORY : FileType.SYMLINK_TO_FILE;
+ } else {
+ type = fileValue.isDirectory() ? FileType.DIRECTORY : FileType.FILE;
+ }
+ return new FileInfo(type, fileValue.realFileStateValue(),
+ fileValue.realRootedPath(), unresolvedLinkTarget);
+ } else {
+ // If it doesn't exist, or it's a dangling symlink, we still want to handle that gracefully.
+ return new FileInfo(
+ fileValue.isSymlink() ? FileType.DANGLING_SYMLINK : FileType.NONEXISTENT,
+ fileValue.realFileStateValue(), null,
+ fileValue.isSymlink() ? fileValue.getUnresolvedLinkTarget() : null);
+ }
}
}
@@ -297,7 +356,8 @@
"{%s} {%s}", traversal, rootInfo);
PackageLookupValue pkgLookup =
(PackageLookupValue)
- getDependentSkyValue(env, PackageLookupValue.key(traversal.path.getRootRelativePath()));
+ getDependentSkyValue(env,
+ PackageLookupValue.key(traversal.root.asRootedPath().getRootRelativePath()));
if (pkgLookup.packageExists()) {
if (traversal.isGenerated) {
@@ -307,7 +367,7 @@
} else {
// The traversal's root was a source directory and it defines a package.
Root pkgRoot = pkgLookup.getRoot();
- if (!pkgRoot.equals(traversal.path.getRoot())) {
+ if (!pkgRoot.equals(traversal.root.asRootedPath().getRoot())) {
// However the root of this package is different from what we expected. stat() the real
// BUILD file of that package.
traversal = traversal.forChangedRootPath(pkgRoot);
@@ -330,18 +390,29 @@
*/
private static Collection<SkyKey> createRecursiveTraversalKeys(
Environment env, TraversalRequest traversal)
- throws MissingDepException, InterruptedException {
+ throws MissingDepException, InterruptedException, IOException {
// Use the traversal's path, even if it's a symlink. The contents of the directory, as listed
// in the result, must be relative to it.
- DirectoryListingValue dirListing = (DirectoryListingValue) getDependentSkyValue(env,
- DirectoryListingValue.key(traversal.path));
+ Iterable<Dirent> dirents;
+ if (traversal.isGenerated) {
+ // If we're dealing with an output file, read the directory directly instead of creating
+ // filesystem nodes under the output tree.
+ List<Dirent> direntsCollection =
+ new ArrayList<>(
+ traversal.root.asRootedPath().asPath().readdir(Symlinks.FOLLOW));
+ Collections.sort(direntsCollection);
+ dirents = direntsCollection;
+ } else {
+ dirents = ((DirectoryListingValue) getDependentSkyValue(env,
+ DirectoryListingValue.key(traversal.root.asRootedPath()))).getDirents();
+ }
List<SkyKey> result = new ArrayList<>();
- for (Dirent dirent : dirListing.getDirents()) {
+ for (Dirent dirent : dirents) {
RootedPath childPath =
RootedPath.toRootedPath(
- traversal.path.getRoot(),
- traversal.path.getRootRelativePath().getRelative(dirent.getName()));
+ traversal.root.asRootedPath().getRoot(),
+ traversal.root.asRootedPath().getRootRelativePath().getRelative(dirent.getName()));
TraversalRequest childTraversal = traversal.forChildEntry(childPath);
result.add(RecursiveFilesystemTraversalValue.key(childTraversal));
}
@@ -395,7 +466,7 @@
root =
ResolvedFileFactory.symlinkToDirectory(
rootInfo.realPath,
- traversal.path,
+ traversal.root.asRootedPath(),
rootInfo.unresolvedSymlinkTarget,
hashDirectorySymlink(children, rootInfo.metadata.hashCode()));
paths = NestedSetBuilder.<ResolvedFile>stableOrder().addTransitive(children).add(root);
@@ -434,12 +505,25 @@
*
* <p>The keys must all be {@link SkyFunctions#RECURSIVE_FILESYSTEM_TRAVERSAL} keys.
*/
- private static Collection<RecursiveFilesystemTraversalValue> traverseChildren(
- Environment env, Iterable<SkyKey> keys) throws MissingDepException, InterruptedException {
- Map<SkyKey, SkyValue> values = env.getValues(keys);
+ private Collection<RecursiveFilesystemTraversalValue> traverseChildren(
+ Environment env, Iterable<SkyKey> keys, boolean inline)
+ throws MissingDepException, InterruptedException,
+ RecursiveFilesystemTraversalFunctionException {
+ Map<SkyKey, SkyValue> values;
+ if (inline) {
+ // Don't create Skyframe nodes for a recursive traversal over the output tree.
+ // Instead, inline the recursion in the top-level request.
+ values = new HashMap<>();
+ for (SkyKey depKey : keys) {
+ values.put(depKey, compute(depKey, env));
+ }
+ } else {
+ values = env.getValues(keys);
+ }
if (env.valuesMissing()) {
throw new MissingDepException();
}
+
return Collections2.transform(values.values(),
new Function<SkyValue, RecursiveFilesystemTraversalValue>() {
@Override
@@ -505,4 +589,60 @@
boolean exists() { return false; }
@Override public abstract String toString();
}
+
+ private static class StatWithDigest implements FileStatusWithDigest {
+ private final FileStatus noFollowStat;
+ private final byte[] digest;
+
+ public StatWithDigest(FileStatus noFollowStat, byte[] digest) {
+ this.noFollowStat = noFollowStat;
+ this.digest = digest;
+ }
+
+ @Nullable
+ @Override
+ public byte[] getDigest() throws IOException {
+ return digest;
+ }
+
+ @Override
+ public boolean isFile() {
+ return noFollowStat.isFile();
+ }
+
+ @Override
+ public boolean isDirectory() {
+ return noFollowStat.isDirectory();
+ }
+
+ @Override
+ public boolean isSymbolicLink() {
+ return noFollowStat.isSymbolicLink();
+ }
+
+ @Override
+ public boolean isSpecialFile() {
+ return noFollowStat.isSpecialFile();
+ }
+
+ @Override
+ public long getSize() throws IOException {
+ return noFollowStat.getSize();
+ }
+
+ @Override
+ public long getLastModifiedTime() throws IOException {
+ return noFollowStat.getLastModifiedTime();
+ }
+
+ @Override
+ public long getLastChangeTime() throws IOException {
+ return noFollowStat.getLastChangeTime();
+ }
+
+ @Override
+ public long getNodeId() throws IOException {
+ return noFollowStat.getNodeId();
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java
index 247e546..7a1a269 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java
@@ -17,6 +17,7 @@
import com.google.common.base.Objects;
import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.actions.FilesetTraversalParams.DirectTraversalRoot;
import com.google.devtools.build.lib.actions.FilesetTraversalParams.PackageBoundaryMode;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -53,8 +54,7 @@
*/
public final class RecursiveFilesystemTraversalValue implements SkyValue {
static final RecursiveFilesystemTraversalValue EMPTY = new RecursiveFilesystemTraversalValue(
- Optional.<ResolvedFile>absent(),
- NestedSetBuilder.<ResolvedFile>emptySet(Order.STABLE_ORDER));
+ Optional.absent(), NestedSetBuilder.emptySet(Order.STABLE_ORDER));
/** The root of the traversal. May only be absent for the {@link #EMPTY} instance. */
private final Optional<ResolvedFile> resolvedRoot;
@@ -110,7 +110,7 @@
public static final class TraversalRequest {
/** The path to start the traversal from; may be a file, a directory or a symlink. */
- final RootedPath path;
+ final DirectTraversalRoot root;
/**
* Whether the path is in the output tree.
@@ -135,24 +135,25 @@
/** Information to be attached to any error messages that may be reported. */
@Nullable final String errorInfo;
- public TraversalRequest(RootedPath path, boolean isRootGenerated,
+ public TraversalRequest(DirectTraversalRoot root, boolean isRootGenerated,
PackageBoundaryMode crossPkgBoundaries, boolean skipTestingForSubpackage,
@Nullable String errorInfo) {
- this.path = path;
+ this.root = root;
this.isGenerated = isRootGenerated;
this.crossPkgBoundaries = crossPkgBoundaries;
this.skipTestingForSubpackage = skipTestingForSubpackage;
this.errorInfo = errorInfo;
}
- private TraversalRequest duplicate(RootedPath newRoot, boolean newSkipTestingForSubpackage) {
+ private TraversalRequest duplicate(DirectTraversalRoot newRoot,
+ boolean newSkipTestingForSubpackage) {
return new TraversalRequest(newRoot, isGenerated, crossPkgBoundaries,
newSkipTestingForSubpackage, errorInfo);
}
/** Creates a new request to traverse a child element in the current directory (the root). */
TraversalRequest forChildEntry(RootedPath newPath) {
- return duplicate(newPath, false);
+ return duplicate(DirectTraversalRoot.forRootedPath(newPath), false);
}
/**
@@ -163,7 +164,9 @@
*/
TraversalRequest forChangedRootPath(Root newRoot) {
return duplicate(
- RootedPath.toRootedPath(newRoot, path.getRootRelativePath()), skipTestingForSubpackage);
+ DirectTraversalRoot.forRootedPath(
+ RootedPath.toRootedPath(newRoot, root.asRootedPath().getRootRelativePath())),
+ skipTestingForSubpackage);
}
@Override
@@ -175,21 +178,21 @@
return false;
}
TraversalRequest o = (TraversalRequest) obj;
- return path.equals(o.path) && isGenerated == o.isGenerated
+ return root.equals(o.root) && isGenerated == o.isGenerated
&& crossPkgBoundaries == o.crossPkgBoundaries
&& skipTestingForSubpackage == o.skipTestingForSubpackage;
}
@Override
public int hashCode() {
- return Objects.hashCode(path, isGenerated, crossPkgBoundaries, skipTestingForSubpackage);
+ return Objects.hashCode(root, isGenerated, crossPkgBoundaries, skipTestingForSubpackage);
}
@Override
public String toString() {
return String.format(
"TraversalParams(root=%s, is_generated=%d, skip_testing_for_subpkg=%d,"
- + " pkg_boundaries=%s)", path, isGenerated ? 1 : 0,
+ + " pkg_boundaries=%s)", root, isGenerated ? 1 : 0,
skipTestingForSubpackage ? 1 : 0, crossPkgBoundaries);
}
}
@@ -660,7 +663,7 @@
* <p>The object stores things such as the absolute path of the file or symlink, its exact type
* and, if it's a symlink, the resolved and unresolved link target paths.
*/
- public static interface ResolvedFile {
+ public interface ResolvedFile {
/** Type of the entity under {@link #getPath()}. */
FileType getType();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
index 7002255..fbfeb1e 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
@@ -29,6 +29,7 @@
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactRoot;
+import com.google.devtools.build.lib.actions.FilesetTraversalParams.DirectTraversalRoot;
import com.google.devtools.build.lib.actions.FilesetTraversalParams.PackageBoundaryMode;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
@@ -37,13 +38,13 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.events.NullEventHandler;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
+import com.google.devtools.build.lib.skyframe.ArtifactSkyKey.OwnedArtifact;
import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalFunction.FileOperationException;
import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.ResolvedFile;
import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.TraversalRequest;
import com.google.devtools.build.lib.testutil.FoundationTestCase;
-import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
@@ -57,9 +58,12 @@
import com.google.devtools.build.skyframe.SequencedRecordingDifferencer;
import com.google.devtools.build.skyframe.SequentialBuildDriver;
import com.google.devtools.build.skyframe.SkyFunction;
+import com.google.devtools.build.skyframe.SkyFunctionException;
+import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
+import java.io.IOException;
import java.io.OutputStream;
import java.nio.charset.StandardCharsets;
import java.util.HashMap;
@@ -68,6 +72,7 @@
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nullable;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -81,10 +86,12 @@
private SequentialBuildDriver driver;
private RecordingDifferencer differencer;
private AtomicReference<PathPackageLocator> pkgLocator;
+ private ArtifactFakeFunction artifactFakeFunction;
@Before
public final void setUp() throws Exception {
AnalysisMock analysisMock = AnalysisMock.get();
+ artifactFakeFunction = new ArtifactFakeFunction();
pkgLocator =
new AtomicReference<>(
new PathPackageLocator(
@@ -104,7 +111,7 @@
ConfiguredRuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider();
Map<SkyFunctionName, SkyFunction> skyFunctions = new HashMap<>();
skyFunctions.put(SkyFunctions.FILE_STATE, new FileStateFunction(
- new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper));
+ new AtomicReference<>(), externalFilesHelper));
skyFunctions.put(SkyFunctions.FILE, new FileFunction(pkgLocator));
skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
skyFunctions.put(
@@ -140,6 +147,9 @@
new FileSymlinkInfiniteExpansionUniquenessFunction());
skyFunctions.put(
SkyFunctions.FILE_SYMLINK_CYCLE_UNIQUENESS, new FileSymlinkCycleUniquenessFunction());
+ skyFunctions.put(
+ SkyFunctions.ARTIFACT,
+ artifactFakeFunction);
progressReceiver = new RecordingEvaluationProgressReceiver();
differencer = new SequencedRecordingDifferencer();
@@ -226,13 +236,14 @@
}
private static TraversalRequest fileLikeRoot(Artifact file, PackageBoundaryMode pkgBoundaryMode) {
- return new TraversalRequest(
- rootedPath(file), !file.isSourceArtifact(), pkgBoundaryMode, false, null);
+ return new TraversalRequest(DirectTraversalRoot.forFileOrDirectory(file),
+ !file.isSourceArtifact(), pkgBoundaryMode, false, null);
}
private static TraversalRequest pkgRoot(
RootedPath pkgDirectory, PackageBoundaryMode pkgBoundaryMode) {
- return new TraversalRequest(pkgDirectory, false, pkgBoundaryMode, true, null);
+ return new TraversalRequest(DirectTraversalRoot.forRootedPath(pkgDirectory), false,
+ pkgBoundaryMode, true, null);
}
private <T extends SkyValue> EvaluationResult<T> eval(SkyKey key) throws Exception {
@@ -278,26 +289,39 @@
return result;
}
- private void appendToFile(RootedPath rootedPath, String content) throws Exception {
+ private void appendToFile(RootedPath rootedPath, SkyKey toInvalidate, String content)
+ throws Exception {
Path path = rootedPath.asPath();
if (path.exists()) {
try (OutputStream os = path.getOutputStream(/*append=*/ true)) {
os.write(content.getBytes(StandardCharsets.UTF_8));
}
- differencer.invalidate(ImmutableList.of(FileStateValue.key(rootedPath)));
+ differencer.invalidate(ImmutableList.of(toInvalidate));
} else {
createFile(path, content);
}
}
+ private void appendToFile(RootedPath rootedPath, String content) throws Exception {
+ appendToFile(rootedPath, FileStateValue.key(rootedPath), content);
+ }
+
private void appendToFile(Artifact file, String content) throws Exception {
- appendToFile(rootedPath(file), content);
+ SkyKey key = file.isSourceArtifact()
+ ? FileStateValue.key(rootedPath(file))
+ : ArtifactSkyKey.key(file, true);
+ appendToFile(rootedPath(file), key, content);
}
private void invalidateDirectory(RootedPath path) {
differencer.invalidate(ImmutableList.of(DirectoryListingStateValue.key(path)));
}
+ private void invalidateOutputArtifact(Artifact output) {
+ assertThat(output.isSourceArtifact()).isFalse();
+ differencer.invalidate(ImmutableList.of(ArtifactSkyKey.key(output, true)));
+ }
+
private void invalidateDirectory(Artifact directoryArtifact) {
invalidateDirectory(rootedPath(directoryArtifact));
}
@@ -461,7 +485,11 @@
// Add a new file to the directory and see that the value is rebuilt.
RootedPath file3 = createFile(childOf(directoryArtifact, "foo.txt"));
- invalidateDirectory(directoryArtifact);
+ if (directoryArtifact.isSourceArtifact()) {
+ invalidateDirectory(directoryArtifact);
+ } else {
+ invalidateOutputArtifact(directoryArtifact);
+ }
ResolvedFile expected3 = regularFileForTesting(file3);
RecursiveFilesystemTraversalValue v2 =
traverseAndAssertFiles(traversalRoot, expected1, expected2, expected3);
@@ -474,16 +502,23 @@
progressReceiver.clear();
// Edit a file in the directory and see that the value is rebuilt.
- appendToFile(file1, "bar");
- RecursiveFilesystemTraversalValue v3 =
- traverseAndAssertFiles(traversalRoot, expected1, expected2, expected3);
- assertThat(progressReceiver.invalidations).contains(rftvSkyKey(traversalRoot));
- assertThat(progressReceiver.evaluations).contains(v3);
- assertThat(v3).isNotEqualTo(v2);
- // Directories always have the same hash code, but that is fine because their contents are also
- // part of the RecursiveFilesystemTraversalValue, so v2 and v3 are unequal.
- assertTraversalRootHashesAreEqual(v2, v3);
- progressReceiver.clear();
+ RecursiveFilesystemTraversalValue v3;
+ if (directoryArtifact.isSourceArtifact()) {
+ SkyKey toInvalidate = FileStateValue.key(file1);
+ appendToFile(file1, toInvalidate, "bar");
+ v3 = traverseAndAssertFiles(traversalRoot, expected1, expected2, expected3);
+ assertThat(progressReceiver.invalidations).contains(rftvSkyKey(traversalRoot));
+ assertThat(progressReceiver.evaluations).contains(v3);
+ assertThat(v3).isNotEqualTo(v2);
+ // Directories always have the same hash code, but that is fine because their contents are
+ // also part of the RecursiveFilesystemTraversalValue, so v2 and v3 are unequal.
+ assertTraversalRootHashesAreEqual(v2, v3);
+ progressReceiver.clear();
+ } else {
+ // Dependency checking of output directories is unsound. Specifically, the directory mtime
+ // is not changed when a contained file is modified.
+ v3 = v2;
+ }
// Add a new file *outside* of the directory and see that the value is *not* rebuilt.
Artifact someFile = sourceArtifact("somewhere/else/a.file");
@@ -851,4 +886,25 @@
assertThat(error.getException()).isInstanceOf(FileOperationException.class);
assertThat(error.getException()).hasMessageThat().contains("Symlink cycle");
}
+
+ private static class ArtifactFakeFunction implements SkyFunction {
+ @Nullable
+ @Override
+ public SkyValue compute(SkyKey skyKey, Environment env)
+ throws SkyFunctionException, InterruptedException {
+ OwnedArtifact ownedArtifact = (OwnedArtifact) skyKey.argument();
+ Artifact artifact = ownedArtifact.getArtifact();
+ try {
+ return FileArtifactValue.create(artifact.getPath());
+ } catch (IOException e) {
+ throw new SkyFunctionException(e, Transience.PERSISTENT){};
+ }
+ }
+
+ @Nullable
+ @Override
+ public String extractTag(SkyKey skyKey) {
+ return null;
+ }
+ }
}
