From 37bd5f665aa614c6dc640c9d19852dd8d5efb0d8 Mon Sep 17 00:00:00 2001 From: felly Date: Tue, 14 Aug 2018 09:26:34 -0700 Subject: Automated rollback of commit 3bace1b937934fb2cea6260067ecc1cdbe526847. *** Reason for rollback *** b/112583337 RELNOTES: None *** Original change description *** Track Fileset in artifact expansion. PiperOrigin-RevId: 208658921 --- .../build/lib/actions/ActionExecutionContext.java | 17 ++--- .../devtools/build/lib/actions/Artifact.java | 9 --- .../build/lib/analysis/actions/SpawnAction.java | 2 +- .../lib/skyframe/ActionExecutionFunction.java | 73 ++++++++-------------- .../build/lib/skyframe/ActionInputMapHelper.java | 40 +----------- .../build/lib/skyframe/CompletionFunction.java | 12 +--- .../build/lib/skyframe/SkyframeActionExecutor.java | 18 ++---- 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 clientEnv; - private final ImmutableMap> topLevelFilesets; + private final ImmutableMap> + 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 clientEnv, - ImmutableMap> topLevelFilesets, + ImmutableMap> 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 clientEnv, - ImmutableMap> topLevelFilesets, + ImmutableMap> 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> getTopLevelFilesets() { - return topLevelFilesets; + public ImmutableMap> 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 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 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>> 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 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> topLevelFilesets = + ImmutableMap> 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> 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 CheckInputResults checkInputs( + private Pair>> checkInputs( Environment env, Action action, Map> 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> expandedArtifacts = new HashMap<>(populateInputData ? 128 : 0); - Map> expandedFilesets = new HashMap<>(); ActionExecutionException firstActionExecutionException = null; for (Map.Entry> @@ -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 filterKnownInputs( @@ -804,7 +782,6 @@ 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 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> expandedArtifacts, - Map> expandedFilesets, Artifact key, - SkyValue value, - Environment env) throws InterruptedException { + SkyValue value) { if (value instanceof AggregatingArtifactValue) { AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value; for (Pair entry : aggregatingValue.getFileArtifacts()) { - Artifact artifact = entry.first; - inputMap.put(artifact, entry.second); - if (artifact.isFileset()) { - ImmutableList expandedFileset = getFilesets(env, artifact); - if (expandedFileset != null) { - expandedFilesets.put(artifact, expandedFileset); - } - } + inputMap.put(entry.first, entry.second); } for (Pair entry : aggregatingValue.getTreeArtifacts()) { expandTreeArtifactAndPopulateArtifactData( @@ -84,26 +70,6 @@ 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 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> expandedArtifacts = null; - Map> 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> expandedInputs; - private final Map> expandedFilesets; - private ArtifactExpanderImpl(Map> expandedInputMiddlemen, - Map> expandedFilesets) { + private ArtifactExpanderImpl(Map> expandedInputMiddlemen) { this.expandedInputs = expandedInputMiddlemen; - this.expandedFilesets = expandedFilesets; } @Override @@ -510,12 +507,6 @@ public final class SkyframeActionExecutor { output.addAll(result); } } - - @Override - public ImmutableList 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> expandedInputs, - Map> expandedFilesets, - ImmutableMap> topLevelFilesets, + ImmutableMap> 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); } -- cgit v1.2.3