Always add containing trees of input tree file artifacts to the metad…

…ata map.

This happens for action templates.

The reason is that the Linux sandbox needs to know where the tree file artifact is materialized to, which information is stored in its parent.

The reason for making this a flag was performance, but it's only at most one tree artifact per action, it's a constant time operation per tree artifact and it only happens rarely (on templated actions) so I think this time, simplicity is better than performance.

The surprising change to ActionInputMapHelper was needed because before this change, the tree artifact was a discovered input and therefore was processed by the code path in ActionExecutionFunction.addDiscoveredInputs(), which populated archivedTreeArtifacts but addToMap() did not. It was presumably the bug.

The depOwner argument of putTreeArtifact may also be wrong there. It's at least inconsistent with the other two call sites, but I don't want to add more polish at some risk to this change so I decided to not add it to addArchivedTreeArtifactMaybe().

Progress towards #20753.

RELNOTES: None.
PiperOrigin-RevId: 596586625
Change-Id: Ib690a84508da07560ec376d4e87ed8fb2211979f

src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java

@@ -21,7 +21,10 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
 
 /** Helper utility to create ActionInput instances. */
 public final class ActionInputHelper {
@@ -122,14 +125,24 @@ public static List<ActionInput> expandArtifacts(
  ArtifactExpander artifactExpander,
  boolean keepEmptyTreeArtifacts) {
  List<ActionInput> result = new ArrayList<>();
+ Set<Artifact> emptyTreeArtifacts = new TreeSet<>();
+ Set<Artifact> treeFileArtifactParents = new HashSet<>();
  for (ActionInput input : inputs.toList()) {
  if (input instanceof Artifact) {
- Artifact.addExpandedArtifact(
- (Artifact) input, result, artifactExpander, keepEmptyTreeArtifacts);
+ Artifact inputArtifact = (Artifact) input;
+ Artifact.addExpandedArtifact(inputArtifact, result, artifactExpander, emptyTreeArtifacts);
+ if (inputArtifact.isChildOfDeclaredDirectory()) {
+ treeFileArtifactParents.add(inputArtifact.getParent());
+ }
  } else {
  result.add(input);
  }
  }
+
+ if (keepEmptyTreeArtifacts) {
+ emptyTreeArtifacts.removeAll(treeFileArtifactParents);
+ result.addAll(emptyTreeArtifacts);
+ }
  return result;
  }
 

src/main/java/com/google/devtools/build/lib/actions/Artifact.java

@@ -59,6 +59,7 @@
 import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.UnaryOperator;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
@@ -1544,21 +1545,20 @@ public static String joinRootRelativePaths(String delimiter, Iterable<Artifact>
  /**
  * Adds an artifact to a collection, expanding it once if it's a middleman or tree artifact.
  *
- * <p>A middleman artifact is never added to the collection. If {@code keepEmptyTreeArtifacts} is
- * true, a tree artifact will be added to the collection when it expands into zero file artifacts.
- * Otherwise, only the file artifacts the tree artifact expands into will be added.
+ * <p>The middleman or tree artifact is never added to the output collection. If a tree artifact
+ * expands into zero file artifacts, it is added to emptyTreeArtifacts.
  */
  static void addExpandedArtifact(
  Artifact artifact,
  Collection<? super Artifact> output,
  ArtifactExpander artifactExpander,
- boolean keepEmptyTreeArtifacts) {
+ Set<Artifact> emptyTreeArtifacts) {
  if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) {
  List<Artifact> expandedArtifacts = new ArrayList<>();
  artifactExpander.expand(artifact, expandedArtifacts);
  output.addAll(expandedArtifacts);
- if (keepEmptyTreeArtifacts && artifact.isTreeArtifact() && expandedArtifacts.isEmpty()) {
- output.add(artifact);
+ if (artifact.isTreeArtifact() && expandedArtifacts.isEmpty()) {
+ emptyTreeArtifacts.add(artifact);
  }
  } else {
  output.add(artifact);

src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java

@@ -1310,8 +1310,7 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(
  topLevelFilesets,
  input,
  value,
- env,
- skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput());
+ env);
  } else if (!hasMissingInputs && input.hasKnownGeneratingAction()) {
  // Derived inputs are mandatory, but we did not detect any missing inputs. This is only
  // possible for indirect inputs (beneath an ArtifactNestedSetKey) when, between the time the

src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java

@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
 import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.util.Map;
@@ -48,8 +49,7 @@ static void addToMap(
  Map<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets,
  Artifact key,
  SkyValue value,
- Environment env,
- boolean requiresTreeMetadataWhenTreeFileIsInput)
+ Environment env)
  throws InterruptedException {
  addToMap(
  inputMap,
@@ -60,8 +60,7 @@ static void addToMap(
  key,
  value,
  env,
- MetadataConsumerForMetrics.NO_OP,
- requiresTreeMetadataWhenTreeFileIsInput);
+ MetadataConsumerForMetrics.NO_OP);
  }
 
  /**
@@ -77,8 +76,7 @@ static void addToMap(
  Artifact key,
  SkyValue value,
  Environment env,
- MetadataConsumerForMetrics consumer,
- boolean requiresTreeMetadataWhenTreeFileIsInput)
+ MetadataConsumerForMetrics consumer)
  throws InterruptedException {
  if (value instanceof RunfilesArtifactValue) {
  // Note: we don't expand the .runfiles/MANIFEST file into the inputs. The reason for that
@@ -125,15 +123,17 @@ static void addToMap(
  /*depOwner=*/ key);
  consumer.accumulate(treeArtifactValue);
  } else if (value instanceof ActionExecutionValue) {
- if (requiresTreeMetadataWhenTreeFileIsInput && key.isChildOfDeclaredDirectory()) {
-  // Actions resulting from the expansion of an ActionTemplate consume only one of the files
-  // in a tree artifact. However, the input prefetcher requires access to the tree metadata
-  // to determine the prefetch location of a tree artifact materialized as a symlink
-  // (cf. TreeArtifactValue#getMaterializationExecPath()).
+ // Actions resulting from the expansion of an ActionTemplate consume only one of the files
+ // in a tree artifact. However, the input prefetcher and the Linux sandbox require access to
+ // the tree metadata to determine the prefetch location of a tree artifact materialized as a
+ // symlink (cf. TreeArtifactValue#getMaterializationExecPath()).
+ if (key.isChildOfDeclaredDirectory()) {
  SpecialArtifact treeArtifact = key.getParent();
  TreeArtifactValue treeArtifactValue =
  ((ActionExecutionValue) value).getTreeArtifactValue(treeArtifact);
  inputMap.putTreeArtifact(treeArtifact, treeArtifactValue, /* depOwner= */ treeArtifact);
+ addArchivedTreeArtifactMaybe(
+ treeArtifact, treeArtifactValue, archivedTreeArtifacts, inputMap, key);
  consumer.accumulate(treeArtifactValue);
  }
  FileArtifactValue metadata = ((ActionExecutionValue) value).getExistingFileArtifactValue(key);
@@ -221,20 +221,27 @@ private static void expandTreeArtifactAndPopulateArtifactData(
  return;
  }
 
- inputMap.putTreeArtifact((SpecialArtifact) treeArtifact, value, depOwner);
  expandedArtifacts.put(treeArtifact, value.getChildren());
+ inputMap.putTreeArtifact((SpecialArtifact) treeArtifact, value, depOwner);
+ addArchivedTreeArtifactMaybe(
+ (SpecialArtifact) treeArtifact, value, archivedTreeArtifacts, inputMap, depOwner);
+ }
+
+ private static void addArchivedTreeArtifactMaybe(
+ SpecialArtifact treeArtifact,
+ TreeArtifactValue value,
+ Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts,
+ ActionInputMapSink inputMap,
+ Artifact depOwner) {
+ if (!value.getArchivedRepresentation().isPresent()) {
+ return;
+ }
 
- value
- .getArchivedRepresentation()
- .ifPresent(
- archivedRepresentation -> {
- inputMap.put(
- archivedRepresentation.archivedTreeFileArtifact(),
- archivedRepresentation.archivedFileValue(),
- depOwner);
- archivedTreeArtifacts.put(
- (SpecialArtifact) treeArtifact,
- archivedRepresentation.archivedTreeFileArtifact());
- });
+ ArchivedRepresentation archivedRepresentation = value.getArchivedRepresentation().get();
+ inputMap.put(
+ archivedRepresentation.archivedTreeFileArtifact(),
+ archivedRepresentation.archivedFileValue(),
+ depOwner);
+ archivedTreeArtifacts.put(treeArtifact, archivedRepresentation.archivedTreeFileArtifact());
  }
 }

src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java

@@ -205,8 +205,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
  input,
  artifactValue,
  env,
- currentConsumer,
- skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput());
+ currentConsumer);
  if (!allArtifactsAreImportant && importantArtifactSet.contains(input)) {
  // Calling #addToMap a second time with `input` and `artifactValue` will perform no-op
  // updates to the secondary collections passed in (eg. expandedArtifacts,
@@ -220,8 +219,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
  topLevelFilesets,
  input,
  artifactValue,
- env,
- skyframeActionExecutor.requiresTreeMetadataWhenTreeFileIsInput());
+ env);
  }
  }
  }

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java

@@ -343,10 +343,6 @@ boolean useArchivedTreeArtifacts(ActionAnalysisMetadata action) {
  .test(action.getMnemonic());
  }
 
- boolean requiresTreeMetadataWhenTreeFileIsInput() {
- return actionInputPrefetcher.requiresTreeMetadataWhenTreeFileIsInput();
- }
-
  boolean publishTargetSummaries() {
  return options.getOptions(BuildEventProtocolOptions.class).publishTargetSummary;
  }