diff options
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); } |