From 8dece49bed76b5f372ff5d3a3f25a32b2a9c1f52 Mon Sep 17 00:00:00 2001 From: felly Date: Tue, 14 Aug 2018 17:53:30 -0700 Subject: 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 --- .../build/lib/actions/ActionExecutionContext.java | 17 +++-- .../devtools/build/lib/actions/Artifact.java | 9 +++ .../build/lib/analysis/actions/SpawnAction.java | 2 +- .../lib/skyframe/ActionExecutionFunction.java | 80 ++++++++++++++-------- .../build/lib/skyframe/ActionInputMapHelper.java | 40 ++++++++++- .../build/lib/skyframe/CompletionFunction.java | 12 +++- .../build/lib/skyframe/SkyframeActionExecutor.java | 18 +++-- 7 files changed, 132 insertions(+), 46 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 44a79f9314..7be226f7ff 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 @@ public class ActionExecutionContext implements Closeable { private final MetadataHandler metadataHandler; private final FileOutErr fileOutErr; private final ImmutableMap clientEnv; - private final ImmutableMap> - inputFilesetMappings; + private final ImmutableMap> topLevelFilesets; @Nullable private final ArtifactExpander artifactExpander; @Nullable private final Environment env; @@ -83,7 +82,7 @@ public class ActionExecutionContext implements Closeable { MetadataHandler metadataHandler, FileOutErr fileOutErr, Map clientEnv, - ImmutableMap> inputFilesetMappings, + ImmutableMap> topLevelFilesets, @Nullable ArtifactExpander artifactExpander, @Nullable SkyFunction.Environment env, @Nullable FileSystem actionFileSystem, @@ -94,7 +93,7 @@ public class ActionExecutionContext implements Closeable { 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 @@ public class ActionExecutionContext implements Closeable { MetadataHandler metadataHandler, FileOutErr fileOutErr, Map clientEnv, - ImmutableMap> inputFilesetMappings, + ImmutableMap> topLevelFilesets, ArtifactExpander artifactExpander, @Nullable FileSystem actionFileSystem, @Nullable Object skyframeDepsResult) { @@ -125,7 +124,7 @@ public class ActionExecutionContext implements Closeable { metadataHandler, fileOutErr, clientEnv, - inputFilesetMappings, + topLevelFilesets, artifactExpander, /*env=*/ null, actionFileSystem, @@ -229,8 +228,8 @@ public class ActionExecutionContext implements Closeable { return executor.getEventHandler(); } - public ImmutableMap> getInputFilesetMappings() { - return inputFilesetMappings; + public ImmutableMap> getTopLevelFilesets() { + return topLevelFilesets; } @Nullable @@ -326,7 +325,7 @@ public class ActionExecutionContext implements Closeable { 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 49dad3ff38..e48740b3a3 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 @@ public class Artifact * Only aggregating middlemen and tree artifacts are expanded. */ void expand(Artifact artifact, Collection output); + + /** + * Retrieve the expansion of Filesets for the given artifact. + * + * @param artifact {@code artifact.isFileset()} must be true. + */ + default ImmutableList getFileset(Artifact artifact) { + throw new UnsupportedOperationException(); + } } public static final ImmutableList 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 493f249532..ce9eeeeb03 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.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 0905cf1bef..f465201da3 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.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; @@ -48,7 +47,6 @@ 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; @@ -157,7 +155,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver env.getValues(state.allInputs.keysRequested); Preconditions.checkState(!env.valuesMissing(), "%s %s", action, state); } - Pair>> 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 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver 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 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver metadataHandler.discardOutputMetadata(); } - ImmutableMap.Builder> filesetMappings = - ImmutableMap.builder(); + // Make sure this is a regular HashMap rather than ImmutableMapBuilder so that we are safe + // in case of collisions. + Map> 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 mapping = + ActionInputMapHelper.getFilesets(env, actionInput); + if (mapping == null) { return null; } - filesetMappings.put(actionInput.getExecPath(), filesetValue.getOutputSymlinks()); + filesetMappings.put(actionInput.getExecPath(), mapping); } - ImmutableMap> filesets = - filesetMappings.build(); + ImmutableMap> 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 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver perActionFileCache, metadataHandler, Collections.unmodifiableMap(state.expandedArtifacts), - filesets, + Collections.unmodifiableMap(state.expandedFilesets), + topLevelFilesets, state.actionFileSystem, skyframeDepsResult)) { if (!state.hasExecutedAction()) { @@ -649,15 +648,32 @@ 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> expandedArtifacts; + /** Artifact expansion mapping for Filesets embedded in Runfiles. */ + private final Map> expandedFilesets; + + public CheckInputResults(ActionInputMap actionInputMap, + Map> expandedArtifacts, + Map> 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>> checkInputs( + private CheckInputResults checkInputs( Environment env, Action action, Map> 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 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver ActionInputMap inputArtifactData = new ActionInputMap(populateInputData ? inputDeps.size() : 0); Map> expandedArtifacts = new HashMap<>(populateInputData ? 128 : 0); + Map> expandedFilesets = new HashMap<>(); ActionExecutionException firstActionExecutionException = null; for (Map.Entry> @@ -677,7 +694,13 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver 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 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver rootCauses.build(), /*catastrophe=*/ false); } - return Pair.of(inputArtifactData, expandedArtifacts); + return new CheckInputResults(inputArtifactData, expandedArtifacts, expandedFilesets); } private static Iterable filterKnownInputs( @@ -782,6 +805,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver ActionInputMap inputArtifactData = null; Map> expandedArtifacts = null; + Map> expandedFilesets = null; Token token = null; Iterable 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 072c019be4..abc7208088 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.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 + // Adds a value obtained by an Artifact skyvalue lookup to the action input map. May do Skyframe + // lookups. static void addToMap( ActionInputMap inputMap, Map> expandedArtifacts, + Map> expandedFilesets, Artifact key, - SkyValue value) { + SkyValue value, + Environment env) throws InterruptedException { if (value instanceof AggregatingArtifactValue) { AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value; for (Pair entry : aggregatingValue.getFileArtifacts()) { - inputMap.put(entry.first, entry.second); + Artifact artifact = entry.first; + inputMap.put(artifact, entry.second); + if (artifact.isFileset()) { + ImmutableList expandedFileset = getFilesets(env, artifact); + if (expandedFileset != null) { + expandedFilesets.put(artifact, expandedFileset); + } + } } for (Pair entry : aggregatingValue.getTreeArtifacts()) { expandTreeArtifactAndPopulateArtifactData( @@ -70,6 +84,26 @@ class ActionInputMapHelper { } } + static ImmutableList 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 57a9d47206..237915aeb0 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 @@ public final class CompletionFunction> expandedArtifacts = null; + Map> expandedFilesets = null; if (createPathResolver) { inputMap = new ActionInputMap(inputDeps.size()); expandedArtifacts = new HashMap<>(); + expandedFilesets = new HashMap<>(); } int missingCount = 0; @@ -341,7 +345,13 @@ public final class CompletionFunction> expandedInputs; + private final Map> expandedFilesets; - private ArtifactExpanderImpl(Map> expandedInputMiddlemen) { + private ArtifactExpanderImpl(Map> expandedInputMiddlemen, + Map> expandedFilesets) { this.expandedInputs = expandedInputMiddlemen; + this.expandedFilesets = expandedFilesets; } @Override @@ -507,6 +510,12 @@ public final class SkyframeActionExecutor { output.addAll(result); } } + + @Override + public ImmutableList getFileset(Artifact artifact) { + Preconditions.checkState(artifact.isFileset()); + return Preconditions.checkNotNull(expandedFilesets.get(artifact)); + } } /** @@ -518,7 +527,8 @@ public final class SkyframeActionExecutor { MetadataProvider graphFileCache, MetadataHandler metadataHandler, Map> expandedInputs, - ImmutableMap> inputFilesetMappings, + Map> expandedFilesets, + ImmutableMap> topLevelFilesets, @Nullable FileSystem actionFileSystem, @Nullable Object skyframeDepsResult) { FileOutErr fileOutErr = actionLogBufferPathGenerator.generate( @@ -531,8 +541,8 @@ public final class SkyframeActionExecutor { metadataHandler, fileOutErr, clientEnv, - inputFilesetMappings, - new ArtifactExpanderImpl(expandedInputs), + topLevelFilesets, + new ArtifactExpanderImpl(expandedInputs, expandedFilesets), actionFileSystem, skyframeDepsResult); } -- cgit v1.2.3