Automated rollback of commit 3bace1b937934fb2cea6260067ecc1cdbe526847.

*** Reason for rollback ***

b/112583337
RELNOTES: None

*** Original change description ***

Track Fileset in artifact expansion.

PiperOrigin-RevId: 208658921

7 files changed, 43 insertions, 128 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
index 7be226f7ff..44a79f9314 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
@@ -63,7 +63,8 @@ public class ActionExecutionContext implements Closeable {
   private final MetadataHandler metadataHandler;
   private final FileOutErr fileOutErr;
   private final ImmutableMap<String, String> clientEnv;
-  private final ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets;
+  private final ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>>
+      inputFilesetMappings;
   @Nullable private final ArtifactExpander artifactExpander;
   @Nullable private final Environment env;
 
@@ -82,7 +83,7 @@ public class ActionExecutionContext implements Closeable {
       MetadataHandler metadataHandler,
       FileOutErr fileOutErr,
       Map<String, String> clientEnv,
-      ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
+      ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> inputFilesetMappings,
       @Nullable ArtifactExpander artifactExpander,
       @Nullable SkyFunction.Environment env,
       @Nullable FileSystem actionFileSystem,
@@ -93,7 +94,7 @@ public class ActionExecutionContext implements Closeable {
     this.metadataHandler = metadataHandler;
     this.fileOutErr = fileOutErr;
     this.clientEnv = ImmutableMap.copyOf(clientEnv);
-    this.topLevelFilesets = topLevelFilesets;
+    this.inputFilesetMappings = inputFilesetMappings;
     this.executor = executor;
     this.artifactExpander = artifactExpander;
     this.env = env;
@@ -112,7 +113,7 @@ public class ActionExecutionContext implements Closeable {
       MetadataHandler metadataHandler,
       FileOutErr fileOutErr,
       Map<String, String> clientEnv,
-      ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
+      ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> inputFilesetMappings,
       ArtifactExpander artifactExpander,
       @Nullable FileSystem actionFileSystem,
       @Nullable Object skyframeDepsResult) {
@@ -124,7 +125,7 @@ public class ActionExecutionContext implements Closeable {
         metadataHandler,
         fileOutErr,
         clientEnv,
-        topLevelFilesets,
+        inputFilesetMappings,
         artifactExpander,
         /*env=*/ null,
         actionFileSystem,
@@ -228,8 +229,8 @@ public class ActionExecutionContext implements Closeable {
     return executor.getEventHandler();
   }
 
-  public ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> getTopLevelFilesets() {
-    return topLevelFilesets;
+  public ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> getInputFilesetMappings() {
+    return inputFilesetMappings;
   }
 
   @Nullable
@@ -325,7 +326,7 @@ public class ActionExecutionContext implements Closeable {
         metadataHandler,
         fileOutErr,
         clientEnv,
-        topLevelFilesets,
+        inputFilesetMappings,
         artifactExpander,
         env,
         actionFileSystem,
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index e48740b3a3..49dad3ff38 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -161,15 +161,6 @@ public class Artifact
      * Only aggregating middlemen and tree artifacts are expanded.
      */
     void expand(Artifact artifact, Collection<? super Artifact> output);
-
-    /**
-     * Retrieve the expansion of Filesets for the given artifact.
-     *
-     * @param artifact {@code artifact.isFileset()} must be true.
-     */
-    default ImmutableList<FilesetOutputSymlink> getFileset(Artifact artifact) {
-      throw new UnsupportedOperationException();
-    }
   }
 
   public static final ImmutableList<Artifact> NO_ARTIFACTS = ImmutableList.of();
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
index ce9eeeeb03..493f249532 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
@@ -356,7 +356,7 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
     return getSpawn(
         actionExecutionContext.getArtifactExpander(),
         actionExecutionContext.getClientEnv(),
-        actionExecutionContext.getTopLevelFilesets());
+        actionExecutionContext.getInputFilesetMappings());
   }
 
   Spawn getSpawn(
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 a199feb4f4..0905cf1bef 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
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.ActionInputMap;
 import com.google.devtools.build.lib.actions.ActionLookupData;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
 import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
@@ -47,6 +48,7 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.rules.cpp.IncludeScannable;
+import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -155,7 +157,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       env.getValues(state.allInputs.keysRequested);
       Preconditions.checkState(!env.valuesMissing(), "%s %s", action, state);
     }
-    CheckInputResults checkedInputs = null;
+    Pair<ActionInputMap, Map<Artifact, Collection<Artifact>>> checkedInputs = null;
     try {
       // Declare deps on known inputs to action. We do this unconditionally to maintain our
       // invariant of asking for the same deps each build.
@@ -197,14 +199,13 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
 
     if (checkedInputs != null) {
       Preconditions.checkState(!state.hasArtifactData(), "%s %s", state, action);
-      state.inputArtifactData = checkedInputs.actionInputMap;
-      state.expandedArtifacts = checkedInputs.expandedArtifacts;
-      state.expandedFilesets = checkedInputs.expandedFilesets;
+      state.inputArtifactData = checkedInputs.first;
+      state.expandedArtifacts = checkedInputs.second;
       if (skyframeActionExecutor.usesActionFileSystem()) {
         state.actionFileSystem =
             skyframeActionExecutor.createActionFileSystem(
                 directories.getRelativeOutputPath(),
-                checkedInputs.actionInputMap,
+                checkedInputs.first,
                 action.getOutputs());
       }
     }
@@ -468,24 +469,26 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
         continue;
       }
 
-      ImmutableList<FilesetOutputSymlink> mapping =
-          ActionInputMapHelper.getFilesets(env, actionInput);
-      if (mapping == null) {
+      ActionLookupKey filesetActionLookupKey = (ActionLookupKey) actionInput.getArtifactOwner();
+      // Index 0 for the Fileset ConfiguredTarget indicates the SkyframeFilesetManifestAction where
+      // we compute the fileset's outputSymlinks.
+      SkyKey filesetActionKey = ActionExecutionValue.key(filesetActionLookupKey, 0);
+      ActionExecutionValue filesetValue = (ActionExecutionValue) env.getValue(filesetActionKey);
+      if (filesetValue == null) {
+        // At this point skyframe does not guarantee that the filesetValue will be ready, since
+        // the current action does not directly depend on the outputs of the
+        // SkyframeFilesetManifestAction whose ActionExecutionValue (filesetValue) is needed here.
+        // TODO(kush): Get rid of this hack by making the outputSymlinks available in the Fileset
+        // artifact, which this action depends on, so its value will be guaranteed to be present.
         return null;
       }
-      filesetMappings.put(actionInput.getExecPath(), mapping);
+      filesetMappings.put(actionInput.getExecPath(), filesetValue.getOutputSymlinks());
     }
 
-    ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets =
+    ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> filesets =
         filesetMappings.build();
-
-    // Aggregate top-level Filesets with Filesets nested in Runfiles. Both should be used to update
-    // the FileSystem context.
-    state.expandedFilesets
-        .forEach((artifact, links) -> filesetMappings.put(artifact.getExecPath(), links));
     try {
-      state.updateFileSystemContext(skyframeActionExecutor, env, metadataHandler,
-          filesetMappings.build());
+      state.updateFileSystemContext(skyframeActionExecutor, env, metadataHandler, filesets);
     } catch (IOException e) {
       throw new ActionExecutionException(
           "Failed to update filesystem context: ", e, action, /*catastrophe=*/ false);
@@ -495,8 +498,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
             perActionFileCache,
             metadataHandler,
             Collections.unmodifiableMap(state.expandedArtifacts),
-            Collections.unmodifiableMap(state.expandedFilesets),
-            topLevelFilesets,
+            filesets,
             state.actionFileSystem,
             skyframeDepsResult)) {
       if (!state.hasExecutedAction()) {
@@ -647,32 +649,15 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     }
   }
 
-  private static class CheckInputResults {
-    /** Metadata about Artifacts consumed by this Action. */
-    private final ActionInputMap actionInputMap;
-    /** Artifact expansion mapping for Runfiles tree and tree artifacts. */
-    private final Map<Artifact, Collection<Artifact>> expandedArtifacts;
-    /** Artifact expansion mapping for Filesets embedded in Runfiles. */
-    private final Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets;
-
-    public CheckInputResults(ActionInputMap actionInputMap,
-        Map<Artifact, Collection<Artifact>> expandedArtifacts,
-        Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets) {
-      this.actionInputMap = actionInputMap;
-      this.expandedArtifacts = expandedArtifacts;
-      this.expandedFilesets = expandedFilesets;
-    }
-  }
-
   /**
    * Declare dependency on all known inputs of action. Throws exception if any are known to be
    * missing. Some inputs may not yet be in the graph, in which case the builder should abort.
    */
-  private CheckInputResults checkInputs(
+  private Pair<ActionInputMap, Map<Artifact, Collection<Artifact>>> checkInputs(
       Environment env,
       Action action,
       Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps)
-      throws ActionExecutionException, InterruptedException {
+      throws ActionExecutionException {
     int missingCount = 0;
     int actionFailures = 0;
     // Only populate input data if we have the input values, otherwise they'll just go unused.
@@ -684,7 +669,6 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     ActionInputMap inputArtifactData = new ActionInputMap(populateInputData ? inputDeps.size() : 0);
     Map<Artifact, Collection<Artifact>> expandedArtifacts =
         new HashMap<>(populateInputData ? 128 : 0);
-    Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets = new HashMap<>();
 
     ActionExecutionException firstActionExecutionException = null;
     for (Map.Entry<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>>
@@ -693,13 +677,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       try {
         SkyValue value = depsEntry.getValue().get();
         if (populateInputData) {
-          ActionInputMapHelper.addToMap(
-              inputArtifactData,
-              expandedArtifacts,
-              expandedFilesets,
-              input,
-              value,
-              env);
+          ActionInputMapHelper.addToMap(inputArtifactData, expandedArtifacts, input, value);
         }
       } catch (MissingInputFileException e) {
         missingCount++;
@@ -748,7 +726,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
           rootCauses.build(),
           /*catastrophe=*/ false);
     }
-    return new CheckInputResults(inputArtifactData, expandedArtifacts, expandedFilesets);
+    return Pair.of(inputArtifactData, expandedArtifacts);
   }
 
   private static Iterable<Artifact> filterKnownInputs(
@@ -804,7 +782,6 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
     ActionInputMap inputArtifactData = null;
 
     Map<Artifact, Collection<Artifact>> expandedArtifacts = null;
-    Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets = null;
     Token token = null;
     Iterable<Artifact> discoveredInputs = null;
     ActionExecutionValue value = null;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
index abc7208088..072c019be4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
@@ -17,39 +17,25 @@ import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionInputMap;
-import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
-import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
 import com.google.devtools.build.lib.util.Pair;
-import com.google.devtools.build.skyframe.SkyFunction.Environment;
-import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.util.Collection;
 import java.util.Map;
 
 class ActionInputMapHelper {
 
-  // Adds a value obtained by an Artifact skyvalue lookup to the action input map. May do Skyframe
-  // lookups.
+  // Adds a value obtained by an Artifact skyvalue lookup to the action input map
   static void addToMap(
       ActionInputMap inputMap,
       Map<Artifact, Collection<Artifact>> expandedArtifacts,
-      Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets,
       Artifact key,
-      SkyValue value,
-      Environment env) throws InterruptedException {
+      SkyValue value) {
     if (value instanceof AggregatingArtifactValue) {
       AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value;
       for (Pair<Artifact, FileArtifactValue> entry : aggregatingValue.getFileArtifacts()) {
-        Artifact artifact = entry.first;
-        inputMap.put(artifact, entry.second);
-        if (artifact.isFileset()) {
-          ImmutableList<FilesetOutputSymlink> expandedFileset = getFilesets(env, artifact);
-          if (expandedFileset != null) {
-            expandedFilesets.put(artifact, expandedFileset);
-          }
-        }
+        inputMap.put(entry.first, entry.second);
       }
       for (Pair<Artifact, TreeArtifactValue> entry : aggregatingValue.getTreeArtifacts()) {
         expandTreeArtifactAndPopulateArtifactData(
@@ -84,26 +70,6 @@ class ActionInputMapHelper {
     }
   }
 
-  static ImmutableList<FilesetOutputSymlink> getFilesets(Environment env,
-      Artifact actionInput) throws InterruptedException {
-    Preconditions.checkState(actionInput.isFileset(), actionInput);
-    ActionLookupKey filesetActionLookupKey = (ActionLookupKey) actionInput.getArtifactOwner();
-    // Index 0 for the Fileset ConfiguredTarget indicates the SkyframeFilesetManifestAction where
-    // we compute the fileset's outputSymlinks.
-    SkyKey filesetActionKey = ActionExecutionValue.key(filesetActionLookupKey, 0);
-    ActionExecutionValue filesetValue = (ActionExecutionValue) env.getValue(filesetActionKey);
-    if (filesetValue == null) {
-      // At this point skyframe does not guarantee that the filesetValue will be ready, since
-      // the current action does not directly depend on the outputs of the
-      // SkyframeFilesetManifestAction whose ActionExecutionValue (filesetValue) is needed here.
-      // TODO(kush): Get rid of this hack by making the outputSymlinks available in the Fileset
-      // artifact, which this action depends on, so its value will be guaranteed to be present.
-      // Also, unify handling of Fileset with Artifact expansion.
-      return null;
-    }
-    return filesetValue.getOutputSymlinks();
-  }
-
   private static void expandTreeArtifactAndPopulateArtifactData(
       Artifact treeArtifact,
       TreeArtifactValue value,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
index 237915aeb0..57a9d47206 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
@@ -13,13 +13,11 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.ActionInputMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.ArtifactSkyKey;
-import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
 import com.google.devtools.build.lib.actions.MissingInputFileException;
 import com.google.devtools.build.lib.analysis.AspectCompleteEvent;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -328,11 +326,9 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S
     boolean createPathResolver = pathResolverFactory.shouldCreatePathResolverForArtifactValues();
     ActionInputMap inputMap = null;
     Map<Artifact, Collection<Artifact>> expandedArtifacts = null;
-    Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets = null;
     if (createPathResolver) {
       inputMap = new ActionInputMap(inputDeps.size());
       expandedArtifacts = new HashMap<>();
-      expandedFilesets = new HashMap<>();
     }
 
     int missingCount = 0;
@@ -345,13 +341,7 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S
       try {
         SkyValue artifactValue = depsEntry.getValue().get();
         if (createPathResolver && artifactValue != null) {
-          ActionInputMapHelper.addToMap(
-              inputMap,
-              expandedArtifacts,
-              expandedFilesets,
-              input,
-              artifactValue,
-              env);
+          ActionInputMapHelper.addToMap(inputMap, expandedArtifacts, input, artifactValue);
         }
       } catch (MissingInputFileException e) {
         missingCount++;
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 fa5b35a55e..3dcb46114c 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
@@ -493,12 +493,9 @@ public final class SkyframeActionExecutor {
 
   private static class ArtifactExpanderImpl implements ArtifactExpander {
     private final Map<Artifact, Collection<Artifact>> expandedInputs;
-    private final Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets;
 
-    private ArtifactExpanderImpl(Map<Artifact, Collection<Artifact>> expandedInputMiddlemen,
-        Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets) {
+    private ArtifactExpanderImpl(Map<Artifact, Collection<Artifact>> expandedInputMiddlemen) {
       this.expandedInputs = expandedInputMiddlemen;
-      this.expandedFilesets = expandedFilesets;
     }
 
     @Override
@@ -510,12 +507,6 @@ public final class SkyframeActionExecutor {
         output.addAll(result);
       }
     }
-
-    @Override
-    public ImmutableList<FilesetOutputSymlink> getFileset(Artifact artifact) {
-      Preconditions.checkState(artifact.isFileset());
-      return Preconditions.checkNotNull(expandedFilesets.get(artifact));
-    }
   }
 
   /**
@@ -527,8 +518,7 @@ public final class SkyframeActionExecutor {
       MetadataProvider graphFileCache,
       MetadataHandler metadataHandler,
       Map<Artifact, Collection<Artifact>> expandedInputs,
-      Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets,
-      ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
+      ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> inputFilesetMappings,
       @Nullable FileSystem actionFileSystem,
       @Nullable Object skyframeDepsResult) {
     FileOutErr fileOutErr = actionLogBufferPathGenerator.generate(
@@ -541,8 +531,8 @@ public final class SkyframeActionExecutor {
         metadataHandler,
         fileOutErr,
         clientEnv,
-        topLevelFilesets,
-        new ArtifactExpanderImpl(expandedInputs, expandedFilesets),
+        inputFilesetMappings,
+        new ArtifactExpanderImpl(expandedInputs),
         actionFileSystem,
         skyframeDepsResult);
   }