diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java index ed6889c1c611a3..f992f833628eea 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java +++ b/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 expandArtifacts( ArtifactExpander artifactExpander, boolean keepEmptyTreeArtifacts) { List result = new ArrayList<>(); + Set emptyTreeArtifacts = new TreeSet<>(); + Set 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; } 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 571cdc72a5969f..c5e79a90ee5962 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 @@ -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 /** * Adds an artifact to a collection, expanding it once if it's a middleman or tree artifact. * - *

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. + *

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 output, ArtifactExpander artifactExpander, - boolean keepEmptyTreeArtifacts) { + Set emptyTreeArtifacts) { if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) { List 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); 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 7c0f9e9fdcb6b3..62723330f9ccd8 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 @@ -1310,8 +1310,7 @@ private 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 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 8971039c3a0a69..515056c489094b 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 @@ -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> 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 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()); } } 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 9bd62c7d886f97..127fc19a374a99 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 @@ -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); } } } 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 df6b92fdb22a6d..42ad29b2091958 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 @@ -343,10 +343,6 @@ boolean useArchivedTreeArtifacts(ActionAnalysisMetadata action) { .test(action.getMnemonic()); } - boolean requiresTreeMetadataWhenTreeFileIsInput() { - return actionInputPrefetcher.requiresTreeMetadataWhenTreeFileIsInput(); - } - boolean publishTargetSummaries() { return options.getOptions(BuildEventProtocolOptions.class).publishTargetSummary; }