Rollback of commit a0eefb52f529b73c6cb92f0a762853646ea2eae6.
*** Reason for rollback ***
Rolling forward with the restored logic to avoid stat calls on injected Metadata.
*** Original change description ***
Automated [] rollback of commit df03e10f6552566982399b8779fe7bc7a17d75dc.
--
MOS_MIGRATED_REVID=114447944
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 3d845f6..0f2fb63 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
@@ -20,12 +20,14 @@
import com.google.common.collect.Sets;
import com.google.common.io.BaseEncoding;
import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactFile;
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;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.FileStatus;
@@ -321,12 +323,45 @@
return value;
}
- Set<ArtifactFile> contents = outputDirectoryListings.get(artifact);
- if (contents != null) {
- value = constructTreeArtifactValue(contents);
+ Set<ArtifactFile> registeredContents = outputDirectoryListings.get(artifact);
+ if (registeredContents != null) {
+ // Check that our registered outputs matches on-disk outputs. Only perform this check
+ // when contents were explicitly registered.
+ // TODO(bazel-team): Provide a way for actions to register empty TreeArtifacts.
+
+ // By the time we're constructing TreeArtifactValues, use of the metadata handler
+ // should be single threaded and there should be no race condition.
+ // The current design of ActionMetadataHandler makes this hard to enforce.
+ Set<PathFragment> paths = null;
+ try {
+ paths = TreeArtifactValue.explodeDirectory(artifact);
+ } catch (TreeArtifactException e) {
+ throw new IllegalStateException(e);
+ }
+ Set<ArtifactFile> diskFiles = ActionInputHelper.asArtifactFiles(artifact, paths);
+ if (!diskFiles.equals(registeredContents)) {
+ // There might be more than one error here. We first look for missing output files.
+ Set<ArtifactFile> missingFiles = Sets.difference(registeredContents, diskFiles);
+ if (!missingFiles.isEmpty()) {
+ // Don't throw IOException--getMetadataMaybe() eats them.
+ // TODO(bazel-team): Report this error in a better way when called by checkOutputs()
+ // Currently it's hard to report this error without refactoring, since checkOutputs()
+ // likes to substitute its own error messages upon catching IOException, and falls
+ // through to unrecoverable error behavior on any other exception.
+ throw new IllegalStateException("Output file " + missingFiles.iterator().next()
+ + " was registered, but not present on disk");
+ }
+
+ Set<ArtifactFile> extraFiles = Sets.difference(diskFiles, registeredContents);
+ // extraFiles cannot be empty
+ throw new IllegalStateException(
+ "File " + extraFiles.iterator().next().getParentRelativePath()
+ + ", present in TreeArtifact " + artifact + ", was not registered");
+ }
+
+ value = constructTreeArtifactValue(registeredContents);
} else {
- // Functionality is planned to construct the TreeArtifactValue from disk here.
- throw new UnsupportedOperationException();
+ value = constructTreeArtifactValueFromFilesystem(artifact);
}
TreeArtifactValue oldValue = outputTreeArtifactData.putIfAbsent(artifact, value);
@@ -361,6 +396,31 @@
return TreeArtifactValue.create(values);
}
+ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(Artifact artifact)
+ throws IOException {
+ Preconditions.checkState(artifact.isTreeArtifact(), artifact);
+
+ if (!artifact.getPath().isDirectory() || artifact.getPath().isSymbolicLink()) {
+ return TreeArtifactValue.MISSING_TREE_ARTIFACT;
+ }
+
+ Set<PathFragment> paths = null;
+ try {
+ paths = TreeArtifactValue.explodeDirectory(artifact);
+ } catch (TreeArtifactException e) {
+ throw new IllegalStateException(e);
+ }
+ // If you're reading tree artifacts from disk while outputDirectoryListings are being injected,
+ // something has gone terribly wrong.
+ Object previousDirectoryListing =
+ outputDirectoryListings.put(artifact,
+ Collections.newSetFromMap(new ConcurrentHashMap<ArtifactFile, Boolean>()));
+ Preconditions.checkState(previousDirectoryListing == null,
+ "Race condition while constructing TreArtifactValue: %s, %s",
+ artifact, previousDirectoryListing);
+ return constructTreeArtifactValue(ActionInputHelper.asArtifactFiles(artifact, paths));
+ }
+
@Override
public void addExpandedTreeOutput(ArtifactFile output) {
Preconditions.checkArgument(output.getParent().isTreeArtifact(),