From 2b3befd9746ffc8ed1a6f02bf260a91282fa489e Mon Sep 17 00:00:00 2001 From: felly Date: Fri, 10 Aug 2018 10:37:56 -0700 Subject: Automated rollback of commit 9374ecf94ce44e8bc56e68678cb512abf4cf9ce2. *** Reason for rollback *** Rollforward with deduplication of traversals and regression test. RELNOTES: None *** Original change description *** Automated rollback of commit 39974a43abdd32e3a1acbc7da945b08da9983e4e. *** Reason for rollback *** b/112458627 *** Original change description *** Allow skyframe-aware actions to pass partial results through ActionExecutionContext. Remove FilesetProvider. PiperOrigin-RevId: 208231719 --- .../build/lib/actions/ActionExecutionContext.java | 22 +- .../build/lib/actions/FilesetTraversalParams.java | 9 +- .../lib/actions/FilesetTraversalParamsFactory.java | 33 +-- .../lib/analysis/ConfiguredTargetFactory.java | 11 - .../devtools/build/lib/analysis/RuleContext.java | 6 +- .../configuredtargets/FileConfiguredTarget.java | 4 - .../FilesetOutputConfiguredTarget.java | 84 ------- .../lib/analysis/fileset/FilesetProvider.java | 30 --- .../lib/skyframe/ActionExecutionFunction.java | 17 +- .../build/lib/skyframe/FilesetEntryFunction.java | 248 +++++++++------------ .../build/lib/skyframe/SkyframeActionExecutor.java | 6 +- .../build/lib/skyframe/SkyframeAwareAction.java | 5 +- .../lib/actions/ExecutableSymlinkActionTest.java | 1 + .../build/lib/actions/util/ActionsTestUtil.java | 6 +- .../analysis/actions/FileWriteActionTestCase.java | 1 + .../analysis/actions/ParamFileWriteActionTest.java | 3 +- .../lib/analysis/actions/SymlinkActionTest.java | 3 +- .../actions/TemplateExpansionActionTest.java | 3 +- .../build/lib/analysis/util/BuildViewTestCase.java | 3 +- .../lib/rules/cpp/CreateIncSymlinkActionTest.java | 3 +- .../build/lib/rules/cpp/LtoBackendActionTest.java | 3 +- .../lib/rules/objc/BazelJ2ObjcLibraryTest.java | 2 + .../lib/skyframe/FilesetEntryFunctionTest.java | 184 +-------------- .../lib/skyframe/SkyframeAwareActionTest.java | 6 +- .../standalone/StandaloneSpawnStrategyTest.java | 3 +- 25 files changed, 192 insertions(+), 504 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FilesetOutputConfiguredTarget.java delete mode 100644 src/main/java/com/google/devtools/build/lib/analysis/fileset/FilesetProvider.java 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 037f47b1ad..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 @@ -69,6 +69,7 @@ public class ActionExecutionContext implements Closeable { @Nullable private final Environment env; @Nullable private final FileSystem actionFileSystem; + @Nullable private final Object skyframeDepsResult; @Nullable private ImmutableList outputSymlinks; @@ -85,7 +86,8 @@ public class ActionExecutionContext implements Closeable { ImmutableMap> inputFilesetMappings, @Nullable ArtifactExpander artifactExpander, @Nullable SkyFunction.Environment env, - @Nullable FileSystem actionFileSystem) { + @Nullable FileSystem actionFileSystem, + @Nullable Object skyframeDepsResult) { this.actionInputFileCache = actionInputFileCache; this.actionInputPrefetcher = actionInputPrefetcher; this.actionKeyContext = actionKeyContext; @@ -97,6 +99,7 @@ public class ActionExecutionContext implements Closeable { this.artifactExpander = artifactExpander; this.env = env; this.actionFileSystem = actionFileSystem; + this.skyframeDepsResult = skyframeDepsResult; this.pathResolver = ArtifactPathResolver.createPathResolver(actionFileSystem, // executor is only ever null in testing. executor == null ? null : executor.getExecRoot()); @@ -112,7 +115,8 @@ public class ActionExecutionContext implements Closeable { Map clientEnv, ImmutableMap> inputFilesetMappings, ArtifactExpander artifactExpander, - @Nullable FileSystem actionFileSystem) { + @Nullable FileSystem actionFileSystem, + @Nullable Object skyframeDepsResult) { this( executor, actionInputFileCache, @@ -124,7 +128,8 @@ public class ActionExecutionContext implements Closeable { inputFilesetMappings, artifactExpander, /*env=*/ null, - actionFileSystem); + actionFileSystem, + skyframeDepsResult); } public static ActionExecutionContext forInputDiscovery( @@ -148,7 +153,8 @@ public class ActionExecutionContext implements Closeable { ImmutableMap.of(), /*artifactExpander=*/ null, env, - actionFileSystem); + actionFileSystem, + /*skyframeDepsResult=*/ null); } public ActionInputPrefetcher getActionInputPrefetcher() { @@ -278,6 +284,11 @@ public class ActionExecutionContext implements Closeable { return artifactExpander; } + @Nullable + public Object getSkyframeDepsResult() { + return skyframeDepsResult; + } + /** * Provide that {@code FileOutErr} that the action should use for redirecting the output and error * stream. @@ -318,6 +329,7 @@ public class ActionExecutionContext implements Closeable { inputFilesetMappings, artifactExpander, env, - actionFileSystem); + actionFileSystem, + skyframeDepsResult); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java index 75907d4f24..e7cbe12c2c 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.actions; import com.google.auto.value.AutoValue; import com.google.auto.value.extension.memoized.Memoized; import com.google.common.base.Optional; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSortedSet; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; @@ -280,14 +279,14 @@ public interface FilesetTraversalParams { Optional getDirectTraversal(); /** - * Returns the parameters of the nested traversal request, if any. + * Returns the Fileset Artifact of the nested traversal request, if any. * *

A nested traversal is the traversal of another Fileset referenced by FilesetEntry.srcdir. * - *

The value is non-empty when {@link #getDirectTraversal} is absent AND the nested Fileset has - * non-empty FilesetEntries. + *

The value is null when {@link #getDirectTraversal} is absent. */ - ImmutableList getNestedTraversal(); + @Nullable + Artifact getNestedArtifact(); /** Adds the fingerprint of this traversal object. */ void fingerprint(Fingerprint fp); diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java index f9585f34df..6fd7cfa8d5 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java @@ -17,9 +17,7 @@ import com.google.auto.value.AutoValue; import com.google.auto.value.extension.memoized.Memoized; import com.google.common.base.Optional; import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSortedSet; -import com.google.common.collect.Iterables; import com.google.common.collect.Ordering; import com.google.devtools.build.lib.actions.FilesetTraversalParams.DirectTraversalRoot; import com.google.devtools.build.lib.actions.FilesetTraversalParams.PackageBoundaryMode; @@ -110,13 +108,11 @@ public final class FilesetTraversalParamsFactory { } /** - * Creates traversal request parameters for a FilesetEntry wrapping another Fileset. If possible, - * the original {@code nested} is returned to avoid unnecessary object creation. In that case, the - * {@code ownerLabelForErrorMessages} may be ignored. Since the wrapping traversal could not have - * an error on its own, any error messages printed will still be correct. + * Creates traversal request parameters for a FilesetEntry wrapping another Fileset. * * @param ownerLabel the rule that created this object - * @param nested the list of traversal params that were used for the nested (inner) Fileset + * @param artifact the Fileset Artifact of traversal params that were used for the nested (inner) + * Fileset * @param destPath path in the Fileset's output directory that will be the root of files coming * from the nested Fileset * @param excludes optional; set of files directly below (not in a subdirectory of) the nested @@ -124,16 +120,11 @@ public final class FilesetTraversalParamsFactory { */ public static FilesetTraversalParams nestedTraversal( Label ownerLabel, - ImmutableList nested, + Artifact artifact, PathFragment destPath, @Nullable Set excludes) { - if (nested.size() == 1 && destPath.isEmpty() && (excludes == null || excludes.isEmpty())) { - // Wrapping the traversal here would not lead to a different result: the output location is - // the same and there are no additional excludes. - return Iterables.getOnlyElement(nested); - } // When srcdir is another Fileset, then files must be null so strip_prefix must also be null. - return NestedTraversalParams.getNestedTraversal(ownerLabel, nested, destPath, excludes); + return NestedTraversalParams.getNestedTraversal(ownerLabel, artifact, destPath, excludes); } private static ImmutableSortedSet getOrderedExcludes(@Nullable Set excludes) { @@ -147,8 +138,8 @@ public final class FilesetTraversalParamsFactory { @AutoValue abstract static class DirectoryTraversalParams implements FilesetTraversalParams { @Override - public ImmutableList getNestedTraversal() { - return ImmutableList.of(); + public Artifact getNestedArtifact() { + return null; } @Memoized @@ -218,7 +209,7 @@ public final class FilesetTraversalParamsFactory { if (!getExcludedFiles().isEmpty()) { fp.addStrings(getExcludedFiles()); } - getNestedTraversal().forEach(nestedTraversal -> nestedTraversal.fingerprint(fp)); + fp.addPath(getNestedArtifact().getExecPath()); return fp.digestAndReset(); } @@ -229,10 +220,10 @@ public final class FilesetTraversalParamsFactory { static NestedTraversalParams getNestedTraversal( Label ownerLabel, - ImmutableList nested, + Artifact nestedArtifact, PathFragment destPath, @Nullable Set excludes) { - return create(ownerLabel, destPath, getOrderedExcludes(excludes), nested); + return create(ownerLabel, destPath, getOrderedExcludes(excludes), nestedArtifact); } @AutoCodec.VisibleForSerialization @@ -241,9 +232,9 @@ public final class FilesetTraversalParamsFactory { Label ownerLabelForErrorMessages, PathFragment destPath, ImmutableSortedSet excludedFiles, - ImmutableList nestedTraversal) { + Artifact nestedArtifact) { return new AutoValue_FilesetTraversalParamsFactory_NestedTraversalParams( - ownerLabelForErrorMessages, destPath, excludedFiles, nestedTraversal); + ownerLabelForErrorMessages, destPath, excludedFiles, nestedArtifact); } } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java index 30861cd3ac..dbce56b589 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java @@ -31,11 +31,9 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; import com.google.devtools.build.lib.analysis.configuredtargets.EnvironmentGroupConfiguredTarget; -import com.google.devtools.build.lib.analysis.configuredtargets.FilesetOutputConfiguredTarget; import com.google.devtools.build.lib.analysis.configuredtargets.InputFileConfiguredTarget; import com.google.devtools.build.lib.analysis.configuredtargets.OutputFileConfiguredTarget; import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget; -import com.google.devtools.build.lib.analysis.fileset.FilesetProvider; import com.google.devtools.build.lib.analysis.skylark.SkylarkRuleConfiguredTargetUtil; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -232,16 +230,7 @@ public final class ConfiguredTargetFactory { } TransitiveInfoCollection rule = targetContext.findDirectPrerequisite( outputFile.getGeneratingRule().getLabel(), config); - if (isFileset) { - return new FilesetOutputConfiguredTarget( - targetContext, - outputFile, - rule, - artifact, - rule.getProvider(FilesetProvider.class).getTraversals()); - } else { return new OutputFileConfiguredTarget(targetContext, outputFile, rule, artifact); - } } else if (target instanceof InputFile) { InputFile inputFile = (InputFile) target; SourceArtifact artifact = diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index 3eebb2d037..0cccfb8345 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -55,7 +55,6 @@ import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode; import com.google.devtools.build.lib.analysis.constraints.ConstraintSemantics; -import com.google.devtools.build.lib.analysis.fileset.FilesetProvider; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext; import com.google.devtools.build.lib.cmdline.Label; @@ -1555,9 +1554,12 @@ public final class RuleContext extends TargetContext } private boolean validateFilesetEntry(FilesetEntry filesetEntry, ConfiguredTargetAndData src) { - if (src.getConfiguredTarget().getProvider(FilesetProvider.class) != null) { + NestedSet filesToBuild = + src.getConfiguredTarget().getProvider(FileProvider.class).getFilesToBuild(); + if (filesToBuild.isSingleton() && Iterables.getOnlyElement(filesToBuild).isFileset()) { return true; } + if (filesetEntry.isSourceFileset()) { return true; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FileConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FileConfiguredTarget.java index 2fec10f91e..cd959fa858 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FileConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FileConfiguredTarget.java @@ -23,7 +23,6 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMap; import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMapBuilder; import com.google.devtools.build.lib.analysis.VisibilityProvider; -import com.google.devtools.build.lib.analysis.fileset.FilesetProvider; import com.google.devtools.build.lib.analysis.test.InstrumentedFilesProvider; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -62,9 +61,6 @@ public abstract class FileConfiguredTarget extends AbstractConfiguredTarget .put(LicensesProvider.class, this) .add(fileProvider) .add(filesToRunProvider); - if (this instanceof FilesetProvider) { - builder.put(FilesetProvider.class, this); - } if (this instanceof InstrumentedFilesProvider) { builder.put(InstrumentedFilesProvider.class, this); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FilesetOutputConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FilesetOutputConfiguredTarget.java deleted file mode 100644 index 08dc1ac78e..0000000000 --- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FilesetOutputConfiguredTarget.java +++ /dev/null @@ -1,84 +0,0 @@ -// Copyright 2014 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.analysis.configuredtargets; - -import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.FilesetTraversalParams; -import com.google.devtools.build.lib.analysis.TargetContext; -import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; -import com.google.devtools.build.lib.analysis.fileset.FilesetProvider; -import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.collect.nestedset.NestedSet; -import com.google.devtools.build.lib.packages.OutputFile; -import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents; -import com.google.devtools.build.lib.skyframe.BuildConfigurationValue; -import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; - -/** - * A configured target for output files generated by {@code Fileset} rules. They are almost the same - * thing as output files except that they implement {@link FilesetProvider} so that {@code Fileset} - * can figure out the link tree behind them. - * - *

In an ideal world, this would not be needed: Filesets would depend on other Filesets and not - * their output directories. However, sometimes a Fileset depends on the output directory of another - * Fileset. Thus, we need this hack. - */ -@AutoCodec -public final class FilesetOutputConfiguredTarget extends OutputFileConfiguredTarget - implements FilesetProvider { - private final ImmutableList filesetTraversals; - - public FilesetOutputConfiguredTarget( - TargetContext targetContext, - OutputFile outputFile, - TransitiveInfoCollection generatingRule, - Artifact outputArtifact, - ImmutableList traversals) { - this( - targetContext.getLabel(), - targetContext.getConfigurationKey(), - targetContext.getVisibility(), - generatingRule, - outputArtifact, - traversals); - Preconditions.checkState( - outputFile.getLabel().equals(targetContext.getLabel()), - "mismatch: %s %s", - outputFile, - targetContext); - } - - @AutoCodec.VisibleForSerialization - @AutoCodec.Instantiator - FilesetOutputConfiguredTarget( - Label label, - BuildConfigurationValue.Key configurationKey, - NestedSet visibility, - TransitiveInfoCollection generatingRule, - Artifact artifact, - ImmutableList traversals) { - super(label, configurationKey, visibility, artifact, generatingRule); - FilesetProvider provider = generatingRule.getProvider(FilesetProvider.class); - Preconditions.checkArgument(provider != null); - filesetTraversals = traversals; - } - - @Override - public ImmutableList getTraversals() { - return filesetTraversals; - } -} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/fileset/FilesetProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/fileset/FilesetProvider.java deleted file mode 100644 index 225a40eb42..0000000000 --- a/src/main/java/com/google/devtools/build/lib/analysis/fileset/FilesetProvider.java +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright 2014 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.analysis.fileset; - -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.actions.FilesetTraversalParams; -import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; - -/** - * Information needed by a Fileset to do the right thing when it depends on another Fileset. - */ -public interface FilesetProvider extends TransitiveInfoProvider { - /** - * Returns a list of the traversals that went into this Fileset. Only used by Skyframe-native - * filesets, so will be null if Skyframe-native filesets are not enabled. - */ - ImmutableList getTraversals(); -} 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 7bff8b0193..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 @@ -187,8 +187,9 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver return null; } + Object skyframeDepsResult; try { - establishSkyframeDependencies(env, action); + skyframeDepsResult = establishSkyframeDependencies(env, action); } catch (ActionExecutionException e) { throw new ActionExecutionFunctionException(e); } @@ -213,7 +214,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver try { result = checkCacheAndExecuteIfNeeded( - action, state, env, clientEnv, actionLookupData, sharedActionAlreadyRan); + action, state, env, clientEnv, actionLookupData, sharedActionAlreadyRan, + skyframeDepsResult); } catch (ActionExecutionException e) { // Remove action from state map in case it's there (won't be unless it discovers inputs). stateMap.remove(action); @@ -363,7 +365,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver Environment env, Map clientEnv, ActionLookupData actionLookupData, - boolean sharedActionAlreadyRan) + boolean sharedActionAlreadyRan, + Object skyframeDepsResult) throws ActionExecutionException, InterruptedException { // If this is a shared action and the other action is the one that executed, we must use that // other action's value, provided here, since it is populated with metadata for the outputs. @@ -496,7 +499,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver metadataHandler, Collections.unmodifiableMap(state.expandedArtifacts), filesets, - state.actionFileSystem)) { + state.actionFileSystem, + skyframeDepsResult)) { if (!state.hasExecutedAction()) { state.value = skyframeActionExecutor.executeAction( @@ -597,7 +601,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver } } - private static void establishSkyframeDependencies(Environment env, Action action) + private static Object establishSkyframeDependencies(Environment env, Action action) throws ActionExecutionException, InterruptedException { // Before we may safely establish Skyframe dependencies, we must build all action inputs by // requesting their ArtifactValues. @@ -614,11 +618,12 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver Preconditions.checkState(action.executeUnconditionally(), action); try { - ((SkyframeAwareAction) action).establishSkyframeDependencies(env); + return ((SkyframeAwareAction) action).establishSkyframeDependencies(env); } catch (SkyframeAwareAction.ExceptionBase e) { throw new ActionExecutionException(e, action, false); } } + return null; } private static Iterable toKeys( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java index 22f5b31935..aa155b1cb3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java @@ -51,170 +51,134 @@ public final class FilesetEntryFunction implements SkyFunction { public SkyValue compute(SkyKey key, Environment env) throws FilesetEntryFunctionException, InterruptedException { FilesetTraversalParams t = (FilesetTraversalParams) key.argument(); - if (t.getDirectTraversal().isPresent()) { - Preconditions.checkState( - t.getNestedTraversal().isEmpty(), - "NestedTraversal must be empty if directTraversal is present: %s", t); - } - - // Create the set of excluded files. Only top-level files can be excluded, i.e. ones that are - // directly under the root if the root is a directory. - Set exclusions = - Sets.filter(t.getExcludedFiles(), e -> PathFragment.create(e).segmentCount() == 1); + Preconditions.checkState( + t.getDirectTraversal().isPresent() && t.getNestedArtifact() == null, + "FilesetEntry does not support nested traversal: %s", t); // The map of output symlinks. Each key is the path of a output symlink that the Fileset must // create, relative to the Fileset.out directory, and each value specifies extra information // about the link (its target, associated metadata and again its name). Map outputSymlinks = new LinkedHashMap<>(); - if (!t.getDirectTraversal().isPresent()) { - // The absence of "direct" traversal indicates the presence of a "nested" fileset and - // getNestedTraversal will return the list FilesetTraversalParams corresponding to each - // FilesetEntry of the nested Fileset. - ImmutableList nestedKeys = FilesetEntryKey.keys(t.getNestedTraversal()); - Map results = env.getValues(nestedKeys); - if (env.valuesMissing()) { - return null; - } - - for (SkyKey nestedKey : nestedKeys) { - FilesetEntryValue nested = (FilesetEntryValue) results.get(nestedKey); - for (FilesetOutputSymlink s : nested.getSymlinks()) { - if (!exclusions.contains(s.getName().getPathString())) { - maybeStoreSymlink(s, t.getDestPath(), outputSymlinks); - } - } - } - } else { - // The "direct" traversal params are present, which is the case when the FilesetEntry - // specifies a package's BUILD file, a directory or a list of files. - - // The root of the direct traversal is defined as follows. - // - // If FilesetEntry.files is specified, then a TraversalRequest is created for each entry, the - // root being the respective entry itself. These are all traversed for they may be - // directories or symlinks to directories, and we need to establish Skyframe dependencies on - // their contents for incremental correctness. If an entry is indeed a directory (but not when - // it's a symlink to one) then we have to create symlinks to each of their childen. - // (NB: there seems to be no good reason for this, it's just how legacy Fileset works. We may - // want to consider creating a symlink just for the directory and not for its child elements.) - // - // If FilesetEntry.files is not specified, then srcdir refers to either a BUILD file or a - // directory. For the former, the root will be the parent of the BUILD file. For the latter, - // the root will be srcdir itself. - DirectTraversal direct = t.getDirectTraversal().get(); - - RecursiveFilesystemTraversalValue rftv; - try { - // Traverse the filesystem to establish skyframe dependencies. - rftv = traverse(env, createErrorInfo(t), direct); - } catch (MissingDepException e) { - return null; - } + // The "direct" traversal params are present, which is the case when the FilesetEntry + // specifies a package's BUILD file, a directory or a list of files. + + // The root of the direct traversal is defined as follows. + // + // If FilesetEntry.files is specified, then a TraversalRequest is created for each entry, the + // root being the respective entry itself. These are all traversed for they may be + // directories or symlinks to directories, and we need to establish Skyframe dependencies on + // their contents for incremental correctness. If an entry is indeed a directory (but not when + // it's a symlink to one) then we have to create symlinks to each of their childen. + // (NB: there seems to be no good reason for this, it's just how legacy Fileset works. We may + // want to consider creating a symlink just for the directory and not for its child elements.) + // + // If FilesetEntry.files is not specified, then srcdir refers to either a BUILD file or a + // directory. For the former, the root will be the parent of the BUILD file. For the latter, + // the root will be srcdir itself. + DirectTraversal direct = t.getDirectTraversal().get(); + + RecursiveFilesystemTraversalValue rftv; + try { + // Traverse the filesystem to establish skyframe dependencies. + rftv = traverse(env, createErrorInfo(t), direct); + } catch (MissingDepException e) { + return null; + } - // The root can only be absent for the EMPTY rftv instance. - if (!rftv.getResolvedRoot().isPresent()) { - return FilesetEntryValue.EMPTY; - } + // The root can only be absent for the EMPTY rftv instance. + if (!rftv.getResolvedRoot().isPresent()) { + return FilesetEntryValue.EMPTY; + } - ResolvedFile resolvedRoot = rftv.getResolvedRoot().get(); + ResolvedFile resolvedRoot = rftv.getResolvedRoot().get(); - // Handle dangling symlinks gracefully be returning empty results. - if (!resolvedRoot.getType().exists()) { - return FilesetEntryValue.EMPTY; - } + // Handle dangling symlinks gracefully be returning empty results. + if (!resolvedRoot.getType().exists()) { + return FilesetEntryValue.EMPTY; + } - // The prefix to remove is the entire path of the root. This is OK: - // - when the root is a file, this removes the entire path, but the traversal's destination - // path is actually the name of the output symlink, so this works out correctly - // - when the root is a directory or a symlink to one then we need to strip off the - // directory's path from every result (this is how the output symlinks must be created) - // before making them relative to the destination path - PathFragment prefixToRemove = direct.getRoot().getRelativePart(); - - Iterable results = null; - - if (direct.isRecursive() - || (resolvedRoot.getType().isDirectory() && !resolvedRoot.getType().isSymlink())) { - // The traversal is recursive (requested for an entire FilesetEntry.srcdir) or it was - // requested for a FilesetEntry.files entry which turned out to be a directory. We need to - // create an output symlink for every file in it and all of its subdirectories. Only - // exception is when the subdirectory is really a symlink to a directory -- no output - // shall be created for the contents of those. - // Now we create Dir objects to model the filesystem tree. The object employs a trick to - // find directory symlinks: directory symlinks have corresponding ResolvedFile entries and - // are added as files too, while their children, also added as files, contain the path of - // the parent. Finding and discarding the children is easy if we traverse the tree from - // root to leaf. - DirectoryTree root = new DirectoryTree(); - for (ResolvedFile f : rftv.getTransitiveFiles().toCollection()) { - PathFragment path = f.getNameInSymlinkTree().relativeTo(prefixToRemove); - if (!path.isEmpty()) { - path = t.getDestPath().getRelative(path); - DirectoryTree dir = root; - for (int i = 0; i < path.segmentCount() - 1; ++i) { - dir = dir.addOrGetSubdir(path.getSegment(i)); - } - dir.maybeAddFile(f); + // The prefix to remove is the entire path of the root. This is OK: + // - when the root is a file, this removes the entire path, but the traversal's destination + // path is actually the name of the output symlink, so this works out correctly + // - when the root is a directory or a symlink to one then we need to strip off the + // directory's path from every result (this is how the output symlinks must be created) + // before making them relative to the destination path + PathFragment prefixToRemove = direct.getRoot().getRelativePart(); + + Iterable results = null; + + if (direct.isRecursive() + || (resolvedRoot.getType().isDirectory() && !resolvedRoot.getType().isSymlink())) { + // The traversal is recursive (requested for an entire FilesetEntry.srcdir) or it was + // requested for a FilesetEntry.files entry which turned out to be a directory. We need to + // create an output symlink for every file in it and all of its subdirectories. Only + // exception is when the subdirectory is really a symlink to a directory -- no output + // shall be created for the contents of those. + // Now we create Dir objects to model the filesystem tree. The object employs a trick to + // find directory symlinks: directory symlinks have corresponding ResolvedFile entries and + // are added as files too, while their children, also added as files, contain the path of + // the parent. Finding and discarding the children is easy if we traverse the tree from + // root to leaf. + DirectoryTree root = new DirectoryTree(); + for (ResolvedFile f : rftv.getTransitiveFiles().toCollection()) { + PathFragment path = f.getNameInSymlinkTree().relativeTo(prefixToRemove); + if (!path.isEmpty()) { + path = t.getDestPath().getRelative(path); + DirectoryTree dir = root; + for (int i = 0; i < path.segmentCount() - 1; ++i) { + dir = dir.addOrGetSubdir(path.getSegment(i)); } + dir.maybeAddFile(f); } - // Here's where the magic happens. The returned iterable will yield all files in the - // directory that are not under symlinked directories, as well as all directory symlinks. - results = root.iterateFiles(); - } else { - // If we're on this branch then the traversal was done for just one entry in - // FilesetEntry.files (which was not a directory, so it was either a file, a symlink to one - // or a symlink to a directory), meaning we'll have only one output symlink. - results = ImmutableList.of(resolvedRoot); } + // Here's where the magic happens. The returned iterable will yield all files in the + // directory that are not under symlinked directories, as well as all directory symlinks. + results = root.iterateFiles(); + } else { + // If we're on this branch then the traversal was done for just one entry in + // FilesetEntry.files (which was not a directory, so it was either a file, a symlink to one + // or a symlink to a directory), meaning we'll have only one output symlink. + results = ImmutableList.of(resolvedRoot); + } - // Create one output symlink for each entry in the results. - for (ResolvedFile f : results) { - // The linkName has to be under the traversal's root, which is also the prefix to remove. - PathFragment linkName = f.getNameInSymlinkTree().relativeTo(prefixToRemove); - - // Check whether the symlink is excluded before attempting to resolve it. - // It may be dangling, but excluding it is still fine. - // TODO(b/64754128): Investigate if we could have made the exclude earlier before - // unnecessarily iterating over all the files in an excluded directory. - if (linkName.segmentCount() > 0 && exclusions.contains(linkName.getSegment(0))) { - continue; - } + // Create the set of excluded files. Only top-level files can be excluded, i.e. ones that are + // directly under the root if the root is a directory. + Set exclusions = + Sets.filter(t.getExcludedFiles(), e -> PathFragment.create(e).segmentCount() == 1); - PathFragment targetName; - try { - targetName = f.getTargetInSymlinkTree(direct.isFollowingSymlinks()); - } catch (DanglingSymlinkException e) { - throw new FilesetEntryFunctionException(e); - } + // Create one output symlink for each entry in the results. + for (ResolvedFile f : results) { + // The linkName has to be under the traversal's root, which is also the prefix to remove. + PathFragment linkName = f.getNameInSymlinkTree().relativeTo(prefixToRemove); + + // Check whether the symlink is excluded before attempting to resolve it. + // It may be dangling, but excluding it is still fine. + // TODO(b/64754128): Investigate if we could have made the exclude earlier before + // unnecessarily iterating over all the files in an excluded directory. + if (linkName.segmentCount() > 0 && exclusions.contains(linkName.getSegment(0))) { + continue; + } - maybeStoreSymlink( - linkName, - targetName, - f.getMetadata(), - t.getDestPath(), - direct.isGenerated(), - outputSymlinks); + PathFragment targetName; + try { + targetName = f.getTargetInSymlinkTree(direct.isFollowingSymlinks()); + } catch (DanglingSymlinkException e) { + throw new FilesetEntryFunctionException(e); } + + maybeStoreSymlink( + linkName, + targetName, + f.getMetadata(), + t.getDestPath(), + direct.isGenerated(), + outputSymlinks); } return FilesetEntryValue.of(ImmutableSet.copyOf(outputSymlinks.values())); } - /** Stores an output symlink unless it would overwrite an existing one. */ - private static void maybeStoreSymlink( - FilesetOutputSymlink nestedLink, - PathFragment destPath, - Map result) { - maybeStoreSymlink( - nestedLink.getName(), - nestedLink.getTargetPath(), - nestedLink.getMetadata(), - destPath, - nestedLink.isGeneratedTarget(), - result); - } - /** Stores an output symlink unless it would overwrite an existing one. */ private static void maybeStoreSymlink( PathFragment linkName, 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 5fd47c7460..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 @@ -519,7 +519,8 @@ public final class SkyframeActionExecutor { MetadataHandler metadataHandler, Map> expandedInputs, ImmutableMap> inputFilesetMappings, - @Nullable FileSystem actionFileSystem) { + @Nullable FileSystem actionFileSystem, + @Nullable Object skyframeDepsResult) { FileOutErr fileOutErr = actionLogBufferPathGenerator.generate( ArtifactPathResolver.createPathResolver(actionFileSystem, executorEngine.getExecRoot())); return new ActionExecutionContext( @@ -532,7 +533,8 @@ public final class SkyframeActionExecutor { clientEnv, inputFilesetMappings, new ArtifactExpanderImpl(expandedInputs), - actionFileSystem); + actionFileSystem, + skyframeDepsResult); } /** diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAwareAction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAwareAction.java index c351cda50b..da81769f7a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAwareAction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAwareAction.java @@ -68,6 +68,9 @@ public interface SkyframeAwareAction { *

This method should not attempt to handle errors or missing dependencies (other than wrapping * exceptions); that is the responsibility of the caller. It should return as soon as possible, * ready to be called again at a later time if need be. + * + *

The return value will be incorporated into the + * {@link com.google.devtools.build.lib.actions.ActionExecutionContext}. */ - void establishSkyframeDependencies(Environment env) throws ExceptionBase, InterruptedException; + Object establishSkyframeDependencies(Environment env) throws ExceptionBase, InterruptedException; } diff --git a/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java index 9f233b3dd4..5f8bf79f1c 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java @@ -66,6 +66,7 @@ public class ExecutableSymlinkActionTest { ImmutableMap.of(), ImmutableMap.of(), null, + null, null); } diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index 80b8e7e403..1aa4022b34 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java @@ -147,7 +147,8 @@ public final class ActionsTestUtil { actionGraph == null ? createDummyArtifactExpander() : ActionInputHelper.actionGraphArtifactExpander(actionGraph), - /*actionFileSystem=*/ null); + /*actionFileSystem=*/ null, + /*skyframeDepsResult=*/ null); } public static ActionExecutionContext createContextForInputDiscovery( @@ -182,7 +183,8 @@ public final class ActionsTestUtil { ImmutableMap.of(), ImmutableMap.of(), createDummyArtifactExpander(), - /*actionFileSystem=*/ null); + /*actionFileSystem=*/ null, + /*skyframeDepsResult=*/ null); } private static ArtifactExpander createDummyArtifactExpander() { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionTestCase.java index 12cbfb4ffe..fe4b0a2805 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/FileWriteActionTestCase.java @@ -68,6 +68,7 @@ public abstract class FileWriteActionTestCase extends BuildViewTestCase { ImmutableMap.of(), ImmutableMap.of(), null, + null, null); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java index 59364a3328..b736576d57 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java @@ -203,7 +203,8 @@ public class ParamFileWriteActionTest extends BuildViewTestCase { ImmutableMap.of(), ImmutableMap.of(), artifactExpander, - /*actionFileSystem=*/ null); + /*actionFileSystem=*/ null, + /*skyframeDepsResult=*/ null); } private enum KeyAttributes { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java index c3e25614fd..ae213a4616 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java @@ -90,7 +90,8 @@ public class SymlinkActionTest extends BuildViewTestCase { ImmutableMap.of(), ImmutableMap.of(), null, - /*actionFileSystem=*/ null)); + /*actionFileSystem=*/ null, + /*skyframeDepsResult=*/ null)); assertThat(actionResult.spawnResults()).isEmpty(); assertThat(output.isSymbolicLink()).isTrue(); assertThat(output.resolveSymbolicLinks()).isEqualTo(input); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java index 48c169af4d..b7fc6c755e 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java @@ -192,7 +192,8 @@ public class TemplateExpansionActionTest extends FoundationTestCase { ImmutableMap.of(), ImmutableMap.of(), null, - /*actionFileSystem=*/ null); + /*actionFileSystem=*/ null, + /*skyframeDepsResult=*/ null); } private void executeTemplateExpansion(String expected) throws Exception { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index c3bea4f72c..9a1816864f 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -2168,7 +2168,8 @@ public abstract class BuildViewTestCase extends FoundationTestCase { clientEnv, ImmutableMap.of(), artifactExpander, - /*actionFileSystem=*/ null); + /*actionFileSystem=*/ null, + /*skyframeDepsResult*/ null); } } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java index c447f67450..e84c832e47 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java @@ -116,7 +116,8 @@ public class CreateIncSymlinkActionTest extends FoundationTestCase { private ActionExecutionContext makeDummyContext() { DummyExecutor executor = new DummyExecutor(fileSystem, rootDirectory); return new ActionExecutionContext( - executor, null, null, null, null, null, ImmutableMap.of(), ImmutableMap.of(), null, null); + executor, null, null, null, null, null, ImmutableMap.of(), ImmutableMap.of(), null, null, + null); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java index 25c7e0d444..f8881ee628 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LtoBackendActionTest.java @@ -90,7 +90,8 @@ public class LtoBackendActionTest extends BuildViewTestCase { ImmutableMap.of(), ImmutableMap.of(), null, - /*actionFileSystem=*/ null); + /*actionFileSystem=*/ null, + /*skyframeDepsResult=*/ null); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java index f617673885..c74e14000d 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java @@ -794,6 +794,7 @@ public class BazelJ2ObjcLibraryTest extends J2ObjcLibraryTest { ImmutableMap.of(), ImmutableMap.of(), DUMMY_ARTIFACT_EXPANDER, + null, null); ByteArrayOutputStream moduleMapStream = new ByteArrayOutputStream(); ByteArrayOutputStream umbrellaHeaderStream = new ByteArrayOutputStream(); @@ -845,6 +846,7 @@ public class BazelJ2ObjcLibraryTest extends J2ObjcLibraryTest { ImmutableMap.of(), ImmutableMap.of(), DUMMY_ARTIFACT_EXPANDER, + null, null); ByteArrayOutputStream moduleMapStream = new ByteArrayOutputStream(); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java index e972020051..5e5ff1420c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java @@ -592,147 +592,6 @@ public final class FilesetEntryFunctionTest extends FoundationTestCase { assertRecursiveTraversalForPackage(SymlinkBehavior.DEREFERENCE, REPORT_ERROR); } - @Test - public void testNestedFileFilesetTraversal() throws Exception { - Artifact path1 = getSourceArtifact("foo/bar.file"); - createFile(path1, "blah"); - Artifact path2 = getSourceArtifact("foo/baz.file"); - createFile(path2, "what"); - FilesetTraversalParams inner1 = - FilesetTraversalParamsFactory.fileTraversal( - /*ownerLabel=*/ label("//foo"), - /*fileToTraverse=*/ path1, - PathFragment.create("inner-out1"), - /*symlinkBehaviorMode=*/ SymlinkBehavior.COPY, - /*pkgBoundaryMode=*/ DONT_CROSS, - /*strictFilesetOutput=*/ false); - FilesetTraversalParams inner2 = - FilesetTraversalParamsFactory.fileTraversal( - /*ownerLabel=*/ label("//foo"), - /*fileToTraverse=*/ path2, - PathFragment.create("inner-out2"), - /*symlinkBehaviorMode=*/ SymlinkBehavior.COPY, - /*pkgBoundaryMode=*/ DONT_CROSS, - /*strictFilesetOutput=*/ false); - FilesetTraversalParams outer = - FilesetTraversalParamsFactory.nestedTraversal( - /*ownerLabel=*/ label("//foo:bar"), - /*nested=*/ ImmutableList.of(inner1, inner2), - PathFragment.create("outer-out"), - /*excludes=*/ null); - assertSymlinksCreatedInOrder( - outer, - symlink("outer-out/inner-out1", rootedPath(path1)), - symlink("outer-out/inner-out2", rootedPath(path2))); - } - - @Test - public void testMultiLevelNesting() throws Exception { - Artifact path1 = getSourceArtifact("foo/bar.file"); - createFile(path1, "blah"); - Artifact path2 = getSourceArtifact("foo/baz.file"); - createFile(path2, "what"); - Artifact path3 = getSourceArtifact("foo/hw.file"); - createFile(path3, "hello"); - FilesetTraversalParams inner1 = - FilesetTraversalParamsFactory.fileTraversal( - /*ownerLabel=*/ label("//foo"), - /*fileToTraverse=*/ path1, - PathFragment.create("inner-out1"), - /*symlinkBehaviorMode=*/ SymlinkBehavior.COPY, - /*pkgBoundaryMode=*/ DONT_CROSS, - /*strictFilesetOutput=*/ false); - FilesetTraversalParams inner2 = - FilesetTraversalParamsFactory.fileTraversal( - /*ownerLabel=*/ label("//foo"), - /*fileToTraverse=*/ path2, - PathFragment.create("inner-out2"), - /*symlinkBehaviorMode=*/ SymlinkBehavior.COPY, - /*pkgBoundaryMode=*/ DONT_CROSS, - /*strictFilesetOutput=*/ false); - FilesetTraversalParams middle1 = - FilesetTraversalParamsFactory.nestedTraversal( - /*ownerLabel=*/ label("//foo:middle1"), - /*nested=*/ ImmutableList.of(inner1, inner2), - PathFragment.create("middle-out1"), - /*excludes=*/ null); - - FilesetTraversalParams inner3 = - FilesetTraversalParamsFactory.fileTraversal( - /*ownerLabel=*/ label("//foo:inner3"), - /*fileToTraverse=*/ path3, - PathFragment.create("inner-out3"), - /*symlinkBehaviorMode=*/ SymlinkBehavior.COPY, - /*pkgBoundaryMode=*/ DONT_CROSS, - /*strictFilesetOutput=*/ false); - FilesetTraversalParams middle2 = - FilesetTraversalParamsFactory.nestedTraversal( - /*ownerLabel=*/ label("//foo:middle2"), - /*nested=*/ ImmutableList.of(inner3), - PathFragment.create("middle-out2"), - /*excludes=*/ null); - - FilesetTraversalParams outer = - FilesetTraversalParamsFactory.nestedTraversal( - /*ownerLabel=*/ label("//foo:bar"), - /*nested=*/ ImmutableList.of(middle1, middle2), - PathFragment.create("outer-out"), - /*excludes=*/ null); - assertSymlinksCreatedInOrder( - outer, - symlink("outer-out/middle-out1/inner-out1", rootedPath(path1)), - symlink("outer-out/middle-out1/inner-out2", rootedPath(path2)), - symlink("outer-out/middle-out2/inner-out3", rootedPath(path3))); - } - - private void assertNestedRecursiveFilesetTraversal(boolean useInnerDir) throws Exception { - Artifact dir = getSourceArtifact("foo/dir"); - RootedPath fileA = createFile(childOf(dir, "file.a"), "hello"); - RootedPath fileB = createFile(childOf(dir, "file.b"), "hello"); - RootedPath fileC = createFile(childOf(dir, "sub/file.c"), "world"); - - FilesetTraversalParams inner = - FilesetTraversalParamsFactory.recursiveTraversalOfDirectory( - /*ownerLabel=*/ label("//foo"), - /*directoryToTraverse=*/ dir, - PathFragment.create(useInnerDir ? "inner-dir" : ""), - /*excludes=*/ null, - /*symlinkBehaviorMode=*/ SymlinkBehavior.COPY, - /*pkgBoundaryMode=*/ DONT_CROSS, - /*strictFilesetOutput=*/ false); - FilesetTraversalParams outer = - FilesetTraversalParamsFactory.nestedTraversal( - /*ownerLabel=*/ label("//foo"), - /*nested=*/ ImmutableList.of(inner), - PathFragment.create("outer-dir"), - /*excludes=*/ ImmutableSet.of("file.a", "sub/file.c")); - - if (useInnerDir) { - assertSymlinksCreatedInOrder( - outer, - // no file is excluded, since no files from "inner" are top-level in the outer Fileset - symlink("outer-dir/inner-dir/file.a", fileA), - symlink("outer-dir/inner-dir/file.b", fileB), - symlink("outer-dir/inner-dir/sub/file.c", fileC)); // only top-level files are excluded - } else { - assertSymlinksCreatedInOrder( - outer, - // file.a can be excluded because it's top-level (there's no output directory for "inner") - symlink("outer-dir/file.b", fileB), - symlink("outer-dir/sub/file.c", fileC)); // only top-level files could be excluded - } - } - - @Test - public void testNestedRecursiveFilesetTraversalWithInnerDestDir() throws Exception { - assertNestedRecursiveFilesetTraversal(true); - } - - @Test - public void testNestedRecursiveFilesetTraversalWithoutInnerDestDir() throws Exception { - assertNestedRecursiveFilesetTraversal(false); - } - @Test public void testFileTraversalForDanglingSymlink() throws Exception { Artifact linkName = getSourceArtifact("foo/dangling.sym"); @@ -1071,48 +930,13 @@ public final class FilesetEntryFunctionTest extends FoundationTestCase { @Test public void testFingerprintOfNestedTraversal() throws Exception { - FilesetTraversalParams n1 = - FilesetTraversalParamsFactory.fileTraversal( - /*ownerLabel=*/ label("//blah"), - /*fileToTraverse=*/ getSourceArtifact("blah/file.a"), - PathFragment.create("output-name"), - /*symlinkBehaviorMode=*/ SymlinkBehavior.COPY, - /*pkgBoundaryMode=*/ DONT_CROSS, - /*strictFilesetOutput=*/ false); - - FilesetTraversalParams n2 = - FilesetTraversalParamsFactory.fileTraversal( - /*ownerLabel=*/ label("//blah"), - /*fileToTraverse=*/ getSourceArtifact("meow/file.b"), - PathFragment.create("output-name"), - /*symlinkBehaviorMode=*/ SymlinkBehavior.COPY, - /*pkgBoundaryMode=*/ DONT_CROSS, - /*strictFilesetOutput=*/ false); - ImmutableList nested1 = ImmutableList.of(n1, n2); - - FilesetTraversalParams n3 = - FilesetTraversalParamsFactory.fileTraversal( - /*ownerLabel=*/ label("//blah"), - /*fileToTraverse=*/ getSourceArtifact("brrr/file.c"), - PathFragment.create("output-name"), - /*symlinkBehaviorMode=*/ SymlinkBehavior.COPY, - /*pkgBoundaryMode=*/ DONT_CROSS, - /*strictFilesetOutput=*/ false); - - FilesetTraversalParams n4 = - FilesetTraversalParamsFactory.fileTraversal( - /*ownerLabel=*/ label("//blah"), - /*fileToTraverse=*/ getSourceArtifact("hurr/file.d"), - PathFragment.create("output-name"), - /*symlinkBehaviorMode=*/ SymlinkBehavior.COPY, - /*pkgBoundaryMode=*/ DONT_CROSS, - /*strictFilesetOutput=*/ false); - ImmutableList nested2 = ImmutableList.of(n3, n4); + Artifact nested1 = getSourceArtifact("a/b"); + Artifact nested2 = getSourceArtifact("a/c"); new FingerprintTester( ImmutableMap.of( "ownerLabel", notPartOfFingerprint("//foo", "//bar"), - "nested", partOfFingerprint(nested1, nested2), + "nestedArtifact", partOfFingerprint(nested1, nested2), "destDir", partOfFingerprint("out1", "out2"), "excludes", partOfFingerprint(ImmutableSet.of(), ImmutableSet.of("x")))) { @@ -1121,7 +945,7 @@ public final class FilesetEntryFunctionTest extends FoundationTestCase { FilesetTraversalParams create(Map kwArgs) throws Exception { return FilesetTraversalParamsFactory.nestedTraversal( label((String) kwArgs.get("ownerLabel")), - (ImmutableList) kwArgs.get("nested"), + (Artifact) kwArgs.get("nestedArtifact"), PathFragment.create((String) kwArgs.get("destDir")), (Set) kwArgs.get("excludes")); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java index 3060a08d3e..18f117f43a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java @@ -249,11 +249,12 @@ public class SkyframeAwareActionTest extends TimestampBuilderTestCase { } @Override - public void establishSkyframeDependencies(Environment env) + public Object establishSkyframeDependencies(Environment env) throws ExceptionBase, InterruptedException { // Establish some Skyframe dependency. A real action would then use this to compute and // cache data for the execute(...) method. env.getValue(actionDepKey); + return null; } } @@ -797,8 +798,9 @@ public class SkyframeAwareActionTest extends TimestampBuilderTestCase { registerAction( new SingleOutputSkyframeAwareAction(genFile1, genFile2) { @Override - public void establishSkyframeDependencies(Environment env) throws ExceptionBase { + public Object establishSkyframeDependencies(Environment env) throws ExceptionBase { assertThat(env.valuesMissing()).isFalse(); + return null; } @Override diff --git a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java index da31f120d2..ccccc612e1 100644 --- a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java @@ -194,7 +194,8 @@ public class StandaloneSpawnStrategyTest { ImmutableMap.of(), ImmutableMap.of(), SIMPLE_ARTIFACT_EXPANDER, - /*actionFileSystem=*/ null); + /*actionFileSystem=*/ null, + /*skyframeDepsResult=*/ null); } @Test -- cgit v1.2.3