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 43c2b84104becd..5e24716111f09d 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 @@ -13,12 +13,15 @@ // 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.base.Preconditions.checkState; + import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; -import com.google.common.base.Objects; -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.common.collect.Maps; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.Artifact; @@ -34,80 +37,104 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.rules.cpp.IncludeScannable; import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation; +import com.google.devtools.build.lib.util.ClassName; +import com.google.devtools.build.lib.util.HashCodes; import com.google.devtools.build.skyframe.SkyValue; import com.google.errorprone.annotations.FormatMethod; import com.google.errorprone.annotations.FormatString; import java.util.Map; import java.util.NoSuchElementException; +import java.util.Objects; import java.util.function.BiFunction; import javax.annotation.Nullable; /** A value representing an executed action. */ @Immutable @ThreadSafe -public final class ActionExecutionValue implements SkyValue { - - /** A map from each output artifact of this action to their {@link FileArtifactValue}s. */ - private final ImmutableMap artifactData; - - /** The TreeArtifactValue of all TreeArtifacts output by this Action. */ - private final ImmutableMap treeArtifactData; +public abstract class ActionExecutionValue implements SkyValue { - @Nullable private final ImmutableList outputSymlinks; + private ActionExecutionValue() {} - @Nullable private final NestedSet discoveredModules; - - /** - * @param artifactData Map from Artifacts to corresponding {@link FileArtifactValue}. - * @param treeArtifactData All tree artifact data. - * @param outputSymlinks This represents the SymlinkTree which is the output of a fileset action. - * @param discoveredModules cpp modules discovered - */ - private ActionExecutionValue( + @VisibleForTesting + public static ActionExecutionValue create( ImmutableMap artifactData, ImmutableMap treeArtifactData, @Nullable ImmutableList outputSymlinks, @Nullable NestedSet discoveredModules) { - for (Map.Entry entry : artifactData.entrySet()) { - Preconditions.checkArgument( - !entry.getKey().isChildOfDeclaredDirectory(), - "%s should only be stored in a TreeArtifactValue", - entry.getKey()); - if (entry.getValue().getType().isFile()) { - Preconditions.checkNotNull( - entry.getValue().getDigest(), "missing digest for %s", entry.getKey()); - } + // Use forEach instead of entrySet to avoid instantiating an EntrySet in ImmutableMap. + artifactData.forEach( + (artifact, value) -> { + checkArgument( + !artifact.isChildOfDeclaredDirectory(), + "%s should only be stored in a TreeArtifactValue", + artifact); + checkArgument( + !value.getType().isFile() || value.getDigest() != null, + "Missing digest for %s", + artifact); + }); + treeArtifactData.forEach( + (tree, treeValue) -> { + if (TreeArtifactValue.OMITTED_TREE_MARKER.equals(treeValue)) { + return; + } + treeValue + .getChildValues() + .forEach( + (child, childValue) -> + // Ignore symlinks to directories, which don't have a digest. + checkArgument( + !childValue.getType().isFile() || childValue.getDigest() != null, + "Missing digest for file %s in tree artifact %s", + child, + tree)); + }); + + if (outputSymlinks != null) { + checkArgument( + artifactData.size() == 1, + "Fileset actions should have a single output file (the manifest): %s", + artifactData); + checkArgument( + treeArtifactData.isEmpty(), + "Fileset actions do not output tree artifacts: %s", + treeArtifactData); + checkArgument( + discoveredModules == null, + "Fileset actions do not discover modules: %s", + discoveredModules); + return new Fileset(artifactData, outputSymlinks); } - for (Map.Entry tree : treeArtifactData.entrySet()) { - TreeArtifactValue treeArtifact = tree.getValue(); - if (TreeArtifactValue.OMITTED_TREE_MARKER.equals(treeArtifact)) { - continue; - } - for (Map.Entry file : - treeArtifact.getChildValues().entrySet()) { - // Tree artifacts can contain symlinks to directories, which don't have a digest. - if (file.getValue().getType().isFile()) { - Preconditions.checkNotNull( - file.getValue().getDigest(), - "missing digest for file %s in tree artifact %s", - file.getKey(), - tree.getKey()); - } - } + if (discoveredModules != null) { + checkArgument( + artifactData.size() == 1, + "Module-discovering actions should have a single output file (the .pcm file): %s", + artifactData); + checkArgument( + treeArtifactData.isEmpty(), + "Module-discovering actions do not output tree artifacts: %s", + treeArtifactData); + return new ModuleDiscovering(artifactData, discoveredModules); } - this.artifactData = artifactData; - this.treeArtifactData = treeArtifactData; - this.outputSymlinks = outputSymlinks; - this.discoveredModules = discoveredModules; + if (!treeArtifactData.isEmpty()) { + return treeArtifactData.size() == 1 && artifactData.isEmpty() + ? new SingleTree(treeArtifactData) + : new MultiTree(artifactData, treeArtifactData); + } + + checkArgument(!artifactData.isEmpty(), "No outputs"); + return artifactData.size() == 1 + ? new SingleOutputFile(artifactData) + : new MultiOutputFile(artifactData); } static ActionExecutionValue createFromOutputStore( OutputStore outputStore, @Nullable ImmutableList outputSymlinks, Action action) { - return new ActionExecutionValue( + return create( outputStore.getAllArtifactData(), outputStore.getAllTreeArtifactData(), outputSymlinks, @@ -121,18 +148,7 @@ public static ActionExecutionValue createForTesting( ImmutableMap artifactData, ImmutableMap treeArtifactData, @Nullable ImmutableList outputSymlinks) { - return new ActionExecutionValue( - artifactData, treeArtifactData, outputSymlinks, /* discoveredModules= */ null); - } - - @VisibleForTesting - public static ActionExecutionValue createForTesting( - ImmutableMap artifactData, - ImmutableMap treeArtifactData, - @Nullable ImmutableList outputSymlinks, - NestedSet discoveredModules) { - return new ActionExecutionValue( - artifactData, treeArtifactData, outputSymlinks, discoveredModules); + return create(artifactData, treeArtifactData, outputSymlinks, /* discoveredModules= */ null); } /** @@ -142,7 +158,7 @@ public static ActionExecutionValue createForTesting( * be thrown. */ public FileArtifactValue getExistingFileArtifactValue(Artifact artifact) { - Preconditions.checkArgument( + checkArgument( artifact instanceof DerivedArtifact && !artifact.isTreeArtifact(), "Cannot request %s from %s", artifact, @@ -150,24 +166,24 @@ public FileArtifactValue getExistingFileArtifactValue(Artifact artifact) { FileArtifactValue result; if (artifact.isChildOfDeclaredDirectory()) { - TreeArtifactValue tree = treeArtifactData.get(artifact.getParent()); + TreeArtifactValue tree = getTreeArtifactValue(artifact.getParent()); result = tree == null ? null : tree.getChildValues().get(artifact); } else if (artifact instanceof ArchivedTreeArtifact) { - TreeArtifactValue tree = treeArtifactData.get(artifact.getParent()); + TreeArtifactValue tree = getTreeArtifactValue(artifact.getParent()); ArchivedRepresentation archivedRepresentation = tree.getArchivedRepresentation() .orElseThrow( () -> new NoSuchElementException("Missing archived representation in: " + tree)); - Preconditions.checkArgument( + checkArgument( archivedRepresentation.archivedTreeFileArtifact().equals(artifact), "Multiple archived tree artifacts for: %s", artifact.getParent()); result = archivedRepresentation.archivedFileValue(); } else { - result = artifactData.get(artifact); + result = getAllFileValues().get(artifact); } - return Preconditions.checkNotNull( + return checkNotNull( result, "Missing artifact %s (generating action key %s) in %s", artifact, @@ -175,53 +191,43 @@ public FileArtifactValue getExistingFileArtifactValue(Artifact artifact) { this); } + @Nullable TreeArtifactValue getTreeArtifactValue(Artifact artifact) { - Preconditions.checkArgument(artifact.isTreeArtifact()); - return treeArtifactData.get(artifact); + checkArgument(artifact.isTreeArtifact(), artifact); + return null; } /** * Returns a map containing all artifacts output by the action, except for tree artifacts which - * are accesible via {@link #getAllTreeArtifactValues}. - * - *

