Make unpredictable action inputs and outputs available to Actions.

--
MOS_MIGRATED_REVID=114249806

3 files changed, 145 insertions, 43 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 0a210149d1..d45d282ac3 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -15,8 +15,7 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Function;
 import com.google.common.base.Predicates;
-import com.google.common.collect.Collections2;
-import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
@@ -25,6 +24,7 @@ import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionCacheChecker.Token;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
+import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFile;
@@ -133,8 +133,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       // invariant of asking for the same deps each build.
       Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps
           = env.getValuesOrThrow(toKeys(state.allInputs.getAllInputs(),
-                  action.discoversInputs() ? action.getMandatoryInputs() : null),
-              MissingInputFileException.class, ActionExecutionException.class);
+              action.discoversInputs() ? action.getMandatoryInputs() : null),
+          MissingInputFileException.class, ActionExecutionException.class);
 
       if (!sharedActionAlreadyRan && !state.hasArtifactData()) {
         // Do we actually need to find our metadata?
@@ -281,8 +281,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
 
       Map<SkyKey,
           ValueOrException2<NoSuchPackageException, InconsistentFilesystemException>> values =
-              env.getValuesOrThrow(depKeys.values(), NoSuchPackageException.class,
-                  InconsistentFilesystemException.class);
+          env.getValuesOrThrow(depKeys.values(), NoSuchPackageException.class,
+              InconsistentFilesystemException.class);
       // Check values even if some are missing so that we can throw an appropriate exception if
       // needed.
 
@@ -337,7 +337,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       // We got a hit from the action cache -- no need to execute.
       return new ActionExecutionValue(
           metadataHandler.getOutputArtifactFileData(),
-          ImmutableMap.<Artifact, TreeArtifactValue>of(),
+          metadataHandler.getOutputTreeArtifactData(),
           metadataHandler.getAdditionalOutputData());
     }
 
