diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index 932a8bb8b6d8d6..db5e4d7caa158f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -85,6 +85,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:sane_analysis_exception", "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_aware_action", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java index 8bfcd776f30b11..19c214dcf2b52c 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java @@ -33,8 +33,8 @@ public interface MetadataHandler extends MetadataProvider, MetadataInjector { *

Freshly created output files (i.e. from an action that just executed) that require a stat to * obtain the metadata will first be set read-only and executable during this call. This ensures * that the returned metadata has an appropriate ctime, which is affected by chmod. Note that this - * does not apply to outputs injected via {@link #injectFile} or {@link #injectDirectory} since a - * stat is not required for them. + * does not apply to outputs injected via {@link #injectFile} or {@link #injectTree} since a stat + * is not required for them. */ @Override @Nullable @@ -47,7 +47,7 @@ public interface MetadataHandler extends MetadataProvider, MetadataInjector { * Constructs a {@link FileArtifactValue} for the given output whose digest is known. * *

This call does not inject the returned metadata. It should be injected with a followup call - * to {@link #injectFile} or {@link #injectDirectory} as appropriate. + * to {@link #injectFile} or {@link #injectTree} as appropriate. * *

chmod will not be called on the output. */ diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java index ccea7b32fde3d8..a425ee8cefb8ec 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java @@ -14,13 +14,11 @@ package com.google.devtools.build.lib.actions.cache; 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; import com.google.devtools.build.lib.actions.FileArtifactValue; -import java.util.Map; +import com.google.devtools.build.lib.skyframe.TreeArtifactInjector; /** Supports metadata injection of action outputs into skyframe. */ -public interface MetadataInjector { +public interface MetadataInjector extends TreeArtifactInjector { /** * Injects the metadata of a file. @@ -29,20 +27,10 @@ public interface MetadataInjector { * *

{@linkplain Artifact#isTreeArtifact Tree artifacts} and their {@linkplain * Artifact#isChildOfDeclaredDirectory children} must not be passed here. Instead, they should be - * passed to {@link #injectDirectory}. + * passed to {@link #injectTree}. * * @param output a regular output file * @param metadata the file metadata */ void injectFile(Artifact output, FileArtifactValue metadata); - - /** - * Injects the metadata of a tree artifact. - * - *

This can be used to save filesystem operations when the metadata is already known. - * - * @param output an output directory {@linkplain Artifact#isTreeArtifact tree artifact} - * @param children the metadata of the files stored in the directory - */ - void injectDirectory(SpecialArtifact output, Map children); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index 04e0f2e0561fd7..9cf2c6064c9469 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -78,6 +78,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/remote/util", "//src/main/java/com/google/devtools/build/lib/skyframe:mutable_supplier", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", "//src/main/java/com/google/devtools/build/lib/util:exit_code", diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index afc1562fdbfff5..7c5bdfd130bddc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.remote; +import static com.google.common.util.concurrent.Futures.immediateFuture; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; @@ -44,7 +45,6 @@ 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.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.actions.cache.MetadataInjector; @@ -63,6 +63,7 @@ import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution; import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution.Code; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.vfs.Dirent; @@ -98,13 +99,8 @@ interface OutputFilesLocker { void lock() throws InterruptedException, IOException; } - private static final ListenableFuture COMPLETED_SUCCESS = SettableFuture.create(); - private static final ListenableFuture EMPTY_BYTES = SettableFuture.create(); - - static { - ((SettableFuture) COMPLETED_SUCCESS).set(null); - ((SettableFuture) EMPTY_BYTES).set(new byte[0]); - } + private static final ListenableFuture COMPLETED_SUCCESS = immediateFuture(null); + private static final ListenableFuture EMPTY_BYTES = immediateFuture(new byte[0]); protected final RemoteCacheClient cacheProtocol; protected final RemoteOptions options; @@ -440,7 +436,7 @@ private void createSymlinks(Iterable symlinks) throws IOExcepti } } - /** Download a file (that is not a directory). The content is fetched from the digest. */ + /** Downloads a file (that is not a directory). The content is fetched from the digest. */ public ListenableFuture downloadFile(Path path, Digest digest) throws IOException { Preconditions.checkNotNull(path.getParentDirectory()).createDirectoryAndParents(); if (digest.getSizeBytes() == 0) { @@ -621,8 +617,7 @@ private void injectRemoteArtifact( + "--experimental_remote_download_outputs=minimal"); } SpecialArtifact parent = (SpecialArtifact) output; - ImmutableMap.Builder childMetadata = - ImmutableMap.builderWithExpectedSize(directory.files.size()); + TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(parent); for (FileMetadata file : directory.files()) { TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, file.path().relativeTo(parent.getPath())); @@ -632,9 +627,9 @@ private void injectRemoteArtifact( file.digest().getSizeBytes(), /*locationIndex=*/ 1, actionId); - childMetadata.put(child, value); + tree.putChild(child, value); } - metadataInjector.injectDirectory(parent, childMetadata.build()); + metadataInjector.injectTree(parent, tree.build()); } else { FileMetadata outputMetadata = metadata.file(execRoot.getRelative(output.getExecPathString())); if (outputMetadata == null) { 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 50a9ee2ca59827..e021d3bbfcc77b 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 @@ -271,16 +271,15 @@ private static TreeArtifactValue transformSharedTree( } SpecialArtifact newParent = (SpecialArtifact) newArtifact; + TreeArtifactValue.Builder newTree = TreeArtifactValue.newBuilder(newParent); - Map newChildren = - Maps.newHashMapWithExpectedSize(tree.getChildValues().size()); for (Map.Entry child : tree.getChildValues().entrySet()) { - newChildren.put( + newTree.putChild( TreeFileArtifact.createTreeOutput(newParent, child.getKey().getParentRelativePath()), child.getValue()); } - return TreeArtifactValue.create(newChildren); + return newTree.build(); } ActionExecutionValue transformForSharedAction(ImmutableSet outputs) { 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 ef44f38540be1b..3112097c801e81 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 @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Sets; import com.google.common.flogger.GoogleLogger; import com.google.common.io.BaseEncoding; @@ -322,8 +321,7 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa setPathReadOnlyAndExecutable(treeDir); } - ImmutableSortedMap.Builder values = - ImmutableSortedMap.naturalOrder(); + TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(parent); TreeArtifactValue.visitTree( treeDir, @@ -347,10 +345,10 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa throw new IOException(errorMessage, e); } - values.put(child, metadata); + tree.putChild(child, metadata); }); - return TreeArtifactValue.create(values.build()); + return tree.build(); } @Override @@ -379,7 +377,7 @@ public void injectFile(Artifact output, FileArtifactValue metadata) { checkArgument(isKnownOutput(output), "%s is not a declared output of this action", output); checkArgument( !output.isTreeArtifact() && !output.isChildOfDeclaredDirectory(), - "Tree artifacts and their children must be injected via injectDirectory: %s", + "Tree artifacts and their children must be injected via injectTree: %s", output); checkState(executionMode.get(), "Tried to inject %s outside of execution", output); @@ -387,13 +385,12 @@ public void injectFile(Artifact output, FileArtifactValue metadata) { } @Override - public void injectDirectory( - SpecialArtifact output, Map children) { + public void injectTree(SpecialArtifact output, TreeArtifactValue tree) { 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); - store.putTreeArtifactData(output, TreeArtifactValue.create(children)); + store.putTreeArtifactData(output, tree); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java index d4696b98cd022f..98a749f657d647 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java @@ -16,8 +16,6 @@ import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionExecutionException; @@ -27,6 +25,7 @@ import com.google.devtools.build.lib.actions.ActionTemplate; import com.google.devtools.build.lib.actions.Artifact; 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.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -189,11 +188,14 @@ private static TreeArtifactValue createTreeArtifactValueFromActionKey( return null; } - // Aggregate the ArtifactValues for individual TreeFileArtifacts into a TreeArtifactValue for - // the parent TreeArtifact. - ImmutableMap.Builder map = ImmutableMap.builder(); + // Aggregate the metadata for individual TreeFileArtifacts into a TreeArtifactValue for the + // parent TreeArtifact. + SpecialArtifact parent = (SpecialArtifact) artifactDependencies.artifact; + TreeArtifactValue.Builder treeBuilder = TreeArtifactValue.newBuilder(parent); boolean omitted = false; + for (ActionLookupData actionKey : expandedActionExecutionKeys) { + boolean sawTreeChild = false; ActionExecutionValue actionExecutionValue = (ActionExecutionValue) Preconditions.checkNotNull( @@ -202,48 +204,47 @@ private static TreeArtifactValue createTreeArtifactValueFromActionKey( artifactDependencies, expansionValue, expandedActionValueMap); - Iterable treeFileArtifacts = - Iterables.transform( - Iterables.filter( - actionExecutionValue.getAllFileValues().keySet(), - artifact -> { - Preconditions.checkState( - artifact.hasParent(), - "No parent: %s %s %s", - artifact, - actionExecutionValue, - artifactDependencies); - return artifact.getParent().equals(artifactDependencies.artifact); - }), - artifact -> (TreeFileArtifact) artifact); - Preconditions.checkState( - !Iterables.isEmpty(treeFileArtifacts), - "Action denoted by %s does not output TreeFileArtifact from %s", - actionKey, - artifactDependencies); + for (Map.Entry entry : + actionExecutionValue.getAllFileValues().entrySet()) { + Artifact artifact = entry.getKey(); + Preconditions.checkState( + artifact.hasParent(), + "Parentless artifact %s found in ActionExecutionValue for %s: %s %s", + artifact, + actionKey, + actionExecutionValue, + artifactDependencies); - for (TreeFileArtifact treeFileArtifact : treeFileArtifacts) { - FileArtifactValue value = - actionExecutionValue.getExistingFileArtifactValue(treeFileArtifact); - if (FileArtifactValue.OMITTED_FILE_MARKER.equals(value)) { - omitted = true; - } else { - map.put(treeFileArtifact, value); + if (artifact.getParent().equals(parent)) { + sawTreeChild = true; + if (FileArtifactValue.OMITTED_FILE_MARKER.equals(entry.getValue())) { + omitted = true; + } else { + treeBuilder.putChild((TreeFileArtifact) artifact, entry.getValue()); + } } } + + Preconditions.checkState( + sawTreeChild, + "Action denoted by %s does not output any TreeFileArtifacts from %s", + actionKey, + artifactDependencies); } - ImmutableMap children = map.build(); + TreeArtifactValue tree = treeBuilder.build(); if (omitted) { Preconditions.checkState( - children.isEmpty(), + tree.getChildValues().isEmpty(), "Action template expansion has some but not all outputs omitted, present outputs: %s", - children); + artifactDependencies, + tree.getChildValues()); return TreeArtifactValue.OMITTED_TREE_MARKER; } - return TreeArtifactValue.create(children); + + return tree; } private static SkyValue createSourceValue(Artifact artifact, Environment env) 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 09603fa15a685e..b3473b40e8c536 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2674,11 +2674,14 @@ java_library( java_library( name = "tree_artifact_value", - srcs = ["TreeArtifactValue.java"], + srcs = [ + "TreeArtifactInjector.java", + "TreeArtifactValue.java", + ], deps = [ - "//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:has_digest", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//src/main/java/com/google/devtools/build/lib/util", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactInjector.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactInjector.java new file mode 100644 index 00000000000000..22a7df5987134e --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactInjector.java @@ -0,0 +1,31 @@ +// 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.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; + +/** Supports metadata injection of action outputs into skyframe. */ +public interface TreeArtifactInjector { + + /** + * Injects the metadata of a tree artifact. + * + *

This can be used to save filesystem operations when the metadata is already known. + * + * @param output an output directory {@linkplain Artifact#isTreeArtifact tree artifact} + * @param tree a {@link TreeArtifactValue} with the metadata of the files stored in the directory + */ + void injectTree(SpecialArtifact output, TreeArtifactValue tree); +} 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 1f570d41d11a0e..6727ecfd59db31 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 @@ -13,10 +13,11 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.collect.ImmutableSet.toImmutableSet; 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; @@ -25,7 +26,6 @@ import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.HasDigest; import com.google.devtools.build.lib.actions.cache.DigestUtils; -import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.util.Fingerprint; @@ -49,19 +49,36 @@ */ public class TreeArtifactValue implements HasDigest, SkyValue { + /** Returns an empty {@link TreeArtifactValue}. */ + public static TreeArtifactValue empty() { + return EMPTY; + } + + /** + * Returns a new {@link Builder} for the given parent tree artifact. + * + *

The returned builder only supports adding children under this parent. To build multiple tree + * artifacts at once, use {@link MultiBuilder}. + */ + public static Builder newBuilder(SpecialArtifact parent) { + return new Builder(parent); + } + /** Builder for constructing multiple instances of {@link TreeArtifactValue} at once. */ public interface MultiBuilder { /** * Puts a child tree file into this builder under its {@linkplain TreeFileArtifact#getParent * parent}. + * + * @return {@code this} for convenience */ - void putChild(TreeFileArtifact child, FileArtifactValue metadata); + MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata); /** * For each unique parent seen by this builder, passes the aggregated metadata to {@link - * MetadataInjector#injectDirectory}. + * TreeArtifactInjector#injectTree}. */ - void injectTo(MetadataInjector metadataInjector); + void injectTo(TreeArtifactInjector treeInjector); } /** Returns a new {@link MultiBuilder}. */ @@ -85,8 +102,7 @@ public static MultiBuilder newConcurrentMultiBuilder() { private final ImmutableSortedMap childData; private final boolean entirelyRemote; - @AutoCodec.VisibleForSerialization - TreeArtifactValue( + private TreeArtifactValue( byte[] digest, ImmutableSortedMap childData, boolean entirelyRemote) { @@ -95,37 +111,6 @@ public static MultiBuilder newConcurrentMultiBuilder() { this.entirelyRemote = entirelyRemote; } - /** - * Returns a TreeArtifactValue out of the given Artifact-relative path fragments and their - * corresponding FileArtifactValues. - */ - static TreeArtifactValue create(Map childData) { - if (childData.isEmpty()) { - return EMPTY; - } - - // Sort the children to ensure a deterministic TreeArtifactValue (including digest). - ImmutableSortedMap sortedChildData = - ImmutableSortedMap.copyOf(childData); - Fingerprint fingerprint = new Fingerprint(); - boolean entirelyRemote = true; - - for (Map.Entry e : sortedChildData.entrySet()) { - TreeFileArtifact child = e.getKey(); - FileArtifactValue value = e.getValue(); - Preconditions.checkState( - !FileArtifactValue.OMITTED_FILE_MARKER.equals(value), - "Cannot construct TreeArtifactValue because child %s was omitted", - child); - // Tolerate a tree artifact having a mix of local and remote children (b/152496153#comment80). - entirelyRemote &= value.isRemote(); - fingerprint.addPath(child.getParentRelativePath()); - value.addTo(fingerprint); - } - - return new TreeArtifactValue(fingerprint.digestAndReset(), sortedChildData, entirelyRemote); - } - FileArtifactValue getMetadata() { return FileArtifactValue.createProxy(digest); } @@ -136,7 +121,6 @@ ImmutableSet getChildPaths() { .collect(toImmutableSet()); } - @Nullable @Override public byte[] getDigest() { return digest.clone(); @@ -180,7 +164,7 @@ public boolean equals(Object other) { @Override public String toString() { - return MoreObjects.toStringHelper(TreeArtifactValue.class) + return MoreObjects.toStringHelper(this) .add("digest", digest) .add("childData", childData) .toString(); @@ -277,7 +261,7 @@ public interface TreeArtifactVisitor { * artifact directory, or if {@link TreeArtifactVisitor#visit} throws {@link IOException} */ public static void visitTree(Path parentDir, TreeArtifactVisitor visitor) throws IOException { - visitTree(parentDir, PathFragment.EMPTY_FRAGMENT, Preconditions.checkNotNull(visitor)); + visitTree(parentDir, PathFragment.EMPTY_FRAGMENT, checkNotNull(visitor)); } private static void visitTree(Path parentDir, PathFragment subdir, TreeArtifactVisitor visitor) @@ -330,20 +314,82 @@ private static void checkSymlink(PathFragment subDir, Path path) throws IOExcept } } + /** Builder for a {@link TreeArtifactValue}. */ + public static final class Builder { + private final ImmutableSortedMap.Builder childData = + ImmutableSortedMap.naturalOrder(); + private final SpecialArtifact parent; + + Builder(SpecialArtifact parent) { + checkArgument(parent.isTreeArtifact(), "%s is not a tree artifact", parent); + this.parent = parent; + } + + /** + * Adds a child to this builder. + * + *

The child's {@linkplain TreeFileArtifact#getParent parent} must match the parent + * with which this builder was initialized. + * + *

Children may be added in any order. The children are sorted prior to constructing the + * final {@link TreeArtifactValue}. + * + *

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 + */ + public Builder putChild(TreeFileArtifact child, FileArtifactValue metadata) { + checkArgument( + child.getParent().equals(parent), + "While building TreeArtifactValue for %s, got %s with parent %s", + parent, + child, + child.getParent()); + checkArgument( + !FileArtifactValue.OMITTED_FILE_MARKER.equals(metadata), + "Cannot construct TreeArtifactValue for %s because child %s was omitted", + parent, + child); + childData.put(child, metadata); + return this; + } + + /** Builds the final {@link TreeArtifactValue}. */ + public TreeArtifactValue build() { + ImmutableSortedMap finalChildData = childData.build(); + if (finalChildData.isEmpty()) { + return EMPTY; + } + + Fingerprint fingerprint = new Fingerprint(); + boolean entirelyRemote = true; + + for (Map.Entry childData : finalChildData.entrySet()) { + // Digest will be deterministic because children are sorted. + fingerprint.addPath(childData.getKey().getParentRelativePath()); + childData.getValue().addTo(fingerprint); + + // Tolerate a mix of local and remote children(b/152496153#comment80). + entirelyRemote &= childData.getValue().isRemote(); + } + + return new TreeArtifactValue(fingerprint.digestAndReset(), finalChildData, entirelyRemote); + } + } + private static final class BasicMultiBuilder implements MultiBuilder { - private final Map< - SpecialArtifact, ImmutableSortedMap.Builder> - map = new HashMap<>(); + private final Map map = new HashMap<>(); @Override - public void putChild(TreeFileArtifact child, FileArtifactValue metadata) { - map.computeIfAbsent(child.getParent(), parent -> ImmutableSortedMap.naturalOrder()) - .put(child, metadata); + public MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata) { + map.computeIfAbsent(child.getParent(), Builder::new).putChild(child, metadata); + return this; } @Override - public void injectTo(MetadataInjector metadataInjector) { - map.forEach((parent, builder) -> metadataInjector.injectDirectory(parent, builder.build())); + public void injectTo(TreeArtifactInjector treeInjector) { + map.forEach((parent, builder) -> treeInjector.injectTree(parent, builder.build())); } } @@ -353,14 +399,20 @@ private static final class ConcurrentMultiBuilder implements MultiBuilder { map = new ConcurrentHashMap<>(); @Override - public void putChild(TreeFileArtifact child, FileArtifactValue metadata) { + public MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata) { map.computeIfAbsent(child.getParent(), parent -> new ConcurrentHashMap<>()) .put(child, metadata); + return this; } @Override - public void injectTo(MetadataInjector metadataInjector) { - map.forEach(metadataInjector::injectDirectory); + public void injectTo(TreeArtifactInjector treeInjector) { + map.forEach( + (parent, children) -> { + Builder builder = new Builder(parent); + children.forEach(builder::putChild); + treeInjector.injectTree(parent, builder.build()); + }); } } } diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index 44c32c9b108d6a..2fb2df4cd4919e 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java @@ -74,6 +74,7 @@ import com.google.devtools.build.lib.exec.SingleBuildFileCache; import com.google.devtools.build.lib.skyframe.ActionTemplateExpansionValue; import com.google.devtools.build.lib.skyframe.ActionTemplateExpansionValue.ActionTemplateExpansionKey; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.syntax.Location; import com.google.devtools.build.lib.util.FileType; @@ -921,8 +922,7 @@ public void injectFile(Artifact output, FileArtifactValue metadata) { } @Override - public void injectDirectory( - SpecialArtifact treeArtifact, Map children) { + public void injectTree(SpecialArtifact treeArtifact, TreeArtifactValue tree) { throw new UnsupportedOperationException(); } diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/BUILD b/src/test/java/com/google/devtools/build/lib/actions/util/BUILD index b21b068c12aeb7..f888b582d6ac21 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/actions/util/BUILD @@ -32,6 +32,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:single_build_file_cache", "//src/main/java/com/google/devtools/build/lib/skyframe:action_template_expansion_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//src/main/java/com/google/devtools/build/lib/syntax:frontend", diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index d6435178e09ae5..b6d93876ce2542 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -66,6 +66,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/remote/merkletree", "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/remote/util", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util:exit_code", diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java index 48081596da0291..96039baaa95f7c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java @@ -38,7 +38,6 @@ import com.google.common.base.Charsets; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; @@ -50,7 +49,6 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ExecException; -import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; @@ -63,6 +61,7 @@ import com.google.devtools.build.lib.remote.util.InMemoryCacheClient; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.util.io.RecordingOutErr; @@ -993,13 +992,16 @@ public void testDownloadMinimalDirectory() throws Exception { // assert assertThat(inMemoryOutput).isNull(); - Map m = - ImmutableMap.of( - TreeFileArtifact.createTreeOutput(dir, "file1"), - new RemoteFileArtifactValue(toBinaryDigest(d1), d1.getSizeBytes(), 1, "action-id"), - TreeFileArtifact.createTreeOutput(dir, "a/file2"), - new RemoteFileArtifactValue(toBinaryDigest(d2), d2.getSizeBytes(), 1, "action-id")); - verify(injector).injectDirectory(eq(dir), eq(m)); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(dir) + .putChild( + TreeFileArtifact.createTreeOutput(dir, "file1"), + new RemoteFileArtifactValue(toBinaryDigest(d1), d1.getSizeBytes(), 1, "action-id")) + .putChild( + TreeFileArtifact.createTreeOutput(dir, "a/file2"), + new RemoteFileArtifactValue(toBinaryDigest(d2), d2.getSizeBytes(), 1, "action-id")) + .build(); + verify(injector).injectTree(dir, tree); Path outputBase = artifactRoot.getRoot().asPath(); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); 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 687ba3c057fb30..d59daff6232e3c 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 @@ -44,7 +44,6 @@ import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.FileNotFoundException; import java.io.IOException; -import java.util.Map; import java.util.Set; import org.junit.Before; import org.junit.Test; @@ -382,34 +381,31 @@ public void injectRemoteTreeArtifactMetadata() throws Exception { /*outputs=*/ ImmutableSet.of(treeArtifact)); handler.prepareForActionExecution(); - RemoteFileArtifactValue fooValue = - new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1, "foo"); - RemoteFileArtifactValue barValue = - new RemoteFileArtifactValue(new byte[] {4, 5, 6}, 10, 1, "bar"); - Map children = - ImmutableMap.of( - TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), fooValue, - TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), barValue); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(treeArtifact) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), + new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1, "foo")) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), + new RemoteFileArtifactValue(new byte[] {4, 5, 6}, 10, 1, "bar")) + .build(); - handler.injectDirectory(treeArtifact, children); + handler.injectTree(treeArtifact, tree); FileArtifactValue value = handler.getMetadata(treeArtifact); assertThat(value).isNotNull(); - TreeArtifactValue treeValue = handler.getOutputStore().getTreeArtifactData(treeArtifact); - assertThat(treeValue).isNotNull(); - assertThat(treeValue.getDigest()).isEqualTo(value.getDigest()); + assertThat(value.getDigest()).isEqualTo(tree.getDigest()); + assertThat(handler.getOutputStore().getTreeArtifactData(treeArtifact)).isEqualTo(tree); assertThat(chmodCalls).isEmpty(); - assertThat(treeValue.getChildPaths()) - .containsExactly(PathFragment.create("foo"), PathFragment.create("bar")); - assertThat(treeValue.getChildValues().values()).containsExactly(fooValue, barValue); - assertThat(handler.getTreeArtifactChildren(treeArtifact)).isEqualTo(treeValue.getChildren()); + assertThat(handler.getTreeArtifactChildren(treeArtifact)).isEqualTo(tree.getChildren()); // Make sure that all children are transferred properly into the ActionExecutionValue. If any // child is missing, getExistingFileArtifactValue will throw. ActionExecutionValue actionExecutionValue = ActionExecutionValue.createFromOutputStore(handler.getOutputStore(), null, null, false); - treeValue.getChildren().forEach(actionExecutionValue::getExistingFileArtifactValue); + tree.getChildren().forEach(actionExecutionValue::getExistingFileArtifactValue); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java index 32047a4d3913ad..9a63d6cc6a2a9f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java @@ -384,20 +384,17 @@ private SpecialArtifact createAndPopulateTreeArtifact(String path, String... chi throws Exception { SpecialArtifact treeArtifact = createTreeArtifact(path); treeArtifact.setGeneratingActionKey(ActionLookupData.create(CTKEY, /*actionIndex=*/ 0)); - Map treeFileArtifactMap = new LinkedHashMap<>(); + TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(treeArtifact); for (String childRelativePath : childRelativePaths) { TreeFileArtifact treeFileArtifact = TreeFileArtifact.createTreeOutput(treeArtifact, childRelativePath); scratch.file(treeFileArtifact.getPath().toString(), childRelativePath); // We do not care about the FileArtifactValues in this test. - treeFileArtifactMap.put( - treeFileArtifact, FileArtifactValue.createForTesting(treeFileArtifact)); + tree.putChild(treeFileArtifact, FileArtifactValue.createForTesting(treeFileArtifact)); } - artifactValueMap.put( - treeArtifact, TreeArtifactValue.create(ImmutableMap.copyOf(treeFileArtifactMap))); - + artifactValueMap.put(treeArtifact, tree.build()); return treeArtifact; } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java index b4753f483bb485..06c5861c749dab 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java @@ -51,7 +51,6 @@ import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; import com.google.devtools.build.lib.vfs.FileSystem; -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.Root; @@ -324,10 +323,10 @@ public void actionExecutionValueSerialization() throws Exception { Path path = treeFileArtifact.getPath(); path.getParentDirectory().createDirectoryAndParents(); writeFile(path, "contents"); - TreeArtifactValue treeArtifactValue = - TreeArtifactValue.create( - ImmutableMap.of( - treeFileArtifact, FileArtifactValue.createForTesting(treeFileArtifact))); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(treeArtifact) + .putChild(treeFileArtifact, FileArtifactValue.createForTesting(treeFileArtifact)) + .build(); DerivedArtifact artifact3 = createDerivedArtifact("three"); FilesetOutputSymlink filesetOutputSymlink = FilesetOutputSymlink.createForTesting( @@ -335,7 +334,7 @@ public void actionExecutionValueSerialization() throws Exception { ActionExecutionValue actionExecutionValue = ActionExecutionValue.create( ImmutableMap.of(artifact1, metadata1, artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN), - ImmutableMap.of(treeArtifact, treeArtifactValue), + ImmutableMap.of(treeArtifact, tree), ImmutableList.of(filesetOutputSymlink), null, true); @@ -349,7 +348,7 @@ public void actionExecutionValueSerialization() throws Exception { } private static void file(Path path, String contents) throws Exception { - FileSystemUtils.createDirectoryAndParents(path.getParentDirectory()); + path.getParentDirectory().createDirectoryAndParents(); writeFile(path, contents); } @@ -493,16 +492,19 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept Preconditions.checkState(!output.isTreeArtifact(), "Cannot omit %s", output); artifactData.put(output, FileArtifactValue.OMITTED_FILE_MARKER); } else if (output.isTreeArtifact()) { + SpecialArtifact parent = (SpecialArtifact) output; TreeFileArtifact treeFileArtifact1 = TreeFileArtifact.createTreeOutput((SpecialArtifact) output, "child1"); TreeFileArtifact treeFileArtifact2 = TreeFileArtifact.createTreeOutput((SpecialArtifact) output, "child2"); - TreeArtifactValue treeArtifactValue = - TreeArtifactValue.create( - ImmutableMap.of( - treeFileArtifact1, FileArtifactValue.createForTesting(treeFileArtifact1), - treeFileArtifact2, FileArtifactValue.createForTesting(treeFileArtifact2))); - treeArtifactData.put(output, treeArtifactValue); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(parent) + .putChild( + treeFileArtifact1, FileArtifactValue.createForTesting(treeFileArtifact1)) + .putChild( + treeFileArtifact2, FileArtifactValue.createForTesting(treeFileArtifact2)) + .build(); + treeArtifactData.put(output, tree); } else if (action.getActionType() == MiddlemanType.NORMAL) { Path path = output.getPath(); FileArtifactValue noDigest = 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 75e5f174a35a2f..ef0009ca2f8dfa 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 @@ -33,7 +33,6 @@ import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; -import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.actions.util.TestAction; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -84,7 +83,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -531,11 +529,11 @@ private void checkDirtyTreeArtifactActions(BatchStat batchStatter) throws Except actionKey2, actionValueWithTreeArtifacts(ImmutableList.of(file21, file22)), actionKeyEmpty, - actionValueWithEmptyDirectory(outEmpty), + actionValueWithTreeArtifact(outEmpty, TreeArtifactValue.empty()), actionKeyUnchanging, - actionValueWithEmptyDirectory(outUnchanging), + actionValueWithTreeArtifact(outUnchanging, TreeArtifactValue.empty()), actionKeyLast, - actionValueWithEmptyDirectory(last))); + actionValueWithTreeArtifact(last, TreeArtifactValue.empty()))); EvaluationContext evaluationContext = EvaluationContext.newBuilder() @@ -861,15 +859,6 @@ private static ActionExecutionValue actionValue(Action action) { /*actionDependsOnBuildId=*/ false); } - private static ActionExecutionValue actionValueWithEmptyDirectory(SpecialArtifact emptyDir) { - return ActionExecutionValue.create( - /*artifactData=*/ ImmutableMap.of(), - ImmutableMap.of(emptyDir, TreeArtifactValue.create(ImmutableMap.of())), - /*outputSymlinks=*/ null, - /*discoveredModules=*/ null, - /*actionDependsOnBuildId=*/ false); - } - private static ActionExecutionValue actionValueWithTreeArtifacts( List contents) { TreeArtifactValue.MultiBuilder treeArtifacts = TreeArtifactValue.newMultiBuilder(); @@ -892,17 +881,7 @@ private static ActionExecutionValue actionValueWithTreeArtifacts( } Map treeArtifactData = new HashMap<>(); - treeArtifacts.injectTo( - new MetadataInjector() { - @Override - public void injectFile(Artifact output, FileArtifactValue metadata) {} - - @Override - public void injectDirectory( - SpecialArtifact output, Map children) { - treeArtifactData.put(output, TreeArtifactValue.create(children)); - } - }); + treeArtifacts.injectTo(treeArtifactData::put); return ActionExecutionValue.create( /*artifactData=*/ ImmutableMap.of(), @@ -912,31 +891,24 @@ public void injectDirectory( /*actionDependsOnBuildId=*/ false); } - private static ActionExecutionValue actionValueWithRemoteTreeArtifact( - SpecialArtifact output, Map children) { - ImmutableMap.Builder childFileValues = - ImmutableMap.builder(); - for (Map.Entry child : children.entrySet()) { - childFileValues.put( - TreeFileArtifact.createTreeOutput(output, child.getKey()), child.getValue()); - } - TreeArtifactValue treeArtifactValue = TreeArtifactValue.create(childFileValues.build()); + private static ActionExecutionValue actionValueWithTreeArtifact( + SpecialArtifact output, TreeArtifactValue tree) { return ActionExecutionValue.create( ImmutableMap.of(), - Collections.singletonMap(output, treeArtifactValue), - /* outputSymlinks= */ null, - /* discoveredModules= */ null, - /* actionDependsOnBuildId= */ false); + ImmutableMap.of(output, tree), + /*outputSymlinks=*/ null, + /*discoveredModules=*/ null, + /*actionDependsOnBuildId=*/ false); } private static ActionExecutionValue actionValueWithRemoteArtifact( Artifact output, RemoteFileArtifactValue value) { return ActionExecutionValue.create( - Collections.singletonMap(output, value), + ImmutableMap.of(output, value), ImmutableMap.of(), - /* outputSymlinks= */ null, - /* discoveredModules= */ null, - /* actionDependsOnBuildId= */ false); + /*outputSymlinks=*/ null, + /*discoveredModules=*/ null, + /*actionDependsOnBuildId=*/ false); } private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents) { @@ -1008,16 +980,17 @@ public void testRemoteAndLocalTreeArtifacts() throws Exception { SpecialArtifact treeArtifact = createTreeArtifact("dir"); treeArtifact.getPath().createDirectoryAndParents(); - Map treeArtifactMetadata = new HashMap<>(); - treeArtifactMetadata.put( - PathFragment.create("foo"), createRemoteFileArtifactValue("foo-content")); - treeArtifactMetadata.put( - PathFragment.create("bar"), createRemoteFileArtifactValue("bar-content")); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(treeArtifact) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), + createRemoteFileArtifactValue("foo-content")) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), + createRemoteFileArtifactValue("bar-content")) + .build(); - Map metadataToInject = new HashMap<>(); - metadataToInject.put( - actionKey, actionValueWithRemoteTreeArtifact(treeArtifact, treeArtifactMetadata)); - differencer.inject(metadataToInject); + differencer.inject(ImmutableMap.of(actionKey, actionValueWithTreeArtifact(treeArtifact, tree))); EvaluationContext evaluationContext = EvaluationContext.newBuilder() diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java index 4b03f6bf4fadce..82cb374619c466 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java @@ -1008,18 +1008,16 @@ public void symlinkChainError() throws Exception { assertThat(error.getException()).hasMessageThat().contains("Symlink cycle"); } - private static class NonHermeticArtifactFakeFunction implements SkyFunction { + private static final class NonHermeticArtifactFakeFunction implements SkyFunction { - private final Map allTreeFiles = new HashMap<>(); + private TreeArtifactValue.Builder tree; - @Nullable @Override - public SkyValue compute(SkyKey skyKey, Environment env) - throws SkyFunctionException, InterruptedException { + public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException { try { if (skyKey.argument() instanceof Artifact && ((Artifact) skyKey.argument()).isTreeArtifact()) { - return TreeArtifactValue.create(allTreeFiles); + return tree.build(); } return FileArtifactValue.createForTesting(((Artifact) skyKey.argument()).getPath()); } catch (IOException e) { @@ -1033,16 +1031,17 @@ public String extractTag(SkyKey skyKey) { return null; } - public void addNewTreeFileArtifact(TreeFileArtifact input) throws IOException { - allTreeFiles.put(input, FileArtifactValue.createForTesting(input.getPath())); + void addNewTreeFileArtifact(TreeFileArtifact input) throws IOException { + if (tree == null) { + tree = TreeArtifactValue.newBuilder(input.getParent()); + } + tree.putChild(input, FileArtifactValue.createForTesting(input.getPath())); } } - private static class ArtifactFakeFunction implements SkyFunction { - @Nullable + private static final class ArtifactFakeFunction implements SkyFunction { @Override - public SkyValue compute(SkyKey skyKey, Environment env) - throws SkyFunctionException, InterruptedException { + public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { return env.getValue(new NonHermeticArtifactSkyKey(skyKey)); } @@ -1053,11 +1052,10 @@ public String extractTag(SkyKey skyKey) { } } - private class ActionFakeFunction implements SkyFunction { + private final class ActionFakeFunction implements SkyFunction { @Nullable @Override - public SkyValue compute(SkyKey skyKey, Environment env) - throws SkyFunctionException, InterruptedException { + public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { return env.getValue( new NonHermeticArtifactSkyKey( Preconditions.checkNotNull( diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java index 747726fa2b1b15..5bc62eef6d2bc9 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java @@ -21,7 +21,6 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.hash.Hashing; import com.google.devtools.build.lib.actions.Action; @@ -627,7 +626,12 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException { actionExecutionContext .getMetadataHandler() - .injectDirectory(out, ImmutableMap.of(child1, remoteFile1, child2, remoteFile2)); + .injectTree( + out, + TreeArtifactValue.newBuilder(out) + .putChild(child1, remoteFile1) + .putChild(child2, remoteFile2) + .build()); } }; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java index b8d1bc272725ee..4a6ccf27d8a19f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java @@ -57,9 +57,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.Comparator; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.Random; import java.util.stream.Collectors; import java.util.stream.IntStream; @@ -263,12 +261,12 @@ private class TreeArtifactExecutionFunction implements SkyFunction { @Override public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException, InterruptedException { - Map treeArtifactData = new HashMap<>(); ActionLookupData actionLookupData = (ActionLookupData) skyKey.argument(); ActionLookupValue actionLookupValue = (ActionLookupValue) env.getValue(actionLookupData.getActionLookupKey()); Action action = actionLookupValue.getAction(actionLookupData.getActionIndex()); SpecialArtifact output = (SpecialArtifact) Iterables.getOnlyElement(action.getOutputs()); + TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(output); for (PathFragment subpath : testTreeArtifactContents) { try { TreeFileArtifact suboutput = TreeFileArtifact.createTreeOutput(output, subpath); @@ -281,17 +279,15 @@ public SkyValue compute(SkyKey skyKey, Environment env) FileArtifactValue withDigest = FileArtifactValue.createFromInjectedDigest( noDigest, path.getDigest(), !output.isConstantMetadata()); - treeArtifactData.put(suboutput, withDigest); + tree.putChild(suboutput, withDigest); } catch (IOException e) { throw new SkyFunctionException(e, Transience.TRANSIENT) {}; } } - TreeArtifactValue treeArtifactValue = TreeArtifactValue.create(treeArtifactData); - return ActionExecutionValue.create( /*artifactData=*/ ImmutableMap.of(), - ImmutableMap.of(output, treeArtifactValue), + ImmutableMap.of(output, tree.build()), /*outputSymlinks=*/ null, /*discoveredModules=*/ null, /*actionDependsOnBuildId=*/ false); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java index fb47a56050a437..5a21c1e9768a23 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java @@ -20,14 +20,12 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -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.SpecialArtifactType; 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.FileArtifactValue.RemoteFileArtifactValue; -import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.util.Pair; @@ -55,6 +53,86 @@ public final class TreeArtifactValueTest { private final Scratch scratch = new Scratch(); private final ArtifactRoot root = ArtifactRoot.asDerivedRoot(scratch.resolve("root"), "bin"); + @Test + public void builderCreatesCorrectValue() { + SpecialArtifact parent = createTreeArtifact("bin/tree"); + TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(parent, "child1"); + TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(parent, "child2"); + FileArtifactValue metadata1 = metadataWithId(1); + FileArtifactValue metadata2 = metadataWithId(2); + + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(parent) + .putChild(child1, metadata1) + .putChild(child2, metadata2) + .build(); + + assertThat(tree.getChildren()).containsExactly(child1, child2); + assertThat(tree.getChildValues()).containsExactly(child1, metadata1, child2, metadata2); + assertThat(tree.getChildPaths()) + .containsExactly(child1.getParentRelativePath(), child2.getParentRelativePath()); + assertThat(tree.getDigest()).isNotNull(); + assertThat(tree.getMetadata().getDigest()).isEqualTo(tree.getDigest()); + } + + @Test + public void empty() { + TreeArtifactValue emptyTree = TreeArtifactValue.empty(); + + assertThat(emptyTree.getChildren()).isEmpty(); + assertThat(emptyTree.getChildValues()).isEmpty(); + assertThat(emptyTree.getChildPaths()).isEmpty(); + assertThat(emptyTree.getDigest()).isNotNull(); + assertThat(emptyTree.getMetadata().getDigest()).isEqualTo(emptyTree.getDigest()); + } + + @Test + public void canonicalEmptyInstance() { + SpecialArtifact parent = createTreeArtifact("bin/tree"); + + TreeArtifactValue emptyTreeFromBuilder = TreeArtifactValue.newBuilder(parent).build(); + + assertThat(emptyTreeFromBuilder).isSameInstanceAs(TreeArtifactValue.empty()); + } + + @Test + public void cannotCreateBuilderForNonTreeArtifact() { + SpecialArtifact notTreeArtifact = + new SpecialArtifact( + root, + PathFragment.create("bin/not_tree"), + ActionsTestUtil.NULL_ARTIFACT_OWNER, + SpecialArtifactType.FILESET); + + assertThrows( + IllegalArgumentException.class, () -> TreeArtifactValue.newBuilder(notTreeArtifact)); + } + + @Test + public void cannotMixParentsWithinSingleBuilder() { + SpecialArtifact parent = createTreeArtifact("bin/tree"); + TreeFileArtifact childOfAnotherParent = + TreeFileArtifact.createTreeOutput(createTreeArtifact("bin/other_tree"), "child"); + + TreeArtifactValue.Builder builderForParent = TreeArtifactValue.newBuilder(parent); + + assertThrows( + IllegalArgumentException.class, + () -> builderForParent.putChild(childOfAnotherParent, metadataWithId(1))); + } + + @Test + public void cannotAddOmittedChildToBuilder() { + SpecialArtifact parent = createTreeArtifact("bin/tree"); + TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, "child"); + + TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(parent); + + assertThrows( + IllegalArgumentException.class, + () -> builder.putChild(child, FileArtifactValue.OMITTED_FILE_MARKER)); + } + @Test public void orderIndependence() { SpecialArtifact parent = createTreeArtifact("bin/tree"); @@ -64,9 +142,15 @@ public void orderIndependence() { FileArtifactValue metadata2 = metadataWithId(2); TreeArtifactValue tree1 = - TreeArtifactValue.create(ImmutableMap.of(child1, metadata1, child2, metadata2)); + TreeArtifactValue.newBuilder(parent) + .putChild(child1, metadata1) + .putChild(child2, metadata2) + .build(); TreeArtifactValue tree2 = - TreeArtifactValue.create(ImmutableMap.of(child2, metadata2, child1, metadata1)); + TreeArtifactValue.newBuilder(parent) + .putChild(child2, metadata2) + .putChild(child1, metadata1) + .build(); assertThat(tree1).isEqualTo(tree2); } @@ -77,8 +161,10 @@ public void nullDigests_equal() { TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, "child"); FileArtifactValue metadataNoDigest = metadataWithIdNoDigest(1); - TreeArtifactValue tree1 = TreeArtifactValue.create(ImmutableMap.of(child, metadataNoDigest)); - TreeArtifactValue tree2 = TreeArtifactValue.create(ImmutableMap.of(child, metadataNoDigest)); + TreeArtifactValue tree1 = + TreeArtifactValue.newBuilder(parent).putChild(child, metadataNoDigest).build(); + TreeArtifactValue tree2 = + TreeArtifactValue.newBuilder(parent).putChild(child, metadataNoDigest).build(); assertThat(metadataNoDigest.getDigest()).isNull(); assertThat(tree1.getDigest()).isNotNull(); @@ -93,8 +179,10 @@ public void nullDigests_notEqual() { FileArtifactValue metadataNoDigest1 = metadataWithIdNoDigest(1); FileArtifactValue metadataNoDigest2 = metadataWithIdNoDigest(2); - TreeArtifactValue tree1 = TreeArtifactValue.create(ImmutableMap.of(child, metadataNoDigest1)); - TreeArtifactValue tree2 = TreeArtifactValue.create(ImmutableMap.of(child, metadataNoDigest2)); + TreeArtifactValue tree1 = + TreeArtifactValue.newBuilder(parent).putChild(child, metadataNoDigest1).build(); + TreeArtifactValue tree2 = + TreeArtifactValue.newBuilder(parent).putChild(child, metadataNoDigest2).build(); assertThat(metadataNoDigest1.getDigest()).isNull(); assertThat(metadataNoDigest2.getDigest()).isNull(); @@ -256,7 +344,7 @@ TreeArtifactValue.MultiBuilder newMultiBuilder() { } @Parameter public MultiBuilderType multiBuilderType; - private final FakeMetadataInjector metadataInjector = new FakeMetadataInjector(); + private final Map results = new HashMap<>(); @Parameters(name = "{0}") public static MultiBuilderType[] params() { @@ -267,9 +355,9 @@ public static MultiBuilderType[] params() { public void empty() { TreeArtifactValue.MultiBuilder treeArtifacts = multiBuilderType.newMultiBuilder(); - treeArtifacts.injectTo(metadataInjector); + treeArtifacts.injectTo(results::put); - assertThat(metadataInjector.injectedTreeArtifacts).isEmpty(); + assertThat(results).isEmpty(); } @Test @@ -279,15 +367,18 @@ public void singleTreeArtifact() { TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(parent, "child1"); TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(parent, "child2"); - treeArtifacts.putChild(child1, metadataWithId(1)); - treeArtifacts.putChild(child2, metadataWithId(2)); - treeArtifacts.injectTo(metadataInjector); + treeArtifacts + .putChild(child1, metadataWithId(1)) + .putChild(child2, metadataWithId(2)) + .injectTo(results::put); - assertThat(metadataInjector.injectedTreeArtifacts) + assertThat(results) .containsExactly( parent, - TreeArtifactValue.create( - ImmutableMap.of(child1, metadataWithId(1), child2, metadataWithId(2)))); + TreeArtifactValue.newBuilder(parent) + .putChild(child1, metadataWithId(1)) + .putChild(child2, metadataWithId(2)) + .build()); } @Test @@ -299,39 +390,28 @@ public void multipleTreeArtifacts() { SpecialArtifact parent2 = createTreeArtifact("bin/tree2"); TreeFileArtifact parent2Child = TreeFileArtifact.createTreeOutput(parent2, "child"); - treeArtifacts.putChild(parent1Child1, metadataWithId(1)); - treeArtifacts.putChild(parent2Child, metadataWithId(3)); - treeArtifacts.putChild(parent1Child2, metadataWithId(2)); - treeArtifacts.injectTo(metadataInjector); + treeArtifacts + .putChild(parent1Child1, metadataWithId(1)) + .putChild(parent2Child, metadataWithId(3)) + .putChild(parent1Child2, metadataWithId(2)) + .injectTo(results::put); - assertThat(metadataInjector.injectedTreeArtifacts) + assertThat(results) .containsExactly( parent1, - TreeArtifactValue.create( - ImmutableMap.of( - parent1Child1, metadataWithId(1), parent1Child2, metadataWithId(2))), + TreeArtifactValue.newBuilder(parent1) + .putChild(parent1Child1, metadataWithId(1)) + .putChild(parent1Child2, metadataWithId(2)) + .build(), parent2, - TreeArtifactValue.create(ImmutableMap.of(parent2Child, metadataWithId(3)))); + TreeArtifactValue.newBuilder(parent2) + .putChild(parent2Child, metadataWithId(3)) + .build()); } private static SpecialArtifact createTreeArtifact(String execPath) { return TreeArtifactValueTest.createTreeArtifact(execPath, ROOT); } - - private static final class FakeMetadataInjector implements MetadataInjector { - private final Map injectedTreeArtifacts = new HashMap<>(); - - @Override - public void injectFile(Artifact output, FileArtifactValue metadata) { - throw new UnsupportedOperationException(); - } - - @Override - public void injectDirectory( - SpecialArtifact output, Map children) { - injectedTreeArtifacts.put(output, TreeArtifactValue.create(children)); - } - } } private SpecialArtifact createTreeArtifact(String execPath) {