Primarily needed by {@link FilesystemValueChecker}. Also called by {@link ArtifactFunction} - * when aggregating a {@link TreeArtifactValue} out of action template expansion outputs. + * are accessible via {@link #getAllTreeArtifactValues}. */ - // Visible only for testing: should be package-private. - public ImmutableMap getAllFileValues() { - return artifactData; - } + public abstract ImmutableMap getAllFileValues(); - /** - * Returns a map containing all tree artifacts output by the action. - * - *

Should only be needed by {@link FilesystemValueChecker}. - */ - // Visible only for testing: should be package-private. + /** Returns a map containing all tree artifacts output by the action. */ public ImmutableMap getAllTreeArtifactValues() { - return treeArtifactData; + return ImmutableMap.of(); } @Nullable public ImmutableList getOutputSymlinks() { - return outputSymlinks; + return null; } @Nullable public NestedSet getDiscoveredModules() { - return discoveredModules; + return null; } @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("artifactData", artifactData) - .add("treeArtifactData", treeArtifactData) + public final String toString() { + return MoreObjects.toStringHelper(ClassName.getSimpleNameWithOuter(getClass())) + .add("files", getAllFileValues()) + .add("trees", getAllTreeArtifactValues()) .toString(); } @Override - public boolean equals(Object obj) { + public final boolean equals(Object obj) { if (this == obj) { return true; } @@ -229,12 +235,12 @@ public boolean equals(Object obj) { return false; } ActionExecutionValue o = (ActionExecutionValue) obj; - return artifactData.equals(o.artifactData) - && treeArtifactData.equals(o.treeArtifactData) - && Objects.equal(outputSymlinks, o.outputSymlinks) + return getAllFileValues().equals(o.getAllFileValues()) + && getAllTreeArtifactValues().equals(o.getAllTreeArtifactValues()) + && Objects.equals(getOutputSymlinks(), o.getOutputSymlinks()) // We use shallowEquals to avoid materializing the nested sets just for change-pruning. This // makes change-pruning potentially less effective, but never incorrect. - && shallowEquals(discoveredModules, o.discoveredModules); + && shallowEquals(getDiscoveredModules(), o.getDiscoveredModules()); } private static boolean shallowEquals( @@ -242,14 +248,18 @@ private static boolean shallowEquals( if (set1 == null) { return set2 == null; } - return set1.shallowEquals(set2); } @Override - public int hashCode() { - return 31 * Objects.hashCode(artifactData, treeArtifactData, outputSymlinks) - + (discoveredModules != null ? discoveredModules.shallowHashCode() : 0); + public final int hashCode() { + int result = + HashCodes.hashObjects(getAllFileValues(), getAllTreeArtifactValues(), getOutputSymlinks()); + NestedSet discoveredModules = getDiscoveredModules(); + if (discoveredModules != null) { + result = 31 * result + discoveredModules.shallowHashCode(); + } + return result; } private static ImmutableMap transformMap( @@ -278,8 +288,7 @@ private static ImmutableMap transformMap( /** Transforms the children of a {@link TreeArtifactValue} so that owners are consistent. */ private static TreeArtifactValue transformSharedTree( Artifact newArtifact, TreeArtifactValue tree) { - Preconditions.checkState( - newArtifact.isTreeArtifact(), "Expected tree artifact, got %s", newArtifact); + checkState(newArtifact.isTreeArtifact(), "Expected tree artifact, got %s", newArtifact); if (TreeArtifactValue.OMITTED_TREE_MARKER.equals(tree)) { return TreeArtifactValue.OMITTED_TREE_MARKER; @@ -312,20 +321,22 @@ private static TreeArtifactValue transformSharedTree( * com.google.devtools.build.lib.actions.Actions#canBeShared shareable} with the action that * originally produced this {@code ActionExecutionValue}. */ - public ActionExecutionValue transformForSharedAction(Action action) + public final ActionExecutionValue transformForSharedAction(Action action) throws ActionTransformException { + ImmutableMap artifactData = getAllFileValues(); + ImmutableMap treeArtifactData = getAllTreeArtifactValues(); if (action.getOutputs().size() != artifactData.size() + treeArtifactData.size()) { throw new ActionTransformException("Cannot share %s with %s", this, action); } - Map newArtifactMap = + ImmutableMap newArtifactMap = Maps.uniqueIndex(action.getOutputs(), OwnerlessArtifactWrapper::new); - return new ActionExecutionValue( + return create( transformMap(artifactData, newArtifactMap, action, (newArtifact, value) -> value), transformMap( treeArtifactData, newArtifactMap, action, ActionExecutionValue::transformSharedTree), - outputSymlinks, + getOutputSymlinks(), // Discovered modules come from the action's inputs, and so don't need to be transformed. - discoveredModules); + getDiscoveredModules()); } /** @@ -338,4 +349,144 @@ private ActionTransformException(@FormatString String format, Object... args) { super(String.format(format, args)); } } + + /** + * The result of an action that outputs a single file (the common case). Optimizes for space by + * storing the single artifact and value without the {@link ImmutableMap} wrapper. + */ + private static class SingleOutputFile extends ActionExecutionValue { + private final Artifact artifact; + private final FileArtifactValue value; + + SingleOutputFile(ImmutableMap artifactData) { + this.artifact = Iterables.getOnlyElement(artifactData.keySet()); + this.value = artifactData.get(artifact); + } + + // Override to avoid creating an ImmutableMap in the common case that the requested artifact is + // correct. This bypasses the preconditions checks in super, but if the artifact is correct, + // those would all pass anyway. + @Override + public final FileArtifactValue getExistingFileArtifactValue(Artifact artifact) { + if (artifact.equals(this.artifact)) { + return value; + } + // This will throw an exception. Call super to make failure modes consistent. + return super.getExistingFileArtifactValue(artifact); + } + + @Override + public final ImmutableMap getAllFileValues() { + return ImmutableMap.of(artifact, value); + } + } + + /** + * The result of a {@link + * com.google.devtools.build.lib.view.fileset.SkyframeFilesetManifestAction}. + */ + private static final class Fileset extends SingleOutputFile { + private final ImmutableList outputSymlinks; + + Fileset( + ImmutableMap artifactData, + ImmutableList outputSymlinks) { + super(artifactData); + this.outputSymlinks = outputSymlinks; + } + + @Override + public ImmutableList getOutputSymlinks() { + return outputSymlinks; + } + } + + /** + * The result of a {@link com.google.devtools.build.lib.rules.cpp.CppCompileAction} that + * {@linkplain IncludeScannable#getDiscoveredModules discovers modules}. + */ + private static final class ModuleDiscovering extends SingleOutputFile { + private final NestedSet discoveredModules; + + ModuleDiscovering( + ImmutableMap artifactData, + NestedSet discoveredModules) { + super(artifactData); + this.discoveredModules = discoveredModules; + } + + @Override + public NestedSet getDiscoveredModules() { + return discoveredModules; + } + } + + /** The result of an action that outputs an arbitrary number of files. */ + private static class MultiOutputFile extends ActionExecutionValue { + private final ImmutableMap artifactData; + + MultiOutputFile(ImmutableMap artifactData) { + this.artifactData = artifactData; + } + + @Override + public final ImmutableMap getAllFileValues() { + return artifactData; + } + } + + /** The result of an action that outputs a single tree artifact and no other files. */ + private static final class SingleTree extends ActionExecutionValue { + private final Artifact treeArtifact; + private final TreeArtifactValue treeValue; + + SingleTree(ImmutableMap treeArtifactData) { + this.treeArtifact = Iterables.getOnlyElement(treeArtifactData.keySet()); + this.treeValue = treeArtifactData.get(treeArtifact); + } + + @Override + @Nullable + TreeArtifactValue getTreeArtifactValue(Artifact artifact) { + checkArgument(artifact.isTreeArtifact(), artifact); + return artifact.equals(treeArtifact) ? treeValue : null; + } + + @Override + public ImmutableMap getAllTreeArtifactValues() { + return ImmutableMap.of(treeArtifact, treeValue); + } + + @Override + public ImmutableMap getAllFileValues() { + return ImmutableMap.of(); + } + } + + /** + * The result of an action that outputs multiple tree artifacts or a combination of tree artifacts + * and files. + */ + private static final class MultiTree extends MultiOutputFile { + private final ImmutableMap treeArtifactData; + + MultiTree( + ImmutableMap artifactData, + ImmutableMap treeArtifactData) { + super(artifactData); + this.treeArtifactData = treeArtifactData; + } + + @Override + @Nullable + TreeArtifactValue getTreeArtifactValue(Artifact artifact) { + checkArgument(artifact.isTreeArtifact(), artifact); + return treeArtifactData.get(artifact); + } + + @Override + public ImmutableMap getAllTreeArtifactValues() { + return treeArtifactData; + } + } } 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 573cebeb227bc8..2073969b881bd5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -465,6 +465,8 @@ java_library( "//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/rules/cpp:cpp_interface", + "//src/main/java/com/google/devtools/build/lib/util", + "//src/main/java/com/google/devtools/build/lib/util:hash_codes", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:guava", "//third_party:jsr305", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java index f81bb23bc17ce6..46783c99d7a53b 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java @@ -13,8 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; -import static org.mockito.Mockito.mock; - import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.testing.EqualsTester; @@ -30,17 +28,22 @@ import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.FilesetOutputSymlink; +import com.google.devtools.build.lib.actions.util.ActionsTestUtil; 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.skyframe.serialization.testutils.SerializationDepsUtils; +import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester; import com.google.devtools.build.lib.testutil.Scratch; +import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Root.RootCodecDependencies; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @RunWith(JUnit4.class) -public class ActionExecutionValueTest { +public final class ActionExecutionValueTest { private static final FileArtifactValue VALUE_1 = RemoteFileArtifactValue.create( /* digest= */ new byte[0], @@ -54,7 +57,7 @@ public class ActionExecutionValueTest { /* locationIndex= */ 2, /* expireAtEpochMilli= */ -1); - private static final ActionLookupKey KEY = mock(ActionLookupKey.class); + private static final ActionLookupKey KEY = ActionsTestUtil.NULL_ARTIFACT_OWNER; private static final ActionLookupData ACTION_LOOKUP_DATA_1 = ActionLookupData.create(KEY, 1); private static final ActionLookupData ACTION_LOOKUP_DATA_2 = ActionLookupData.create(KEY, 2); @@ -62,7 +65,7 @@ public class ActionExecutionValueTest { ArtifactRoot.asDerivedRoot(new Scratch().resolve("/execroot"), RootType.Output, "out"); @Test - public void equals_returnsFalseForDifferentValues() { + public void equality() { SpecialArtifact tree1 = tree("tree1"); TreeArtifactValue tree1Value1 = TreeArtifactValue.newBuilder(tree1) @@ -84,7 +87,6 @@ public void equals_returnsFalseForDifferentValues() { PathFragment.create("execPath2")); new EqualsTester() - .addEqualityGroup(createWithArtifactData(ImmutableMap.of())) .addEqualityGroup(createWithArtifactData(ImmutableMap.of(output("file1"), VALUE_1))) .addEqualityGroup(createWithArtifactData(ImmutableMap.of(output("file1"), VALUE_2))) .addEqualityGroup( @@ -129,37 +131,66 @@ public void equals_returnsFalseForDifferentValues() { NestedSetBuilder.stableOrder() .add(output("file1")) .addTransitive(NestedSetBuilder.create(Order.STABLE_ORDER, output("file2"))))) - // Non-empty collection for each member. - .addEqualityGroup( - ActionExecutionValue.createForTesting( - /* artifactData= */ ImmutableMap.of(output("file1"), VALUE_1), - /* treeArtifactData= */ ImmutableMap.of(tree1, tree1Value1), - /* outputSymlinks= */ ImmutableList.of(symlink1), - /* discoveredModules= */ NestedSetBuilder.create( - Order.STABLE_ORDER, output("file1")))) .testEquals(); } - ActionExecutionValue createWithArtifactData( + @Test + public void serialization() throws Exception { + new SerializationTester( + // Single output file + createWithArtifactData(ImmutableMap.of(output("output1"), VALUE_1)), + // Fileset + createWithOutputSymlinks( + ImmutableList.of( + FilesetOutputSymlink.createForTesting( + PathFragment.create("name"), + PathFragment.create("target"), + PathFragment.create("execPath")))), + // Module discovering + createWithDiscoveredModules( + NestedSetBuilder.create(Order.STABLE_ORDER, output("module"))), + // Multiple output files + createWithArtifactData( + ImmutableMap.of(output("output1"), VALUE_1, output("output2"), VALUE_2)), + // Single tree + createWithTreeArtifactData(ImmutableMap.of(tree("tree"), TreeArtifactValue.empty())), + // Multiple trees + createWithTreeArtifactData( + ImmutableMap.of( + tree("tree1"), + TreeArtifactValue.empty(), + tree("tree2"), + TreeArtifactValue.empty())), + // Mixed file and tree + ActionExecutionValue.createForTesting( + ImmutableMap.of(output("file"), VALUE_1), + ImmutableMap.of(tree("tree"), TreeArtifactValue.empty()), + /* outputSymlinks= */ null)) + .addDependency(FileSystem.class, OUTPUT_ROOT.getRoot().getFileSystem()) + .addDependency( + RootCodecDependencies.class, new RootCodecDependencies(OUTPUT_ROOT.getRoot())) + .addDependencies(SerializationDepsUtils.SERIALIZATION_DEPS_FOR_TEST) + .runTests(); + } + + private static ActionExecutionValue createWithArtifactData( ImmutableMap artifactData) { return ActionExecutionValue.createForTesting( /* artifactData= */ artifactData, /* treeArtifactData= */ ImmutableMap.of(), - /* outputSymlinks= */ ImmutableList.of()); + /* outputSymlinks= */ null); } - ActionExecutionValue createWithTreeArtifactData( + private static ActionExecutionValue createWithTreeArtifactData( ImmutableMap treeArtifactData) { return ActionExecutionValue.createForTesting( - /* artifactData= */ ImmutableMap.of(), - treeArtifactData, - /* outputSymlinks= */ ImmutableList.of()); + /* artifactData= */ ImmutableMap.of(), treeArtifactData, /* outputSymlinks= */ null); } - ActionExecutionValue createWithOutputSymlinks( + private static ActionExecutionValue createWithOutputSymlinks( ImmutableList outputSymlinks) { return ActionExecutionValue.createForTesting( - /* artifactData= */ ImmutableMap.of(), + ImmutableMap.of(output("fileset.manifest"), VALUE_1), /* treeArtifactData= */ ImmutableMap.of(), outputSymlinks); } @@ -171,10 +202,10 @@ private static ActionExecutionValue createWithDiscoveredModules( private static ActionExecutionValue createWithDiscoveredModules( NestedSet discoveredModules) { - return ActionExecutionValue.createForTesting( - /* artifactData= */ ImmutableMap.of(), + return ActionExecutionValue.create( + /* artifactData= */ ImmutableMap.of(output("modules.pcm"), VALUE_1), /* treeArtifactData= */ ImmutableMap.of(), - /* outputSymlinks= */ ImmutableList.of(), + /* outputSymlinks= */ null, discoveredModules); } 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 139107b3b56862..3865ed01be60cd 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 @@ -36,7 +36,6 @@ import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; import com.google.devtools.build.lib.actions.BasicActionLookupValue; import com.google.devtools.build.lib.actions.FileArtifactValue; -import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.actions.MiddlemanType; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; @@ -46,12 +45,9 @@ import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.events.NullEventHandler; import com.google.devtools.build.lib.skyframe.ArtifactFunction.SourceArtifactException; -import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationDepsUtils; -import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester; import com.google.devtools.build.lib.util.Pair; 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.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; @@ -315,41 +311,6 @@ public void cannotOmitSomeButNotAllActionTemplateExpansionOutputs() throws Throw + ".*"); } - @Test - public void actionExecutionValueSerialization() throws Exception { - ActionLookupData dummyData = ActionLookupData.create(ALL_OWNER, 0); - DerivedArtifact artifact1 = createDerivedArtifact("one"); - FileArtifactValue metadata1 = - ActionMetadataHandler.fileArtifactValueFromArtifact( - artifact1, null, SyscallCache.NO_CACHE, null); - SpecialArtifact treeArtifact = createDerivedTreeArtifactOnly("tree"); - treeArtifact.setGeneratingActionKey(dummyData); - TreeFileArtifact treeFileArtifact = TreeFileArtifact.createTreeOutput(treeArtifact, "subpath"); - Path path = treeFileArtifact.getPath(); - path.getParentDirectory().createDirectoryAndParents(); - writeFile(path, "contents"); - TreeArtifactValue tree = - TreeArtifactValue.newBuilder(treeArtifact) - .putChild(treeFileArtifact, FileArtifactValue.createForTesting(treeFileArtifact)) - .build(); - DerivedArtifact artifact3 = createDerivedArtifact("three"); - FilesetOutputSymlink filesetOutputSymlink = - FilesetOutputSymlink.createForTesting( - PathFragment.EMPTY_FRAGMENT, PathFragment.EMPTY_FRAGMENT, PathFragment.EMPTY_FRAGMENT); - ActionExecutionValue actionExecutionValue = - ActionExecutionValue.createForTesting( - ImmutableMap.of(artifact1, metadata1, artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN), - ImmutableMap.of(treeArtifact, tree), - ImmutableList.of(filesetOutputSymlink)); - new SerializationTester(actionExecutionValue) - .addDependency(FileSystem.class, root.getFileSystem()) - .addDependency( - Root.RootCodecDependencies.class, - new Root.RootCodecDependencies(Root.absoluteRoot(root.getFileSystem()))) - .addDependencies(SerializationDepsUtils.SERIALIZATION_DEPS_FOR_TEST) - .runTests(); - } - private static void file(Path path, String contents) throws Exception { path.getParentDirectory().createDirectoryAndParents(); writeFile(path, contents);