@@ -446,6 +446,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
   private static Map<Artifact, FileArtifactValue> addDiscoveredInputs(
       Map<Artifact, FileArtifactValue> originalInputData, Collection<Artifact> discoveredInputs,
       Environment env) {
+    // We assume nobody would want to discover a TreeArtifact, since TreeArtifacts are precisely
+    // for undiscoverable contents.
     Map<Artifact, FileArtifactValue> result = new HashMap<>(originalInputData);
     Set<SkyKey> keys = new HashSet<>();
     for (Artifact artifact : discoveredInputs) {
@@ -542,25 +544,30 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       Artifact input = ArtifactValue.artifact(depsEntry.getKey());
       try {
         ArtifactValue value = (ArtifactValue) depsEntry.getValue().get();
-        if (populateInputData && value instanceof AggregatingArtifactValue) {
-          AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value;
-          for (Pair<Artifact, FileArtifactValue> entry : aggregatingValue.getInputs()) {
-            inputArtifactData.put(entry.first, entry.second);
+        if (populateInputData) {
+          if (value instanceof AggregatingArtifactValue) {
+            AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value;
+            for (Pair<Artifact, FileArtifactValue> entry : aggregatingValue.getInputs()) {
+              inputArtifactData.put(entry.first, entry.second);
+            }
+            // We have to cache the "digest" of the aggregating value itself,
+            // because the action cache checker may want it.
+            inputArtifactData.put(input, aggregatingValue.getSelfData());
+            ImmutableList.Builder<ArtifactFile> expansionBuilder = ImmutableList.builder();
+            for (Pair<Artifact, FileArtifactValue> pair : aggregatingValue.getInputs()) {
+              expansionBuilder.add(pair.first);
+            }
+            expandedArtifacts.put(input, expansionBuilder.build());
+          } else if (value instanceof TreeArtifactValue) {
+            TreeArtifactValue setValue = (TreeArtifactValue) value;
+            expandedArtifacts.put(input, ActionInputHelper.asArtifactFiles(
+                    input, setValue.getChildPaths()));
+            // Again, we cache the "digest" of the value for cache checking.
+            inputArtifactData.put(input, setValue.getSelfData());
+          } else if (value instanceof FileArtifactValue) {
+            // TODO(bazel-team): Make sure middleman "virtual" artifact data is properly processed.
+            inputArtifactData.put(input, (FileArtifactValue) value);
           }
-          // We have to cache the "digest" of the aggregating value itself, because the action cache
-          // checker may want it.
-          inputArtifactData.put(input, aggregatingValue.getSelfData());
-          expandedArtifacts.put(input,
-              Collections2.transform(aggregatingValue.getInputs(),
-                  new Function<Pair<Artifact, FileArtifactValue>, ArtifactFile>() {
-                    @Override
-                    public ArtifactFile apply(Pair<Artifact, FileArtifactValue> pair) {
-                      return pair.first;
-                    }
-                  }));
-        } else if (populateInputData && value instanceof FileArtifactValue) {
-          // TODO(bazel-team): Make sure middleman "virtual" artifact data is properly processed.
-          inputArtifactData.put(input, (FileArtifactValue) value);
         }
       } catch (MissingInputFileException e) {
         missingCount++;
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 3d845f6858..0f2fb63fdc 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.Maps;
 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 @@ public class ActionMetadataHandler implements MetadataHandler {
       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 @@ public class ActionMetadataHandler implements MetadataHandler {
     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(),
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index 39adae3b33..fb90b58bf0 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -32,6 +32,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionStatusReporter;
 import com.google.devtools.build.lib.actions.ActionGraph;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
+import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator;
 import com.google.devtools.build.lib.actions.ActionMiddlemanEvent;
 import com.google.devtools.build.lib.actions.ActionStartedEvent;
@@ -439,7 +440,8 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
 
     @Override
     public void expand(Artifact artifact, Collection<? super ArtifactFile> output) {
-      Preconditions.checkState(artifact.isMiddlemanArtifact(), artifact);
+      Preconditions.checkState(artifact.isMiddlemanArtifact() || artifact.isTreeArtifact(),
+          artifact);
       Collection<ArtifactFile> result = expandedInputs.get(artifact);
       // Note that result may be null for non-aggregating middlemen.
       if (result != null) {
@@ -610,14 +612,12 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
         }
         statusReporterRef.get().setPreparing(action);
 
-        createOutputDirectories(action);
-
         Preconditions.checkState(actionExecutionContext.getMetadataHandler() == metadataHandler,
             "%s %s", actionExecutionContext.getMetadataHandler(), metadataHandler);
         prepareScheduleExecuteAndCompleteAction(action, actionExecutionContext, actionStartTime);
         return new ActionExecutionValue(
             metadataHandler.getOutputArtifactFileData(),
-            ImmutableMap.<Artifact, TreeArtifactValue>of(),
+            metadataHandler.getOutputTreeArtifactData(),
             metadataHandler.getAdditionalOutputData());
       } finally {
         profiler.completeTask(ProfilerTask.ACTION);
@@ -629,7 +629,13 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
     try {
       Set<Path> done = new HashSet<>(); // avoid redundant calls for the same directory.
       for (Artifact outputFile : action.getOutputs()) {
-        Path outputDir = outputFile.getPath().getParentDirectory();
+        Path outputDir;
+        if (outputFile.isTreeArtifact()) {
+          outputDir = outputFile.getPath();
+        } else {
+          outputDir = outputFile.getPath().getParentDirectory();
+        }
+
         if (done.add(outputDir)) {
           try {
             createDirectoryAndParents(outputDir);
@@ -700,6 +706,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
     // the action really does produce the outputs.
     try {
       action.prepare(context.getExecutor().getExecRoot());
+      createOutputDirectories(action);
     } catch (IOException e) {
       reportError("failed to delete output files before executing action", e, action, null);
     }
@@ -842,6 +849,35 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
     }
   }
 
+  // TODO TODOTIJDSO: maybe just set directories r/o and executable always
+  private static void setPathReadOnlyAndExecutable(MetadataHandler metadataHandler,
+      ArtifactFile file)
+      throws IOException {
+    Path path = file.getPath();
+    if (path.isFile(Symlinks.NOFOLLOW)) { // i.e. regular files only.
+      // We trust the files created by the execution-engine to be non symlinks with expected
+      // chmod() settings already applied.
+      if (!metadataHandler.isInjected(file)) {
+        path.chmod(0555);  // Sets the file read-only and executable.
+      }
+    }
+  }
+
+  private static void setTreeReadOnlyAndExecutable(MetadataHandler metadataHandler, Artifact parent,
+      PathFragment subpath) throws IOException {
+    Path path = parent.getPath().getRelative(subpath);
+    if (path.isDirectory()) {
+      path.chmod(0555);
+      for (Path child : path.getDirectoryEntries()) {
+        setTreeReadOnlyAndExecutable(metadataHandler, parent,
+            subpath.getChild(child.getBaseName()));
+      }
+    } else {
+      setPathReadOnlyAndExecutable(
+          metadataHandler, ActionInputHelper.artifactFile(parent, subpath));
+    }
+  }
+
   /**
    * For each of the action's outputs that is a regular file (not a symbolic
    * link or directory), make it read-only and executable.
@@ -863,14 +899,13 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
     Preconditions.checkState(!action.getActionType().isMiddleman());
 
     for (Artifact output : action.getOutputs()) {
-      Path path = output.getPath();
-      if (metadataHandler.isInjected(output)) {
-        // We trust the files created by the execution-engine to be non symlinks with expected
-        // chmod() settings already applied.
-        continue;
-      }
-      if (path.isFile(Symlinks.NOFOLLOW)) { // i.e. regular files only.
-        path.chmod(0555);  // Sets the file read-only and executable.
+      if (output.isTreeArtifact()) {
+        // Preserve existing behavior: we don't set non-TreeArtifact directories
+        // read only and executable. However, it's unusual for non-TreeArtifact outputs
+        // to be directories.
+        setTreeReadOnlyAndExecutable(metadataHandler, output, PathFragment.EMPTY_FRAGMENT);
+      } else {
+        setPathReadOnlyAndExecutable(metadataHandler, output);
       }
     }
   }