Add an archived representation for tree artifacts. Tree artifacts are currently passed as individual files which are in the directory. This change is a prototype which allows to pass those directories using a single archived file instead. Add a new option which, if enabled, assigns an archived representation to each of the tree artifacts. The representation is a single file, which contains all of the files in the artifact (zip archive). Those files are added in a directory using a reserved name (containing a `:`), therefore cannot clash with user-created files. Expand the TreeArtifactValue to enable storing the archived representation and update the inputs mapping code to include the archived file. Add handling for changes to the archived files in `FilesystemValueChecker` to ensure correctness in case of incremental builds, when the files get edited/deleted. Add explicit errors for use cases for which archived tree artifacts are not supported yet like inputs discovery, shared actions, action templates or discarding orphaned artifacts. Please note that this feature relies on spawn runners to create the artifact itself -- using it with one which does not support it will result with an error (missing tree artifact output). Add a new, parameterized category of tests for `FilesystemValueChecker` to cover handling of archived files. Improve reliability of `FilesystemValueChecker` by replacing the clock used to generate file `ctimes` for files with a `ManualClock` (previously `JavaClock`, using `System.currentTimeMillis()`). PiperOrigin-RevId: 327472179
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Actions.java b/src/main/java/com/google/devtools/build/lib/actions/Actions.java index fb08733..dbaa0f9 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Actions.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Actions.java
@@ -429,6 +429,7 @@ Artifact artifactI = Preconditions.checkNotNull(artifactPathMap.get(pathI), pathI); Artifact artifactJ = Preconditions.checkNotNull(artifactPathMap.get(pathJ), pathJ); + // TODO(b/159733792): Test this check with compressed tree artifact input. // We ignore the artifact prefix conflict between a TreeFileArtifact and its parent // TreeArtifact. // We can only have such a conflict here if:
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 b2eda56..60f2cd3d 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
@@ -220,17 +220,32 @@ default ImmutableList<FilesetOutputSymlink> getFileset(Artifact artifact) { throw new UnsupportedOperationException(); } + + /** + * Return an {@link ArchivedTreeArtifact} for a provided {@linkplain SpecialArtifact tree + * artifact} if one is available. + * + * <p>The {@linkplain ArchivedTreeArtifact archived tree artifact} can be used instead of the + * tree artifact expansion. + */ + @Nullable + default ArchivedTreeArtifact getArchivedTreeArtifact(SpecialArtifact treeArtifact) { + return null; + } } /** Implementation of {@link ArtifactExpander} */ public static class ArtifactExpanderImpl implements ArtifactExpander { private final Map<Artifact, Collection<Artifact>> expandedInputs; + private final Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts; private final Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets; public ArtifactExpanderImpl( Map<Artifact, Collection<Artifact>> expandedInputs, + Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts, Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets) { this.expandedInputs = expandedInputs; + this.archivedTreeArtifacts = archivedTreeArtifacts; this.expandedFilesets = expandedFilesets; } @@ -249,6 +264,11 @@ Preconditions.checkState(artifact.isFileset()); return Preconditions.checkNotNull(expandedFilesets.get(artifact)); } + + @Override + public ArchivedTreeArtifact getArchivedTreeArtifact(SpecialArtifact treeArtifact) { + return archivedTreeArtifacts.get(treeArtifact); + } } /** A Predicate that evaluates to true if the Artifact is not a middleman artifact. */ @@ -775,7 +795,7 @@ * access for their contents should be safe, even in a distributed context. * * TODO(shahan): move {@link Artifact#getPath} to this subclass. - * */ + */ public static final class SourceArtifact extends Artifact { private final ArtifactOwner owner; @@ -941,6 +961,100 @@ } /** + * Artifact representing a single-file archive with the filesystem tree belonging to a {@linkplain + * SpecialArtifact tree artifact}. + * + * <p>The archive is equivalent to the entire tree artifact -- it contains all of the {@linkplain + * TreeFileArtifact children} (and nothing else) of the tree artifact with their filesystem + * structure, relative to the {@linkplain SpecialArtifact#getExecPath() tree artifact directory}. + */ + @AutoCodec + public static final class ArchivedTreeArtifact extends DerivedArtifact { + private static final PathFragment ARCHIVED_ARTIFACTS_DERIVED_TREE_ROOT = + PathFragment.create(":archived_tree_artifacts"); + private final SpecialArtifact treeArtifact; + + public ArchivedTreeArtifact( + SpecialArtifact treeArtifact, ArtifactRoot root, PathFragment execPath) { + super(root, execPath, treeArtifact.getArtifactOwner()); + this.treeArtifact = treeArtifact; + } + + @Override + public SpecialArtifact getParent() { + return treeArtifact; + } + + /** + * Creates an {@link ArchivedTreeArtifact} for a given tree artifact. Returned artifact is + * stored in a permanent location, therefore can be shared across actions and builds. + * + * <p>Example: for a tree artifact of {@code bazel-out/k8-fastbuild/bin/directory} returns an + * {@linkplain ArchivedTreeArtifact artifact} of: {@code + * bazel-out/:archived_tree_artifacts/k8-fastbuild/bin/directory.zip}. + */ + public static ArchivedTreeArtifact create( + SpecialArtifact treeArtifact, PathFragment derivedPathPrefix) { + return createWithCustomDerivedTreeRoot( + treeArtifact, + derivedPathPrefix, + ARCHIVED_ARTIFACTS_DERIVED_TREE_ROOT, + treeArtifact.getRootRelativePath().replaceName(treeArtifact.getFilename() + ".zip")); + } + + /** + * Creates an {@link ArchivedTreeArtifact} for a given tree artifact within provided derived + * tree directory. + * + * <p>Example: for a tree artifact with root of {@code bazel-out/k8-fastbuild/bin} returns an + * {@linkplain ArchivedTreeArtifact artifact} of: {@code + * bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin/{rootRelativePath}} with root of: {@code + * bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin}. + */ + public static ArchivedTreeArtifact createWithCustomDerivedTreeRoot( + SpecialArtifact treeArtifact, + PathFragment derivedPathPrefix, + PathFragment customDerivedTreeRoot, + PathFragment rootRelativePath) { + ArtifactRoot artifactRoot = + createRootForArchivedArtifact( + treeArtifact.getRoot(), derivedPathPrefix, customDerivedTreeRoot); + ArchivedTreeArtifact archivedTreeArtifact = + new ArchivedTreeArtifact( + treeArtifact, artifactRoot, artifactRoot.getExecPath().getRelative(rootRelativePath)); + + archivedTreeArtifact.setGeneratingActionKey(treeArtifact.getGeneratingActionKey()); + return archivedTreeArtifact; + } + + private static ArtifactRoot createRootForArchivedArtifact( + ArtifactRoot treeArtifactRoot, + PathFragment derivedPathPrefix, + PathFragment customDerivedTreeRoot) { + Path execRoot = getExecRoot(treeArtifactRoot); + + // bazel-out/k8-fastbuild/bin -> bazel-out/{customDerivedTreeRoot}/k8-fastbuild/bin + PathFragment rootExecPath = + derivedPathPrefix + .getRelative(customDerivedTreeRoot) + .getRelative(treeArtifactRoot.getExecPath().relativeTo(derivedPathPrefix)); + + return ArtifactRoot.asDerivedRoot(execRoot, rootExecPath); + } + + private static Path getExecRoot(ArtifactRoot artifactRoot) { + // /output_base/execroot/bazel-out/k8-fastbuild/bin + Path rootPath = artifactRoot.getRoot().asPath(); + PathFragment rootPathFragment = rootPath.asFragment(); + // /output_base/execroot + PathFragment execRootPath = + rootPathFragment.subFragment( + 0, rootPathFragment.segmentCount() - artifactRoot.getExecPath().segmentCount()); + return rootPath.getFileSystem().getPath(execRootPath); + } + } + + /** * A special kind of artifact that represents a concrete file created at execution time under its * associated parent TreeArtifact. *
diff --git a/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java b/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java index 3ae550e..0d60069 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java
@@ -18,8 +18,10 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpanderImpl; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehavior; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -56,6 +58,7 @@ public static CompletionContext create( Map<Artifact, Collection<Artifact>> expandedArtifacts, + Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts, Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets, boolean expandFilesets, boolean fullyResolveFilesetSymlinks, @@ -64,7 +67,8 @@ Path execRoot, String workspaceName) throws IOException { - ArtifactExpander expander = new ArtifactExpanderImpl(expandedArtifacts, expandedFilesets); + ArtifactExpander expander = + new ArtifactExpanderImpl(expandedArtifacts, archivedTreeArtifacts, expandedFilesets); ArtifactPathResolver pathResolver = pathResolverFactory.shouldCreatePathResolverForArtifactValues() ? pathResolverFactory.createPathResolverForArtifactValues(
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index a5686f9..3e30b3f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -841,6 +841,16 @@ + " ignored unless --experimental_enable_flag_alias is set to true.") public List<Map.Entry<String, String>> commandLineFlagAliases; + @Option( + name = "experimental_send_archived_tree_artifact_inputs", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.EXECUTION}, + defaultValue = "false", + help = + "Send input tree artifacts as a single archived file rather than sending each file in the" + + " artifact as a separate input.") + public boolean sendArchivedTreeArtifactInputs; + /** Ways configured targets may provide the {@link Fragment}s they require. */ public enum IncludeConfigFragmentsEnum { /** @@ -916,6 +926,7 @@ host.cpu = hostCpu; host.includeRequiredConfigFragmentsProvider = includeRequiredConfigFragmentsProvider; host.enableAggregatingMiddleman = enableAggregatingMiddleman; + host.sendArchivedTreeArtifactInputs = sendArchivedTreeArtifactInputs; // === Runfiles === host.buildRunfilesManifests = buildRunfilesManifests;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java index b1dc981..567b2b6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -363,7 +363,7 @@ ImmutableList.of(CcCompilationHelper.getStlCcCompilationContext(ruleContext))) .setHeadersCheckingMode(semantics.determineHeadersCheckingMode(ruleContext)) .setCodeCoverageEnabled(CcCompilationHelper.isCodeCoverageEnabled(ruleContext)); - CompilationInfo compilationInfo = compilationHelper.compile(); + CompilationInfo compilationInfo = compilationHelper.compile(ruleContext::ruleError); CcCompilationContext ccCompilationContext = compilationInfo.getCcCompilationContext(); CcCompilationOutputs precompiledFileObjects = CcCompilationOutputs.builder()
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java index 6bef59b..e5126a2 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
@@ -31,6 +31,7 @@ import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext; import com.google.devtools.build.lib.analysis.actions.SymlinkAction; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.PerLabelOptions; import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector; import com.google.devtools.build.lib.cmdline.Label; @@ -59,6 +60,7 @@ import java.util.Map; import java.util.Set; import java.util.TreeMap; +import java.util.function.Consumer; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -730,7 +732,8 @@ * * @throws RuleErrorException */ - public CompilationInfo compile() throws RuleErrorException, InterruptedException { + public CompilationInfo compile(Consumer<String> errorReporter) + throws RuleErrorException, InterruptedException { if (!generatePicAction && !generateNoPicAction) { ruleErrorConsumer.ruleError("Either PIC or no PIC actions have to be created."); @@ -744,7 +747,7 @@ "All cc rules must support module maps."); // Create compile actions (both PIC and no-PIC). - CcCompilationOutputs ccOutputs = createCcCompileActions(); + CcCompilationOutputs ccOutputs = createCcCompileActions(errorReporter); return new CompilationInfo(ccCompilationContext, ccOutputs); } @@ -1296,7 +1299,8 @@ * file. It takes into account coverage, and PIC, in addition to using the settings specified on * the current object. This method should only be called once. */ - private CcCompilationOutputs createCcCompileActions() throws RuleErrorException { + private CcCompilationOutputs createCcCompileActions(Consumer<String> errorReporter) + throws RuleErrorException { CcCompilationOutputs.Builder result = CcCompilationOutputs.builder(); Preconditions.checkNotNull(ccCompilationContext); @@ -1330,6 +1334,7 @@ } outputNameMap = calculateOutputNameMapByType(compilationUnitSources, outputNamePrefixDir); + boolean hasTemplateAction = false; for (CppSource source : compilationUnitSources.values()) { Artifact sourceArtifact = source.getSource(); Label sourceLabel = source.getLabel(); @@ -1377,6 +1382,7 @@ break; } } else { + hasTemplateAction = true; switch (source.getType()) { case HEADER: Artifact headerTokenFile = @@ -1417,6 +1423,16 @@ } } + // TODO(b/159733792): Remove this -- this fails the analysis phase to avoid a crash later due to + // missing archived artifact for the template action output. + + if (hasTemplateAction + && configuration.getOptions().get(CoreOptions.class).sendArchivedTreeArtifactInputs) { + errorReporter.accept( + "Template actions are not supported when using" + + " --experimental_send_archived_tree_artifact_inputs option"); + } + return result.build(); }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcImport.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcImport.java index 060aba8..f4056c4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcImport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcImport.java
@@ -171,7 +171,7 @@ .setHeadersCheckingMode(HeadersCheckingMode.STRICT) .setCodeCoverageEnabled(CcCompilationHelper.isCodeCoverageEnabled(ruleContext)) .setPurpose(common.getPurpose(semantics)) - .compile(); + .compile(ruleContext::ruleError); Map<String, NestedSet<Artifact>> outputGroups = CcCompilationHelper.buildOutputGroups(compilationInfo.getCcCompilationOutputs());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java index d6472b7..f423473 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
@@ -299,7 +299,7 @@ + "even though there are no sources to compile in this configuration")); } - CompilationInfo compilationInfo = compilationHelper.compile(); + CompilationInfo compilationInfo = compilationHelper.compile(ruleContext::ruleError); CcCompilationOutputs precompiledFilesObjects = CcCompilationOutputs.builder() .addObjectFiles(precompiledFiles.getObjectFiles(/* usePic= */ true))
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java index 40d115a..d84043b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
@@ -1838,7 +1838,7 @@ helper.setStripIncludePrefix(stripIncludePrefix); } try { - CompilationInfo compilationInfo = helper.compile(); + CompilationInfo compilationInfo = helper.compile(actions.getRuleContext()::ruleError); return Tuple.of( compilationInfo.getCcCompilationContext(), compilationInfo.getCcCompilationOutputs()); } catch (RuleErrorException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanning.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanning.java index 4d53e00..9cbc36b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanning.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/IncludeScanning.java
@@ -23,6 +23,7 @@ import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.UserExecException; @@ -151,6 +152,8 @@ if (included.hasParent() && included.getParent().isTreeArtifact()) { // Note that this means every file in the TreeArtifact becomes an input to the action, and // we have spurious rebuilds if non-included files change. + Preconditions.checkArgument( + included instanceof TreeFileArtifact, "Not a TreeFileArtifact: %s", included); inputs.add(included.getParent()); } else { inputs.add(included);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java index 904f88c..52f0c5d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java
@@ -213,7 +213,7 @@ filesBuilder.addAll(outputs); createProtoCompileAction(outputs); - CompilationInfo compilationInfo = compilationHelper.compile(); + CompilationInfo compilationInfo = compilationHelper.compile(ruleContext::ruleError); CcCompilationOutputs ccCompilationOutputs = compilationInfo.getCcCompilationOutputs(); CcLinkingHelper ccLinkingHelper = initializeLinkingHelper(featureConfiguration, deps); if (ccToolchain(ruleContext).supportsInterfaceSharedLibraries(featureConfiguration)) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java index 21c0a80..00a8b99 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
@@ -322,7 +322,7 @@ result.doNotGenerateModuleMap(); } - return result.compile(); + return result.compile(ruleContext::ruleError); } private static class CompilationResult {
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 0930356..0116f1a 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
@@ -41,7 +41,9 @@ import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.DiscoveredInputsEvent; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -63,6 +65,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; +import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; @@ -185,7 +188,16 @@ } ActionExecutionValue actionExecutionValue = topDownActionCache.get(sketch); if (actionExecutionValue != null) { - return actionExecutionValue.transformForSharedAction(action.getOutputs()); + try { + Environment finalEnv = env; + return actionExecutionValue.transformForSharedAction( + action.getOutputs(), + action, + errorMessage -> finalEnv.getListener().handle(Event.error(errorMessage))); + } catch (ActionExecutionException e) { + stateMap.remove(action); + throw new ActionExecutionFunctionException(e); + } } } @@ -290,6 +302,7 @@ Preconditions.checkState(!state.hasArtifactData(), "%s %s", state, action); state.inputArtifactData = checkedInputs.actionInputMap; state.expandedArtifacts = checkedInputs.expandedArtifacts; + state.archivedTreeArtifacts = checkedInputs.archivedTreeArtifacts; state.filesetsInsideRunfiles = checkedInputs.filesetsInsideRunfiles; state.topLevelFilesets = checkedInputs.topLevelFilesets; if (skyframeActionExecutor.actionFileSystemType().isEnabled()) { @@ -734,7 +747,9 @@ ArtifactExpander artifactExpander = new Artifact.ArtifactExpanderImpl( - Collections.unmodifiableMap(state.expandedArtifacts), expandedFilesets); + Collections.unmodifiableMap(state.expandedArtifacts), + Collections.unmodifiableMap(state.archivedTreeArtifacts), + expandedFilesets); ArtifactPathResolver pathResolver = ArtifactPathResolver.createPathResolver( @@ -743,10 +758,12 @@ ActionMetadataHandler.create( state.inputArtifactData, action.discoversInputs(), + skyframeActionExecutor.useArchivedTreeArtifacts(), action.getOutputs(), tsgm.get(), pathResolver, skyframeActionExecutor.getExecRoot().asFragment(), + PathFragment.create(directories.getRelativeOutputPath()), expandedFilesets); // We only need to check the action cache if we haven't done it on a previous run. @@ -808,6 +825,7 @@ switch (addDiscoveredInputs( state.inputArtifactData, state.expandedArtifacts, + state.archivedTreeArtifacts, state.filterKnownDiscoveredInputs(), env, action)) { @@ -871,6 +889,7 @@ switch (addDiscoveredInputs( state.inputArtifactData, state.expandedArtifacts, + state.archivedTreeArtifacts, state.filterKnownDiscoveredInputs(), env, action)) { @@ -893,7 +912,9 @@ new Artifact.ArtifactExpanderImpl( // Skipping the filesets in runfiles since those cannot participate in command line // creation. - Collections.unmodifiableMap(state.expandedArtifacts), expandedFilesets), + Collections.unmodifiableMap(state.expandedArtifacts), + Collections.unmodifiableMap(state.archivedTreeArtifacts), + expandedFilesets), state.token, clientEnv); } @@ -908,6 +929,7 @@ private DiscoveredState addDiscoveredInputs( ActionInputMap inputData, Map<Artifact, Collection<Artifact>> expandedArtifacts, + Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts, Iterable<Artifact> discoveredInputs, Environment env, Action actionForError) @@ -964,6 +986,16 @@ inputData.putWithNoDepOwner(child.getKey(), child.getValue()); } inputData.putWithNoDepOwner(input, treeValue.getMetadata()); + treeValue + .getArchivedRepresentation() + .ifPresent( + archivedRepresentation -> { + inputData.putWithNoDepOwner( + archivedRepresentation.archivedTreeFileArtifact(), + archivedRepresentation.archivedFileValue()); + archivedTreeArtifacts.put( + (SpecialArtifact) input, archivedRepresentation.archivedTreeFileArtifact()); + }); } else if (retrievedMetadata instanceof ActionExecutionValue) { inputData.putWithNoDepOwner( input, ((ActionExecutionValue) retrievedMetadata).getExistingFileArtifactValue(input)); @@ -1016,6 +1048,8 @@ private final ActionInputMap actionInputMap; /** Artifact expansion mapping for Runfiles tree and tree artifacts. */ private final Map<Artifact, Collection<Artifact>> expandedArtifacts; + /** Archived representations for tree artifacts. */ + private final Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts; /** Artifact expansion mapping for Filesets embedded in Runfiles. */ private final ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetsInsideRunfiles; @@ -1025,10 +1059,12 @@ CheckInputResults( ActionInputMap actionInputMap, Map<Artifact, Collection<Artifact>> expandedArtifacts, + Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts, Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetsInsideRunfiles, Map<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets) { this.actionInputMap = actionInputMap; this.expandedArtifacts = expandedArtifacts; + this.archivedTreeArtifacts = archivedTreeArtifacts; this.filesetsInsideRunfiles = ImmutableMap.copyOf(filesetsInsideRunfiles); this.topLevelFilesets = ImmutableMap.copyOf(topLevelFilesets); } @@ -1038,6 +1074,7 @@ R create( S actionInputMapSink, Map<Artifact, Collection<Artifact>> expandedArtifacts, + Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts, Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetsInsideRunfiles, Map<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets); } @@ -1089,8 +1126,11 @@ allInputs, mandatoryInputs, ignoredInputDepsSize -> new ActionInputDepOwnerMap(lostInputs), - (actionInputMapSink, expandedArtifacts, filesetsInsideRunfiles, topLevelFilesets) -> - actionInputMapSink); + (actionInputMapSink, + expandedArtifacts, + archivedArtifacts, + filesetsInsideRunfiles, + topLevelFilesets) -> actionInputMapSink); } private <S extends ActionInputMapSink, R> R accumulateInputs( @@ -1129,7 +1169,9 @@ S inputArtifactData = actionInputMapSinkFactory.apply(populateInputData ? allInputsList.size() : 0); Map<Artifact, Collection<Artifact>> expandedArtifacts = - new HashMap<>(populateInputData ? 128 : 0); + Maps.newHashMapWithExpectedSize(populateInputData ? 128 : 0); + Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts = + Maps.newHashMapWithExpectedSize(128); Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetsInsideRunfiles = Maps.newHashMapWithExpectedSize(0); Map<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets = @@ -1214,6 +1256,7 @@ ActionInputMapHelper.addToMap( inputArtifactData, expandedArtifacts, + archivedTreeArtifacts, filesetsInsideRunfiles, topLevelFilesets, input, @@ -1253,7 +1296,11 @@ throw createMissingInputsException(action, missingArtifactCauses); } return accumulateInputResultsFactory.create( - inputArtifactData, expandedArtifacts, filesetsInsideRunfiles, topLevelFilesets); + inputArtifactData, + expandedArtifacts, + archivedTreeArtifacts, + filesetsInsideRunfiles, + topLevelFilesets); } /** @@ -1303,6 +1350,8 @@ Maps.newHashMapWithExpectedSize(0); S inputArtifactData = actionInputMapSinkFactory.apply(allInputsList.size()); Map<Artifact, Collection<Artifact>> expandedArtifacts = Maps.newHashMapWithExpectedSize(128); + Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts = + Maps.newHashMapWithExpectedSize(128); for (Artifact input : allInputsList) { SkyValue value = ArtifactNestedSetFunction.getInstance().getValueForKey(Artifact.key(input)); @@ -1318,6 +1367,7 @@ ActionInputMapHelper.addToMap( inputArtifactData, expandedArtifacts, + archivedTreeArtifacts, filesetsInsideRunfiles, topLevelFilesets, input, @@ -1329,7 +1379,11 @@ actionExecutionFunctionExceptionHandler.maybeThrowException(); return accumulateInputResultsFactory.create( - inputArtifactData, expandedArtifacts, filesetsInsideRunfiles, topLevelFilesets); + inputArtifactData, + expandedArtifacts, + archivedTreeArtifacts, + filesetsInsideRunfiles, + topLevelFilesets); } static LabelCause handleMissingFile( @@ -1410,6 +1464,7 @@ ActionInputMap inputArtifactData = null; Map<Artifact, Collection<Artifact>> expandedArtifacts = null; + Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts = null; ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> filesetsInsideRunfiles = null; ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets = null; Token token = null; @@ -1430,6 +1485,7 @@ boolean hasArtifactData() { boolean result = inputArtifactData != null; Preconditions.checkState(result == (expandedArtifacts != null), this); + Preconditions.checkState(result == (archivedTreeArtifacts != null), this); return result; }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionState.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionState.java index e9c4f24..fd04a88 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionState.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionState.java
@@ -19,6 +19,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionException; import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.bugreport.BugReport; +import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.skyframe.SkyFunction; import javax.annotation.Nullable; import javax.annotation.concurrent.GuardedBy; @@ -111,7 +112,10 @@ result = state.get(); } sharedActionCallback.actionCompleted(); - return result.transformForSharedAction(action.getOutputs()); + return result.transformForSharedAction( + action.getOutputs(), + action, + errorMessage -> env.getListener().handle(Event.error(errorMessage))); } private ActionExecutionValue runStateMachine(SkyFunction.Environment env)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java index e021d3bb..825ec57 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
@@ -20,7 +20,10 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; +import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionExecutionException; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; @@ -30,9 +33,14 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.lib.server.FailureDetails; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation; +import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.skyframe.SkyValue; import java.util.Map; +import java.util.NoSuchElementException; import java.util.function.BiFunction; +import java.util.function.Consumer; import javax.annotation.Nullable; /** A value representing an executed action. */ @@ -139,6 +147,17 @@ if (artifact.isChildOfDeclaredDirectory()) { TreeArtifactValue tree = treeArtifactData.get(artifact.getParent()); result = tree == null ? null : tree.getChildValues().get(artifact); + } else if (artifact instanceof ArchivedTreeArtifact) { + TreeArtifactValue tree = treeArtifactData.get(artifact.getParent()); + ArchivedRepresentation archivedRepresentation = + tree.getArchivedRepresentation() + .orElseThrow( + () -> new NoSuchElementException("Missing archived representation in: " + tree)); + Preconditions.checkArgument( + archivedRepresentation.archivedTreeFileArtifact().equals(artifact), + "Multiple archived tree artifacts for: %s", + artifact.getParent()); + result = archivedRepresentation.archivedFileValue(); } else { result = artifactData.get(artifact); } @@ -282,9 +301,35 @@ return newTree.build(); } - ActionExecutionValue transformForSharedAction(ImmutableSet<Artifact> outputs) { + ActionExecutionValue transformForSharedAction( + ImmutableSet<Artifact> outputs, Action actionForError, Consumer<String> errorReporter) + throws ActionExecutionException { Map<OwnerlessArtifactWrapper, Artifact> newArtifactMap = Maps.uniqueIndex(outputs, OwnerlessArtifactWrapper::new); + + if (treeArtifactData.values().stream() + .anyMatch(treeValue -> treeValue.getArchivedRepresentation().isPresent())) { + // TODO(b/163543290): Add support for sending archived tree artifacts produced by shared + // actions. + String message = + "Shared actions that produce tree artifacts are not supported when" + + " '--experimental_send_archived_tree_artifact_inputs' is enabled"; + errorReporter.accept(message); + throw new ActionExecutionException( + message, + actionForError, + /*catastrophe=*/ true, + DetailedExitCode.of( + FailureDetails.FailureDetail.newBuilder() + .setMessage(message) + .setExecution( + FailureDetails.Execution.newBuilder() + .setCode( + FailureDetails.Execution.Code + .SEND_ARCHIVED_TREE_ARTIFACT_INPUTS_PREREQ_UNMET)) + .build())); + } + // This is only called for shared actions, so we'll almost certainly have to transform all keys // in all sets. // Discovered modules come from the action's inputs, and so don't need to be transformed.
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 0a90d9a..38cf911 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
@@ -22,6 +22,7 @@ import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -45,6 +46,7 @@ static void addToMap( ActionInputMapSink inputMap, Map<Artifact, Collection<Artifact>> expandedArtifacts, + Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts, Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesetsInsideRunfiles, Map<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets, Artifact key, @@ -69,6 +71,7 @@ entry.getFirst(), Preconditions.checkNotNull(entry.getSecond()), expandedArtifacts, + archivedTreeArtifacts, inputMap, /*depOwner=*/ key); } @@ -90,7 +93,12 @@ } } else if (value instanceof TreeArtifactValue) { expandTreeArtifactAndPopulateArtifactData( - key, (TreeArtifactValue) value, expandedArtifacts, inputMap, /*depOwner=*/ key); + key, + (TreeArtifactValue) value, + expandedArtifacts, + archivedTreeArtifacts, + inputMap, + /*depOwner=*/ key); } else if (value instanceof ActionExecutionValue) { inputMap.put(key, ((ActionExecutionValue) value).getExistingFileArtifactValue(key), key); if (key.isFileset()) { @@ -157,6 +165,7 @@ Artifact treeArtifact, TreeArtifactValue value, Map<Artifact, Collection<Artifact>> expandedArtifacts, + Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts, ActionInputMapSink inputMap, Artifact depOwner) { if (TreeArtifactValue.OMITTED_TREE_MARKER.equals(value)) { @@ -172,5 +181,18 @@ expandedArtifacts.put(treeArtifact, children.build()); // Again, we cache the "digest" of the value for cache checking. inputMap.put(treeArtifact, value.getMetadata(), depOwner); + + value + .getArchivedRepresentation() + .ifPresent( + archivedRepresentation -> { + inputMap.put( + archivedRepresentation.archivedTreeFileArtifact(), + archivedRepresentation.archivedFileValue(), + depOwner); + archivedTreeArtifacts.put( + (SpecialArtifact) treeArtifact, + archivedRepresentation.archivedTreeFileArtifact()); + }); } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 777fcc2..c6c6aa6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; @@ -37,6 +38,7 @@ import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.actions.cache.DigestUtils; import com.google.devtools.build.lib.actions.cache.MetadataHandler; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileStatus; @@ -86,24 +88,29 @@ static ActionMetadataHandler create( ActionInputMap inputArtifactData, boolean forInputDiscovery, + boolean archivedTreeArtifactsEnabled, ImmutableSet<Artifact> outputs, TimestampGranularityMonitor tsgm, ArtifactPathResolver artifactPathResolver, PathFragment execRoot, + PathFragment derivedPathPrefix, Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets) { return new ActionMetadataHandler( inputArtifactData, forInputDiscovery, + archivedTreeArtifactsEnabled, outputs, tsgm, artifactPathResolver, execRoot, + derivedPathPrefix, createFilesetMapping(expandedFilesets, execRoot), new OutputStore()); } private final ActionInputMap inputArtifactData; private final boolean forInputDiscovery; + private final boolean archivedTreeArtifactsEnabled; private final ImmutableMap<PathFragment, FileArtifactValue> filesetMapping; private final Set<Artifact> omittedOutputs = Sets.newConcurrentHashSet(); @@ -112,6 +119,7 @@ private final TimestampGranularityMonitor tsgm; private final ArtifactPathResolver artifactPathResolver; private final PathFragment execRoot; + private final PathFragment derivedPathPrefix; private final AtomicBoolean executionMode = new AtomicBoolean(false); private final OutputStore store; @@ -119,18 +127,22 @@ private ActionMetadataHandler( ActionInputMap inputArtifactData, boolean forInputDiscovery, + boolean archivedTreeArtifactsEnabled, ImmutableSet<Artifact> outputs, TimestampGranularityMonitor tsgm, ArtifactPathResolver artifactPathResolver, PathFragment execRoot, + PathFragment derivedPathPrefix, ImmutableMap<PathFragment, FileArtifactValue> filesetMapping, OutputStore store) { this.inputArtifactData = checkNotNull(inputArtifactData); this.forInputDiscovery = forInputDiscovery; + this.archivedTreeArtifactsEnabled = archivedTreeArtifactsEnabled; this.outputs = checkNotNull(outputs); this.tsgm = checkNotNull(tsgm); this.artifactPathResolver = checkNotNull(artifactPathResolver); this.execRoot = checkNotNull(execRoot); + this.derivedPathPrefix = checkNotNull(derivedPathPrefix); this.filesetMapping = checkNotNull(filesetMapping); this.store = checkNotNull(store); } @@ -150,10 +162,12 @@ return new ActionMetadataHandler( inputArtifactData, /*forInputDiscovery=*/ false, + archivedTreeArtifactsEnabled, outputs, tsgm, artifactPathResolver, execRoot, + derivedPathPrefix, filesetMapping, store); } @@ -348,6 +362,15 @@ tree.putChild(child, metadata); }); + if (archivedTreeArtifactsEnabled) { + ArchivedTreeArtifact archivedTreeArtifact = + ArchivedTreeArtifact.create(parent, derivedPathPrefix); + tree.setArchivedRepresentation( + ArchivedRepresentation.create( + archivedTreeArtifact, + constructFileArtifactValueFromFilesystem(archivedTreeArtifact))); + } + return tree.build(); } @@ -389,6 +412,11 @@ checkArgument(isKnownOutput(output), "%s is not a declared output of this action", output); checkArgument(output.isTreeArtifact(), "Output must be a tree artifact: %s", output); checkState(executionMode.get(), "Tried to inject %s outside of execution", output); + checkArgument( + archivedTreeArtifactsEnabled == tree.getArchivedRepresentation().isPresent(), + "Archived representation presence mismatched for: %s with archivedTreeArtifactsEnabled: %s", + tree, + archivedTreeArtifactsEnabled); store.putTreeArtifactData(output, tree); }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index b9edfcc..b1278e7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -392,6 +392,7 @@ ":action_execution_value", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/bugreport", + "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/skyframe", "//third_party:guava", "//third_party:jsr305", @@ -404,12 +405,15 @@ deps = [ ":output_store", ":tree_artifact_value", + "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/actions:fileset_output_symlink", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", + "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//src/main/protobuf:failure_details_java_proto", "//third_party:guava", "//third_party:jsr305", ], @@ -2692,6 +2696,7 @@ "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//third_party:auto_value", "//third_party:guava", "//third_party:jsr305", ],
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 d7c1be6..63f130d 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
@@ -20,6 +20,8 @@ import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.CompletionContext; import com.google.devtools.build.lib.actions.CompletionContext.PathResolverFactory; import com.google.devtools.build.lib.actions.FilesetOutputSymlink; @@ -192,6 +194,7 @@ ActionInputMap inputMap = new ActionInputMap(inputDeps.size()); Map<Artifact, Collection<Artifact>> expandedArtifacts = new HashMap<>(); Map<Artifact, ImmutableList<FilesetOutputSymlink>> expandedFilesets = new HashMap<>(); + Map<SpecialArtifact, ArchivedTreeArtifact> archivedTreeArtifacts = new HashMap<>(); Map<Artifact, ImmutableList<FilesetOutputSymlink>> topLevelFilesets = new HashMap<>(); int missingCount = 0; @@ -217,6 +220,7 @@ ActionInputMapHelper.addToMap( inputMap, expandedArtifacts, + archivedTreeArtifacts, expandedFilesets, topLevelFilesets, input, @@ -270,6 +274,7 @@ ctx = CompletionContext.create( expandedArtifacts, + archivedTreeArtifacts, expandedFilesets, key.topLevelArtifactContext().expandFilesets(), key.topLevelArtifactContext().fullyResolveFilesetSymlinks(),
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java index d3bdf1d..a6beab7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
@@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; +import com.google.common.collect.Maps; import com.google.common.collect.Range; import com.google.common.collect.Sets; import com.google.common.flogger.GoogleLogger; @@ -37,6 +38,7 @@ import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.skyframe.SkyValueDirtinessChecker.DirtyResult; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation; import com.google.devtools.build.lib.util.LoggingUtil; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; @@ -294,6 +296,14 @@ fileToKeyAndValue.put(child, keyAndValue); } } + tree.getArchivedRepresentation() + .map(ArchivedRepresentation::archivedTreeFileArtifact) + .filter( + archivedTreeArtifact -> + shouldCheckFile(knownModifiedOutputFiles, archivedTreeArtifact)) + .ifPresent( + archivedTreeArtifact -> + fileToKeyAndValue.put(archivedTreeArtifact, keyAndValue)); if (shouldCheckTreeArtifact(sortedKnownModifiedOutputFiles.get(), treeArtifact)) { treeArtifactsToKeyAndValue.put(treeArtifact, keyAndValue); } @@ -500,6 +510,18 @@ isDirty = true; } } + isDirty = + isDirty + || tree.getArchivedRepresentation() + .map( + archivedRepresentation -> + artifactIsDirtyWithDirectSystemCalls( + knownModifiedOutputFiles, + trustRemoteArtifacts, + Maps.immutableEntry( + archivedRepresentation.archivedTreeFileArtifact(), + archivedRepresentation.archivedFileValue()))) + .orElse(false); } Artifact treeArtifact = entry.getKey();
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 1cdfeb4..1e94a7f 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
@@ -73,6 +73,7 @@ import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.actions.cache.MetadataHandler; import com.google.devtools.build.lib.actions.cache.MetadataInjector; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.buildtool.BuildRequestOptions; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -287,6 +288,10 @@ return executorEngine.getExecRoot(); } + boolean useArchivedTreeArtifacts() { + return options.getOptions(CoreOptions.class).sendArchivedTreeArtifactInputs; + } + /** REQUIRES: {@link #actionFileSystemType()} to be not {@code DISABLED}. */ FileSystem createActionFileSystem( String relativeOutputPath,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java index ec36e83..df63590 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java
@@ -15,12 +15,17 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import com.google.auto.value.AutoValue; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -38,6 +43,7 @@ import java.util.Arrays; import java.util.HashMap; import java.util.Map; +import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import javax.annotation.Nullable; @@ -75,6 +81,31 @@ MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata); /** + * Sets the archived representation and its metadata for the {@linkplain + * ArchivedTreeArtifact#getParent parent} of the provided tree artifact. + * + * <p>Setting an archived representation is only allowed once per {@linkplain SpecialArtifact + * tree artifact}. + */ + MultiBuilder setArchivedRepresentation( + ArchivedTreeArtifact archivedArtifact, FileArtifactValue metadata); + + /** + * Make sure the builder will inject a {@link TreeArtifactValue} for a given {@linkplain + * SpecialArtifact tree artifact}. + * + * <p>Convenience method allowing to construct potentially empty {@link TreeArtifactValue} with + * a {@link MultiBuilder}. + * + * <p>There is no need to call this method before {@link #putChild(TreeFileArtifact, + * FileArtifactValue)} or {@link #setArchivedRepresentation(ArchivedTreeArtifact, + * FileArtifactValue)} since both methods implicitly make sure that the {@link + * TreeArtifactValue} will be created for the related {@linkplain SpecialArtifact parent tree + * artifact}. + */ + MultiBuilder addTreeArtifact(SpecialArtifact treeArtifact); + + /** * For each unique parent seen by this builder, passes the aggregated metadata to {@link * TreeArtifactInjector#injectTree}. */ @@ -91,23 +122,54 @@ return new ConcurrentMultiBuilder(); } - @SerializationConstant @AutoCodec.VisibleForSerialization + /** + * Archived representation of a tree artifact which contains a representation of the filesystem + * tree starting with the tree artifact directory. + * + * <p>Contains both the {@linkplain ArchivedTreeArtifact artifact} for the archived file and the + * metadata for it. + */ + @AutoValue + abstract static class ArchivedRepresentation { + abstract ArchivedTreeArtifact archivedTreeFileArtifact(); + + abstract FileArtifactValue archivedFileValue(); + + static ArchivedRepresentation create( + ArchivedTreeArtifact archivedTreeFileArtifact, FileArtifactValue fileArtifactValue) { + return new AutoValue_TreeArtifactValue_ArchivedRepresentation( + archivedTreeFileArtifact, fileArtifactValue); + } + } + + @SuppressWarnings("WeakerAccess") // Serialization constant. + @SerializationConstant + @AutoCodec.VisibleForSerialization static final TreeArtifactValue EMPTY = new TreeArtifactValue( DigestUtils.fromMetadata(ImmutableMap.of()), ImmutableSortedMap.of(), + /*archivedRepresentation=*/ null, /*entirelyRemote=*/ false); private final byte[] digest; private final ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> childData; + /** + * Optional archived representation of the entire tree artifact which can be sent instead of all + * the items in the directory. + */ + @Nullable private final ArchivedRepresentation archivedRepresentation; + private final boolean entirelyRemote; private TreeArtifactValue( byte[] digest, ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> childData, + @Nullable ArchivedRepresentation archivedRepresentation, boolean entirelyRemote) { this.digest = digest; this.childData = childData; + this.archivedRepresentation = archivedRepresentation; this.entirelyRemote = entirelyRemote; } @@ -130,6 +192,16 @@ return childData.keySet(); } + /** Return archived representation of the tree artifact (if present). */ + Optional<ArchivedRepresentation> getArchivedRepresentation() { + return Optional.ofNullable(archivedRepresentation); + } + + @VisibleForTesting + public boolean hasArchivedArtifactForTesting() { + return archivedRepresentation != null; + } + ImmutableMap<TreeFileArtifact, FileArtifactValue> getChildValues() { return childData; } @@ -184,7 +256,11 @@ static final TreeArtifactValue MISSING_TREE_ARTIFACT = createMarker("MISSING_TREE_ARTIFACT"); private static TreeArtifactValue createMarker(String toStringRepresentation) { - return new TreeArtifactValue(null, ImmutableSortedMap.of(), /*entirelyRemote=*/ false) { + return new TreeArtifactValue( + null, + ImmutableSortedMap.of(), + /*archivedRepresentation=*/ null, + /*entirelyRemote=*/ false) { @Override public ImmutableSet<TreeFileArtifact> getChildren() { throw new UnsupportedOperationException(toString()); @@ -318,6 +394,7 @@ public static final class Builder { private final ImmutableSortedMap.Builder<TreeFileArtifact, FileArtifactValue> childData = ImmutableSortedMap.naturalOrder(); + private ArchivedRepresentation archivedRepresentation; private final SpecialArtifact parent; Builder(SpecialArtifact parent) { @@ -334,7 +411,9 @@ * <p>Children may be added in any order. The children are sorted prior to constructing the * final {@link TreeArtifactValue}. * - * <p>It is illegal to call this method with {@link FileArtifactValue.OMITTED_FILE_MARKER}. When + * <p>It is illegal to call this method with {@link FileArtifactValue#OMITTED_FILE_MARKER}. When + * + * <p>It is illegal to call this method with {@link FileArtifactValue#OMITTED_FILE_MARKER}. When * children are omitted, use {@link TreeArtifactValue#OMITTED_TREE_MARKER}. * * @return {@code this} for convenience @@ -355,15 +434,30 @@ return this; } + public Builder setArchivedRepresentation(ArchivedRepresentation archivedRepresentation) { + checkState( + this.archivedRepresentation == null, + "Tried to add 2 archived representations for: %s", + archivedRepresentation); + checkArgument( + archivedRepresentation.archivedTreeFileArtifact().getParent().equals(parent), + "Cannot add archived representation: %s for a mismatching tree artifact: %s", + archivedRepresentation, + parent); + this.archivedRepresentation = archivedRepresentation; + return this; + } + /** Builds the final {@link TreeArtifactValue}. */ public TreeArtifactValue build() { ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> finalChildData = childData.build(); - if (finalChildData.isEmpty()) { + if (finalChildData.isEmpty() && archivedRepresentation == null) { return EMPTY; } Fingerprint fingerprint = new Fingerprint(); - boolean entirelyRemote = true; + boolean entirelyRemote = + archivedRepresentation == null || archivedRepresentation.archivedFileValue().isRemote(); for (Map.Entry<TreeFileArtifact, FileArtifactValue> childData : finalChildData.entrySet()) { // Digest will be deterministic because children are sorted. @@ -374,7 +468,12 @@ entirelyRemote &= childData.getValue().isRemote(); } - return new TreeArtifactValue(fingerprint.digestAndReset(), finalChildData, entirelyRemote); + if (archivedRepresentation != null) { + archivedRepresentation.archivedFileValue().addTo(fingerprint); + } + + return new TreeArtifactValue( + fingerprint.digestAndReset(), finalChildData, archivedRepresentation, entirelyRemote); } } @@ -388,6 +487,22 @@ } @Override + public MultiBuilder setArchivedRepresentation( + ArchivedTreeArtifact archivedArtifact, FileArtifactValue metadata) { + map.computeIfAbsent(archivedArtifact.getParent(), Builder::new) + .setArchivedRepresentation(ArchivedRepresentation.create(archivedArtifact, metadata)); + return this; + } + + @Override + public MultiBuilder addTreeArtifact(SpecialArtifact treeArtifact) { + Preconditions.checkArgument( + treeArtifact.isTreeArtifact(), "Not a tree artifact: %s", treeArtifact); + map.computeIfAbsent(treeArtifact, Builder::new); + return this; + } + + @Override public void injectTo(TreeArtifactInjector treeInjector) { map.forEach((parent, builder) -> treeInjector.injectTree(parent, builder.build())); } @@ -396,21 +511,50 @@ @ThreadSafe private static final class ConcurrentMultiBuilder implements MultiBuilder { private final ConcurrentMap<SpecialArtifact, ConcurrentMap<TreeFileArtifact, FileArtifactValue>> - map = new ConcurrentHashMap<>(); + children = new ConcurrentHashMap<>(); + private final ConcurrentMap<SpecialArtifact, ArchivedRepresentation> archivedRepresentations = + new ConcurrentHashMap<>(); @Override public MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata) { - map.computeIfAbsent(child.getParent(), parent -> new ConcurrentHashMap<>()) + children + .computeIfAbsent(child.getParent(), parent -> new ConcurrentHashMap<>()) .put(child, metadata); return this; } @Override + public MultiBuilder setArchivedRepresentation( + ArchivedTreeArtifact archivedArtifact, FileArtifactValue metadata) { + Object oldValue = + archivedRepresentations.putIfAbsent( + archivedArtifact.getParent(), + ArchivedRepresentation.create(archivedArtifact, metadata)); + Preconditions.checkArgument( + oldValue == null, + "Tried to add 2 archived representations for %s", + archivedArtifact.getParent()); + return this; + } + + @Override + public MultiBuilder addTreeArtifact(SpecialArtifact treeArtifact) { + Preconditions.checkArgument( + treeArtifact.isTreeArtifact(), "Not a tree artifact: %s", treeArtifact); + children.computeIfAbsent(treeArtifact, ignored -> new ConcurrentHashMap<>()); + return null; + } + + @Override public void injectTo(TreeArtifactInjector treeInjector) { - map.forEach( + children.forEach( (parent, children) -> { Builder builder = new Builder(parent); children.forEach(builder::putChild); + ArchivedRepresentation archivedRepresentation = archivedRepresentations.get(parent); + if (archivedRepresentation != null) { + builder.setArchivedRepresentation(archivedRepresentation); + } treeInjector.injectTree(parent, builder.build()); }); }
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/BatchStat.java b/src/main/java/com/google/devtools/build/lib/vfs/BatchStat.java index 601f507..73289b8 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/BatchStat.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/BatchStat.java
@@ -22,7 +22,6 @@ public interface BatchStat { /** - * * @param includeDigest whether to include a file digest in the return values. * @param includeLinks whether to include a symlink stat in the return values. * @param paths The input paths to stat(), relative to the exec root.
diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index dfff759..50e29a5 100644 --- a/src/main/protobuf/failure_details.proto +++ b/src/main/protobuf/failure_details.proto
@@ -415,6 +415,8 @@ CYCLE = 35 [(metadata) = { exit_code: 1 }]; SOURCE_INPUT_MISSING = 36 [(metadata) = { exit_code: 1 }]; UNEXPECTED_EXCEPTION = 37 [(metadata) = { exit_code: 1 }]; + SEND_ARCHIVED_TREE_ARTIFACT_INPUTS_PREREQ_UNMET = 38 + [(metadata) = { exit_code: 2 }]; } Code code = 1;
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java index d59daff..0038aba 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
@@ -74,8 +74,9 @@ private final ArtifactRoot sourceRoot = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.resolve("/workspace"))); + private final PathFragment derivedPathPrefix = PathFragment.create("bin"); private final ArtifactRoot outputRoot = - ArtifactRoot.asDerivedRoot(scratch.resolve("/output"), "bin"); + ArtifactRoot.asDerivedRoot(scratch.resolve("/output"), derivedPathPrefix); private final Path execRoot = outputRoot.getRoot().asPath(); @Before @@ -89,10 +90,12 @@ return ActionMetadataHandler.create( inputMap, forInputDiscovery, + /*archivedTreeArtifactsEnabled=*/ false, outputs, tsgm, ArtifactPathResolver.IDENTITY, execRoot.asFragment(), + derivedPathPrefix, /*expandedFilesets=*/ ImmutableMap.of()); } @@ -431,10 +434,12 @@ ActionMetadataHandler.create( new ActionInputMap(0), /*forInputDiscovery=*/ false, + /*archivedTreeArtifactsEnabled=*/ false, /*outputs=*/ ImmutableSet.of(), tsgm, ArtifactPathResolver.IDENTITY, execRoot.asFragment(), + derivedPathPrefix, expandedFilesets); // Only the regular FileArtifactValue should have its metadata stored.
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerParameterizedTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerParameterizedTest.java new file mode 100644 index 0000000..8051c56 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerParameterizedTest.java
@@ -0,0 +1,257 @@ +// Copyright 2020 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.skyframe; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.actions.ActionLookupData; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; +import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.vfs.BatchStat; +import com.google.devtools.build.lib.vfs.FileStatusWithDigest; +import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.ModifiedFileSet; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Symlinks; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import javax.annotation.Nullable; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +/** {@link FilesystemValueChecker} checker parameterized on {@link BatchStatMode}. */ +@RunWith(Parameterized.class) +public final class FilesystemValueCheckerParameterizedTest extends FilesystemValueCheckerTestBase { + + private static final ActionLookupData ACTION_LOOKUP_DATA = actionLookupData(0); + + @Parameter public BatchStatMode batchStat; + + @Parameters(name = "batchStat={0}") + public static ImmutableList<Object[]> batchStatModes() { + return Arrays.stream(BatchStatMode.values()) + .map(mode -> new BatchStatMode[] {mode}) + .collect(toImmutableList()); + } + + @Test + public void getDirtyActionValues_unchangedEmptyTreeArtifactWithArchivedFile_noDirtyKeys() + throws Exception { + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + treeArtifact.getPath().createDirectoryAndParents(); + ActionExecutionValue actionExecutionValue = + actionValueWithTreeArtifacts( + ImmutableList.of(), + ImmutableList.of(createArchivedTreeArtifactWithContent(treeArtifact))); + + assertThat(getDirtyActionValues(actionExecutionValue)).isEmpty(); + } + + @Test + public void getDirtyActionValues_unchangedTreeArtifactWithArchivedFile_noDirtyKeys() + throws Exception { + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + ActionExecutionValue actionExecutionValue = + actionValueWithTreeArtifacts( + ImmutableList.of( + createTreeFileArtifactWithContent(treeArtifact, "file1", "content"), + createTreeFileArtifactWithContent(treeArtifact, "file2", "content2")), + ImmutableList.of(createArchivedTreeArtifactWithContent(treeArtifact))); + + assertThat(getDirtyActionValues(actionExecutionValue)).isEmpty(); + } + + @Test + public void getDirtyActionValues_editedArchivedFileForEmptyTreeArtifact_reportsChange() + throws Exception { + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + treeArtifact.getPath().createDirectoryAndParents(); + ArchivedTreeArtifact archivedTreeArtifact = + createArchivedTreeArtifactWithContent(treeArtifact, "old content"); + ActionExecutionValue actionExecutionValue = + actionValueWithTreeArtifacts(ImmutableList.of(), ImmutableList.of(archivedTreeArtifact)); + + writeFile(archivedTreeArtifact.getPath(), "new content"); + assertThat(getDirtyActionValues(actionExecutionValue)).containsExactly(ACTION_LOOKUP_DATA); + } + + @Test + public void getDirtyActionValues_editedArchivedFileForTreeArtifact_reportsChange() + throws Exception { + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + ArchivedTreeArtifact archivedTreeArtifact = + createArchivedTreeArtifactWithContent(treeArtifact, "old content"); + ActionExecutionValue actionExecutionValue = + actionValueWithTreeArtifacts( + ImmutableList.of( + createTreeFileArtifactWithContent( + treeArtifact, /*parentRelativePath=*/ "file1", "content"), + createTreeFileArtifactWithContent( + treeArtifact, /*parentRelativePath=*/ "file2", "content2")), + ImmutableList.of(archivedTreeArtifact)); + + writeFile(archivedTreeArtifact.getPath(), "new content"); + assertThat(getDirtyActionValues(actionExecutionValue)).containsExactly(ACTION_LOOKUP_DATA); + } + + @Test + public void getDirtyActionValues_deletedArchivedFileForTreeArtifact_reportsChange() + throws Exception { + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + ArchivedTreeArtifact archivedTreeArtifact = createArchivedTreeArtifactWithContent(treeArtifact); + ActionExecutionValue actionExecutionValue = + actionValueWithTreeArtifacts( + ImmutableList.of( + createTreeFileArtifactWithContent( + treeArtifact, /*parentRelativePath=*/ "file1", "content"), + createTreeFileArtifactWithContent( + treeArtifact, /*parentRelativePath=*/ "file2", "content2")), + ImmutableList.of(archivedTreeArtifact)); + + archivedTreeArtifact.getPath().delete(); + assertThat(getDirtyActionValues(actionExecutionValue)).containsExactly(ACTION_LOOKUP_DATA); + } + + @Test + public void getDirtyActionValues_deletedArchivedFileForEmptyTreeArtifact_reportsChange() + throws Exception { + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + ArchivedTreeArtifact archivedTreeArtifact = createArchivedTreeArtifactWithContent(treeArtifact); + ActionExecutionValue actionExecutionValue = + actionValueWithTreeArtifacts(ImmutableList.of(), ImmutableList.of(archivedTreeArtifact)); + + archivedTreeArtifact.getPath().delete(); + assertThat(getDirtyActionValues(actionExecutionValue)).containsExactly(ACTION_LOOKUP_DATA); + } + + @Test + public void getDirtyActionValues_editedFileForTreeArtifactWithArchivedFile_reportsChange() + throws Exception { + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + TreeFileArtifact child1 = + createTreeFileArtifactWithContent( + treeArtifact, /*parentRelativePath=*/ "file1", "old content"); + ActionExecutionValue actionExecutionValue = + actionValueWithTreeArtifacts( + ImmutableList.of( + child1, + createTreeFileArtifactWithContent( + treeArtifact, /*parentRelativePath=*/ "file2", "content2")), + ImmutableList.of(createArchivedTreeArtifactWithContent(treeArtifact))); + + writeFile(child1.getPath(), "new content"); + assertThat(getDirtyActionValues(actionExecutionValue)).containsExactly(ACTION_LOOKUP_DATA); + } + + @Test + public void getDirtyActionValues_treeArtifactWithArchivedArtifact_reportsOnlyChangedKey() + throws Exception { + SpecialArtifact unchangedTreeArtifact = createTreeArtifact("dir1"); + ActionExecutionValue unchangedValue = + actionValueWithTreeArtifacts( + ImmutableList.of(createTreeFileArtifactWithContent(unchangedTreeArtifact, "child")), + ImmutableList.of(createArchivedTreeArtifactWithContent(unchangedTreeArtifact))); + SpecialArtifact changedTreeArtifact = createTreeArtifact("dir2"); + ArchivedTreeArtifact changedArchivedTreeArtifact = + createArchivedTreeArtifactWithContent(changedTreeArtifact, "old content"); + ActionExecutionValue changedValue = + actionValueWithTreeArtifacts( + ImmutableList.of( + createTreeFileArtifactWithContent(changedTreeArtifact, "file", "content")), + ImmutableList.of(changedArchivedTreeArtifact)); + + writeFile(changedArchivedTreeArtifact.getPath(), "new content"); + assertThat( + getDirtyActionValues( + ImmutableMap.of( + actionLookupData(0), unchangedValue, actionLookupData(1), changedValue))) + .containsExactly(actionLookupData(1)); + } + + private Collection<SkyKey> getDirtyActionValues(ActionExecutionValue actionExecutionValue) + throws InterruptedException { + return getDirtyActionValues(ImmutableMap.of(ACTION_LOOKUP_DATA, actionExecutionValue)); + } + + private Collection<SkyKey> getDirtyActionValues(ImmutableMap<SkyKey, SkyValue> valuesMap) + throws InterruptedException { + return new FilesystemValueChecker( + /*tsgm=*/ null, /*lastExecutionTimeRange=*/ null, FSVC_THREADS_FOR_TEST) + .getDirtyActionValues( + valuesMap, + batchStat.getBatchStat(fs), + ModifiedFileSet.EVERYTHING_MODIFIED, + /*trustRemoteArtifacts=*/ false); + } + + private TreeFileArtifact createTreeFileArtifactWithContent( + SpecialArtifact treeArtifact, String parentRelativePath, String... contentLines) + throws IOException { + TreeFileArtifact artifact = TreeFileArtifact.createTreeOutput(treeArtifact, parentRelativePath); + writeFile(artifact.getPath(), contentLines); + return artifact; + } + + private ArchivedTreeArtifact createArchivedTreeArtifactWithContent( + SpecialArtifact treeArtifact, String... contentLines) throws IOException { + ArchivedTreeArtifact artifact = + ArchivedTreeArtifact.create(treeArtifact, PathFragment.create("bin")); + writeFile(artifact.getPath(), contentLines); + return artifact; + } + + private static ActionLookupData actionLookupData(int actionIndex) { + return ActionLookupData.create(ACTION_LOOKUP_KEY, actionIndex); + } + + private enum BatchStatMode { + DISABLED { + @Nullable + @Override + BatchStat getBatchStat(FileSystem fileSystem) { + return null; + } + }, + ENABLED { + @Override + BatchStat getBatchStat(FileSystem fileSystem) { + return (useDigest, includeLinks, paths) -> { + List<FileStatusWithDigest> stats = new ArrayList<>(); + for (PathFragment pathFrag : paths) { + stats.add( + FileStatusWithDigestAdapter.adapt( + fileSystem.getPath("/").getRelative(pathFrag).statIfFound(Symlinks.NOFOLLOW))); + } + return stats; + }; + } + }; + + @Nullable + abstract BatchStat getBatchStat(FileSystem fileSystem); + } +}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java index 5bd1ed4..cae0cb1 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
@@ -24,7 +24,6 @@ import com.google.common.util.concurrent.Runnables; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionLookupData; -import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; @@ -38,7 +37,6 @@ import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ServerDirectories; import com.google.devtools.build.lib.clock.BlazeClock; -import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.events.NullEventHandler; @@ -66,7 +64,6 @@ import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.UnixGlob; -import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.build.skyframe.Differencer.Diff; import com.google.devtools.build.skyframe.EvaluationContext; import com.google.devtools.build.skyframe.EvaluationResult; @@ -97,7 +94,7 @@ /** Tests for {@link FilesystemValueChecker}. */ @RunWith(JUnit4.class) -public final class FilesystemValueCheckerTest { +public final class FilesystemValueCheckerTest extends FilesystemValueCheckerTestBase { private static final EvaluationContext EVALUATION_OPTIONS = EvaluationContext.newBuilder() .setKeepGoing(false) @@ -108,16 +105,12 @@ private RecordingDifferencer differencer; private MemoizingEvaluator evaluator; private SequentialBuildDriver driver; - private MockFileSystem fs; private Path pkgRoot; - private static final int FSVC_THREADS_FOR_TEST = 200; - @Before public final void setUp() throws Exception { ImmutableMap.Builder<SkyFunctionName, SkyFunction> skyFunctions = ImmutableMap.builder(); - fs = new MockFileSystem(); pkgRoot = fs.getPath("/testroot"); pkgRoot.createDirectoryAndParents(); FileSystemUtils.createEmptyFile(pkgRoot.getRelative("WORKSPACE")); @@ -318,9 +311,10 @@ // their ctime. TimestampGranularityUtils.waitForTimestampGranularity( System.currentTimeMillis(), OutErr.SYSTEM_OUT_ERR); - // Update path1's contents and mtime. This will update the file's ctime. + // Update path1's contents. This will update the file's ctime with current time indicated by the + // clock. + fs.advanceClockMillis(1); FileSystemUtils.writeContentAsLatin1(path1, "hello1"); - path1.setLastModifiedTime(27); // Update path2's mtime but not its contents. We expect that an mtime change suffices to update // the ctime. path2.setLastModifiedTime(42); @@ -387,9 +381,8 @@ FileSystemUtils.writeContentAsLatin1(out2.getPath(), "fizzlepop"); TimestampGranularityMonitor tsgm = new TimestampGranularityMonitor(BlazeClock.instance()); - ActionLookupKey actionLookupKey = new SimpleActionLookupKey(); - SkyKey actionKey1 = ActionLookupData.create(actionLookupKey, 0); - SkyKey actionKey2 = ActionLookupData.create(actionLookupKey, 1); + SkyKey actionKey1 = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); + SkyKey actionKey2 = ActionLookupData.create(ACTION_LOOKUP_KEY, 1); pretendBuildTwoArtifacts(out1, actionKey1, out2, actionKey2, batchStatter, tsgm); @@ -516,12 +509,11 @@ SpecialArtifact last = createTreeArtifact("zzzzzzzzzz"); last.getPath().createDirectoryAndParents(); - ActionLookupKey actionLookupKey = new SimpleActionLookupKey(); - SkyKey actionKey1 = ActionLookupData.create(actionLookupKey, 0); - SkyKey actionKey2 = ActionLookupData.create(actionLookupKey, 1); - SkyKey actionKeyEmpty = ActionLookupData.create(actionLookupKey, 2); - SkyKey actionKeyUnchanging = ActionLookupData.create(actionLookupKey, 3); - SkyKey actionKeyLast = ActionLookupData.create(actionLookupKey, 4); + SkyKey actionKey1 = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); + SkyKey actionKey2 = ActionLookupData.create(ACTION_LOOKUP_KEY, 1); + SkyKey actionKeyEmpty = ActionLookupData.create(ACTION_LOOKUP_KEY, 2); + SkyKey actionKeyUnchanging = ActionLookupData.create(ACTION_LOOKUP_KEY, 3); + SkyKey actionKeyLast = ActionLookupData.create(ACTION_LOOKUP_KEY, 4); differencer.inject( ImmutableMap.of( actionKey1, @@ -732,17 +724,6 @@ ArtifactRoot.asDerivedRoot(fs.getPath("/"), outSegment), outputPath.getRelative(relPath)); } - private SpecialArtifact createTreeArtifact(String relPath) throws IOException { - String outSegment = "bin"; - Path outputDir = fs.getPath("/" + outSegment); - Path outputPath = outputDir.getRelative(relPath); - outputDir.createDirectory(); - ArtifactRoot derivedRoot = ArtifactRoot.asDerivedRoot(fs.getPath("/"), outSegment); - return ActionsTestUtil.createTreeArtifactWithGeneratingAction( - derivedRoot, - derivedRoot.getExecPath().getRelative(derivedRoot.getRoot().relativize(outputPath))); - } - @Test // TODO(b/154337187): Remove the following annotation to re-enable once this test is de-flaked. @Ignore @@ -859,38 +840,6 @@ /*actionDependsOnBuildId=*/ false); } - private static ActionExecutionValue actionValueWithTreeArtifacts( - List<TreeFileArtifact> contents) { - TreeArtifactValue.MultiBuilder treeArtifacts = TreeArtifactValue.newMultiBuilder(); - - for (TreeFileArtifact output : contents) { - Path path = output.getPath(); - try { - FileArtifactValue noDigest = - ActionMetadataHandler.fileArtifactValueFromArtifact( - output, - FileStatusWithDigestAdapter.adapt(path.statIfFound(Symlinks.NOFOLLOW)), - null); - FileArtifactValue withDigest = - FileArtifactValue.createFromInjectedDigest( - noDigest, path.getDigest(), !output.isConstantMetadata()); - treeArtifacts.putChild(output, withDigest); - } catch (IOException e) { - throw new IllegalStateException(e); - } - } - - Map<Artifact, TreeArtifactValue> treeArtifactData = new HashMap<>(); - treeArtifacts.injectTo(treeArtifactData::put); - - return ActionExecutionValue.create( - /*artifactData=*/ ImmutableMap.of(), - treeArtifactData, - /*outputSymlinks=*/ null, - /*discoveredModules=*/ null, - /*actionDependsOnBuildId=*/ false); - } - private static ActionExecutionValue actionValueWithTreeArtifact( SpecialArtifact output, TreeArtifactValue tree) { return ActionExecutionValue.create( @@ -923,9 +872,8 @@ // Test that injected remote artifacts are trusted by the FileSystemValueChecker // if it is configured to trust remote artifacts, and that local files always take precedence // over remote files. - ActionLookupKey actionLookupKey = new SimpleActionLookupKey(); - SkyKey actionKey1 = ActionLookupData.create(actionLookupKey, 0); - SkyKey actionKey2 = ActionLookupData.create(actionLookupKey, 1); + SkyKey actionKey1 = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); + SkyKey actionKey2 = ActionLookupData.create(ACTION_LOOKUP_KEY, 1); Artifact out1 = createDerivedArtifact("foo"); Artifact out2 = createDerivedArtifact("bar"); @@ -975,8 +923,7 @@ public void testRemoteAndLocalTreeArtifacts() throws Exception { // Test that injected remote tree artifacts are trusted by the FileSystemValueChecker // and that local files always takes preference over remote files. - ActionLookupKey actionLookupKey = new SimpleActionLookupKey(); - SkyKey actionKey = ActionLookupData.create(actionLookupKey, 0); + SkyKey actionKey = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); SpecialArtifact treeArtifact = createTreeArtifact("dir"); treeArtifact.getPath().createDirectoryAndParents(); @@ -1053,27 +1000,6 @@ .containsExactlyElementsIn(Arrays.asList(keysWithNewValues)); } - private static final class MockFileSystem extends InMemoryFileSystem { - boolean statThrowsRuntimeException; - boolean readlinkThrowsIoException; - - @Override - public FileStatus statIfFound(Path path, boolean followSymlinks) throws IOException { - if (statThrowsRuntimeException) { - throw new RuntimeException("bork"); - } - return super.statIfFound(path, followSymlinks); - } - - @Override - protected PathFragment readSymbolicLink(Path path) throws IOException { - if (readlinkThrowsIoException) { - throw new IOException("readlink failed"); - } - return super.readSymbolicLink(path); - } - } - private static FileStatusWithDigest statWithDigest(final Path path, final FileStatus stat) { return new FileStatusWithDigest() { @Nullable @@ -1128,17 +1054,4 @@ FilesystemValueChecker checker) throws InterruptedException { return checker.getDirtyKeys(evaluator.getValues(), new BasicFilesystemDirtinessChecker()); } - - private static final class SimpleActionLookupKey implements ActionLookupKey { - @Override - public SkyFunctionName functionName() { - return SkyFunctionName.FOR_TESTING; - } - - @Nullable - @Override - public Label getLabel() { - return null; - } - } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTestBase.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTestBase.java new file mode 100644 index 0000000..09a1136 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTestBase.java
@@ -0,0 +1,155 @@ +// Copyright 2020 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.skyframe; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.actions.ActionLookupKey; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; +import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ArtifactRoot; +import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.util.ActionsTestUtil; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.testutil.ManualClock; +import com.google.devtools.build.lib.vfs.FileStatus; +import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Symlinks; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import com.google.devtools.build.skyframe.SkyFunctionName; +import java.io.IOException; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import javax.annotation.Nullable; + +/** + * Helper class to allow sharing test helpers between {@link FilesystemValueCheckerTest} and {@link + * FilesystemValueCheckerParameterizedTest}. + */ +public class FilesystemValueCheckerTestBase { + + static final int FSVC_THREADS_FOR_TEST = 200; + static final ActionLookupKey ACTION_LOOKUP_KEY = + new ActionLookupKey() { + @Override + public SkyFunctionName functionName() { + return SkyFunctionName.FOR_TESTING; + } + + @Nullable + @Override + public Label getLabel() { + return null; + } + }; + + final MockFileSystem fs = new MockFileSystem(); + + SpecialArtifact createTreeArtifact(String relPath) throws IOException { + String outSegment = "bin"; + Path outputDir = fs.getPath("/" + outSegment); + Path outputPath = outputDir.getRelative(relPath); + outputDir.createDirectory(); + ArtifactRoot derivedRoot = ArtifactRoot.asDerivedRoot(fs.getPath("/"), outSegment); + return ActionsTestUtil.createTreeArtifactWithGeneratingAction( + derivedRoot, + derivedRoot.getExecPath().getRelative(derivedRoot.getRoot().relativize(outputPath))); + } + + static ActionExecutionValue actionValueWithTreeArtifacts(List<TreeFileArtifact> contents) + throws IOException { + return actionValueWithTreeArtifacts(contents, ImmutableList.of()); + } + + static ActionExecutionValue actionValueWithTreeArtifacts( + Iterable<TreeFileArtifact> contents, Iterable<ArchivedTreeArtifact> archivedTreeArtifacts) + throws IOException { + TreeArtifactValue.MultiBuilder treeArtifacts = TreeArtifactValue.newMultiBuilder(); + + for (TreeFileArtifact output : contents) { + treeArtifacts.putChild(output, createMetadataFromFileSystem(output)); + } + + for (ArchivedTreeArtifact archivedTreeArtifact : archivedTreeArtifacts) { + treeArtifacts.setArchivedRepresentation( + archivedTreeArtifact, createMetadataFromFileSystem(archivedTreeArtifact)); + } + + Map<Artifact, TreeArtifactValue> treeArtifactData = new HashMap<>(); + treeArtifacts.injectTo(treeArtifactData::put); + + return ActionExecutionValue.create( + /*artifactData=*/ ImmutableMap.of(), + treeArtifactData, + /*outputSymlinks=*/ null, + /*discoveredModules=*/ null, + /*actionDependsOnBuildId=*/ false); + } + + private static FileArtifactValue createMetadataFromFileSystem(Artifact artifact) + throws IOException { + Path path = artifact.getPath(); + FileArtifactValue noDigest = + ActionMetadataHandler.fileArtifactValueFromArtifact( + artifact, FileStatusWithDigestAdapter.adapt(path.statIfFound(Symlinks.NOFOLLOW)), null); + return FileArtifactValue.createFromInjectedDigest( + noDigest, path.getDigest(), !artifact.isConstantMetadata()); + } + + void writeFile(Path path, String... lines) throws IOException { + // Make sure we advance the clock to detect modifications which do not change the size, which + // rely on ctime. + fs.advanceClockMillis(1); + FileSystemUtils.writeIsoLatin1(path, lines); + } + + static final class MockFileSystem extends InMemoryFileSystem { + boolean statThrowsRuntimeException; + boolean readlinkThrowsIoException; + + MockFileSystem() { + this(new ManualClock()); + } + + private MockFileSystem(ManualClock clock) { + super(clock); + } + + @Override + public FileStatus statIfFound(Path path, boolean followSymlinks) throws IOException { + if (statThrowsRuntimeException) { + throw new RuntimeException("bork"); + } + return super.statIfFound(path, followSymlinks); + } + + @Override + protected PathFragment readSymbolicLink(Path path) throws IOException { + if (readlinkThrowsIoException) { + throw new IOException("readlink failed"); + } + return super.readSymbolicLink(path); + } + + void advanceClockMillis(int millis) { + ((ManualClock) clock).advanceMillis(millis); + } + } +}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java index 6f7a1d4..cdf5f95 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java
@@ -75,6 +75,7 @@ import com.google.devtools.build.lib.analysis.OutputGroupInfo; import com.google.devtools.build.lib.analysis.TopLevelArtifactContext; import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.buildtool.BuildRequestOptions; import com.google.devtools.build.lib.buildtool.SkyframeBuilder; @@ -193,7 +194,10 @@ OptionsParser.builder() .optionsClasses( ImmutableList.of( - KeepGoingOption.class, BuildRequestOptions.class, AnalysisOptions.class)) + KeepGoingOption.class, + BuildRequestOptions.class, + AnalysisOptions.class, + CoreOptions.class)) .build(); options.parse("--jobs=20"); }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java index 30bd726..28c409f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
@@ -58,6 +58,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ServerDirectories; import com.google.devtools.build.lib.analysis.TopLevelArtifactContext; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.buildtool.BuildRequestOptions; import com.google.devtools.build.lib.buildtool.SkyframeBuilder; import com.google.devtools.build.lib.clock.BlazeClock; @@ -151,7 +152,7 @@ public final void initialize() throws Exception { options = OptionsParser.builder() - .optionsClasses(KeepGoingOption.class, BuildRequestOptions.class) + .optionsClasses(KeepGoingOption.class, BuildRequestOptions.class, CoreOptions.class) .build(); options.parse(); inMemoryCache = new InMemoryActionCache();
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java index 3dbc7df..94fa7f5 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java
@@ -30,7 +30,6 @@ import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; -import com.google.devtools.build.lib.actions.Artifact.ArtifactExpanderImpl; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; @@ -3114,6 +3113,15 @@ }; } + private static ArtifactExpander createArtifactExpander( + Artifact directory, ImmutableList<Artifact> files) { + return (artifact, output) -> { + if (artifact.equals(directory)) { + output.addAll(files); + } + }; + } + private String getDigest(CommandLine commandLine) throws CommandLineExpansionException { return getDigest(commandLine, /*artifactExpander=*/ null); } @@ -3157,9 +3165,8 @@ // Now ask for one with an expanded directory Artifact file1 = getBinArtifactWithNoOwner("foo/dir/file1"); Artifact file2 = getBinArtifactWithNoOwner("foo/dir/file2"); - ArtifactExpanderImpl artifactExpander = - new ArtifactExpanderImpl( - ImmutableMap.of(directory, ImmutableList.of(file1, file2)), ImmutableMap.of()); + ArtifactExpander artifactExpander = + createArtifactExpander(directory, ImmutableList.of(file1, file2)); assertThat(commandLine.arguments(artifactExpander)) .containsExactly("foo/dir/file1", "foo/dir/file2"); } @@ -3180,9 +3187,8 @@ Artifact file1 = getBinArtifactWithNoOwner("foo/dir/file1"); Artifact file2 = getBinArtifactWithNoOwner("foo/dir/file2"); - ArtifactExpanderImpl artifactExpander = - new ArtifactExpanderImpl( - ImmutableMap.of(directory, ImmutableList.of(file1, file2)), ImmutableMap.of()); + ArtifactExpander artifactExpander = + createArtifactExpander(directory, ImmutableList.of(file1, file2)); // First expanded, then not expanded (two separate calls) assertThat(commandLine.arguments(artifactExpander)) .containsExactly("foo/dir/file1", "foo/dir/file2", "foo/dir"); @@ -3232,9 +3238,8 @@ Artifact file1 = getBinArtifactWithNoOwner("foo/dir/file1"); Artifact file2 = getBinArtifactWithNoOwner("foo/dir/file2"); - ArtifactExpanderImpl artifactExpander = - new ArtifactExpanderImpl( - ImmutableMap.of(directory, ImmutableList.of(file1, file2)), ImmutableMap.of()); + ArtifactExpander artifactExpander = + createArtifactExpander(directory, ImmutableList.of(file1, file2)); assertThat(commandLine.arguments(artifactExpander)) .containsExactly("foo/dir/file1", "foo/dir/file2", "foo/file3"); }