Automated rollback of commit 37bd5f665aa614c6dc640c9d19852dd8d5efb0d8. *** Reason for rollback *** Rollforward with fix: Don't use Immutable map builder to agggregate Filesets. We may reach the same one via direct and indirect runfiles, so we need to allow for duplicate entries. *** Original change description *** Automated rollback of commit 3bace1b937934fb2cea6260067ecc1cdbe526847. *** Reason for rollback *** b/112583337 RELNOTES: None *** Original change description *** Track Fileset in artifact expansion. PiperOrigin-RevId: 208748163
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 44a79f9..7be226f 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,8 +63,7 @@ private final MetadataHandler metadataHandler; private final FileOutErr fileOutErr; private final ImmutableMap<String, String> clientEnv; - private final ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> - inputFilesetMappings; + private final ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets; @Nullable private final ArtifactExpander artifactExpander; @Nullable private final Environment env; @@ -83,7 +82,7 @@ MetadataHandler metadataHandler, FileOutErr fileOutErr, Map<String, String> clientEnv, - ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> inputFilesetMappings, + ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets, @Nullable ArtifactExpander artifactExpander, @Nullable SkyFunction.Environment env, @Nullable FileSystem actionFileSystem, @@ -94,7 +93,7 @@ this.metadataHandler = metadataHandler; this.fileOutErr = fileOutErr; this.clientEnv = ImmutableMap.copyOf(clientEnv); - this.inputFilesetMappings = inputFilesetMappings; + this.topLevelFilesets = topLevelFilesets; this.executor = executor; this.artifactExpander = artifactExpander; this.env = env; @@ -113,7 +112,7 @@ MetadataHandler metadataHandler, FileOutErr fileOutErr, Map<String, String> clientEnv, - ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> inputFilesetMappings, + ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets, ArtifactExpander artifactExpander, @Nullable FileSystem actionFileSystem, @Nullable Object skyframeDepsResult) { @@ -125,7 +124,7 @@ metadataHandler, fileOutErr, clientEnv, - inputFilesetMappings, + topLevelFilesets, artifactExpander, /*env=*/ null, actionFileSystem, @@ -229,8 +228,8 @@ return executor.getEventHandler(); } - public ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> getInputFilesetMappings() { - return inputFilesetMappings; + public ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> getTopLevelFilesets() { + return topLevelFilesets; } @Nullable @@ -326,7 +325,7 @@ metadataHandler, fileOutErr, clientEnv, - inputFilesetMappings, + topLevelFilesets, 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 49dad3f..e48740b 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,6 +161,15 @@ * 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 493f249..ce9eeee 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 @@ return getSpawn( actionExecutionContext.getArtifactExpander(), actionExecutionContext.getClientEnv(), - actionExecutionContext.getInputFilesetMappings()); + actionExecutionContext.getTopLevelFilesets()); } 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 0905cf1..f465201 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,7 +28,6 @@ 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; @@ -48,7 +47,6 @@ 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; @@ -157,7 +155,7 @@ env.getValues(state.allInputs.keysRequested); Preconditions.checkState(!env.valuesMissing(), "%s %s", action, state); } - Pair<ActionInputMap, Map<Artifact, Collection<Artifact>>> checkedInputs = null; + CheckInputResults 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. @@ -199,13 +197,14 @@ if (checkedInputs != null) { Preconditions.checkState(!state.hasArtifactData(), "%s %s", state, action); - state.inputArtifactData = checkedInputs.first; - state.expandedArtifacts = checkedInputs.second; + state.inputArtifactData = checkedInputs.actionInputMap; + state.expandedArtifacts = checkedInputs.expandedArtifacts; + state.expandedFilesets = checkedInputs.expandedFilesets; if (skyframeActionExecutor.usesActionFileSystem()) { state.actionFileSystem = skyframeActionExecutor.createActionFileSystem( directories.getRelativeOutputPath(), - checkedInputs.first, + checkedInputs.actionInputMap, action.getOutputs()); } } @@ -462,33 +461,32 @@ metadataHandler.discardOutputMetadata(); } - ImmutableMap.Builder<PathFragment, ImmutableList<FilesetOutputSymlink>> filesetMappings = - ImmutableMap.builder(); + // Make sure this is a regular HashMap rather than ImmutableMapBuilder so that we are safe + // in case of collisions. + Map<PathFragment, ImmutableList<FilesetOutputSymlink>> filesetMappings = new HashMap<>(); for (Artifact actionInput : action.getInputs()) { if (!actionInput.isFileset()) { continue; } - 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. + ImmutableList<FilesetOutputSymlink> mapping = + ActionInputMapHelper.getFilesets(env, actionInput); + if (mapping == null) { return null; } - filesetMappings.put(actionInput.getExecPath(), filesetValue.getOutputSymlinks()); + filesetMappings.put(actionInput.getExecPath(), mapping); } - ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> filesets = - filesetMappings.build(); + ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets = + ImmutableMap.copyOf(filesetMappings); + + // 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, filesets); + state.updateFileSystemContext(skyframeActionExecutor, env, metadataHandler, + ImmutableMap.copyOf(filesetMappings)); } catch (IOException e) { throw new ActionExecutionException( "Failed to update filesystem context: ", e, action, /*catastrophe=*/ false); @@ -498,7 +496,8 @@ perActionFileCache, metadataHandler, Collections.unmodifiableMap(state.expandedArtifacts), - filesets, + Collections.unmodifiableMap(state.expandedFilesets), + topLevelFilesets, state.actionFileSystem, skyframeDepsResult)) { if (!state.hasExecutedAction()) { @@ -649,15 +648,32 @@ } } + 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 Pair<ActionInputMap, Map<Artifact, Collection<Artifact>>> checkInputs( + private CheckInputResults checkInputs( Environment env, Action action, Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps) - throws ActionExecutionException { + throws ActionExecutionException, InterruptedException { int missingCount = 0; int actionFailures = 0; // Only populate input data if we have the input values, otherwise they'll just go unused. @@ -669,6 +685,7 @@ 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>> @@ -677,7 +694,13 @@ try { SkyValue value = depsEntry.getValue().get(); if (populateInputData) { - ActionInputMapHelper.addToMap(inputArtifactData, expandedArtifacts, input, value); + ActionInputMapHelper.addToMap( + inputArtifactData, + expandedArtifacts, + expandedFilesets, + input, + value, + env); } } catch (MissingInputFileException e) { missingCount++; @@ -726,7 +749,7 @@ rootCauses.build(), /*catastrophe=*/ false); } - return Pair.of(inputArtifactData, expandedArtifacts); + return new CheckInputResults(inputArtifactData, expandedArtifacts, expandedFilesets); } private static Iterable<Artifact> filterKnownInputs( @@ -782,6 +805,7 @@ 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 072c019..abc7208 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,25 +17,39 @@ 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 + // Adds a value obtained by an Artifact skyvalue lookup to the action input map. May do Skyframe + // lookups. static void addToMap( ActionInputMap inputMap, Map<Artifact, Collection<Artifact>> expandedArtifacts, + Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets, Artifact key, - SkyValue value) { + SkyValue value, + Environment env) throws InterruptedException { if (value instanceof AggregatingArtifactValue) { AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value; for (Pair<Artifact, FileArtifactValue> entry : aggregatingValue.getFileArtifacts()) { - inputMap.put(entry.first, entry.second); + 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); + } + } } for (Pair<Artifact, TreeArtifactValue> entry : aggregatingValue.getTreeArtifacts()) { expandTreeArtifactAndPopulateArtifactData( @@ -70,6 +84,26 @@ } } + 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 57a9d47..237915a 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,11 +13,13 @@ // 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; @@ -326,9 +328,11 @@ 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; @@ -341,7 +345,13 @@ try { SkyValue artifactValue = depsEntry.getValue().get(); if (createPathResolver && artifactValue != null) { - ActionInputMapHelper.addToMap(inputMap, expandedArtifacts, input, artifactValue); + ActionInputMapHelper.addToMap( + inputMap, + expandedArtifacts, + expandedFilesets, + input, + artifactValue, + env); } } 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 3dcb461..fa5b35a 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,9 +493,12 @@ 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) { + private ArtifactExpanderImpl(Map<Artifact, Collection<Artifact>> expandedInputMiddlemen, + Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets) { this.expandedInputs = expandedInputMiddlemen; + this.expandedFilesets = expandedFilesets; } @Override @@ -507,6 +510,12 @@ output.addAll(result); } } + + @Override + public ImmutableList<FilesetOutputSymlink> getFileset(Artifact artifact) { + Preconditions.checkState(artifact.isFileset()); + return Preconditions.checkNotNull(expandedFilesets.get(artifact)); + } } /** @@ -518,7 +527,8 @@ MetadataProvider graphFileCache, MetadataHandler metadataHandler, Map<Artifact, Collection<Artifact>> expandedInputs, - ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> inputFilesetMappings, + Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets, + ImmutableMap<PathFragment, ImmutableList<FilesetOutputSymlink>> topLevelFilesets, @Nullable FileSystem actionFileSystem, @Nullable Object skyframeDepsResult) { FileOutErr fileOutErr = actionLogBufferPathGenerator.generate( @@ -531,8 +541,8 @@ metadataHandler, fileOutErr, clientEnv, - inputFilesetMappings, - new ArtifactExpanderImpl(expandedInputs), + topLevelFilesets, + new ArtifactExpanderImpl(expandedInputs, expandedFilesets), actionFileSystem, skyframeDepsResult); }