`SymlinkAction` injects metadata from its input. This reduces filesystem ops. It also allows preserving richer metadata from the input, for example `RemoteFileArtifactValue`. It is incorrect to do this for directory artifacts. There was already an existing test case that caught this for a fileset, and this change adds one that catches this for a tree artifact. Added some useful helper methods to `BuildIntegrationTestCase`. PiperOrigin-RevId: 690849579 Change-Id: I8c44a4d28d21a4d27f216763124a48f93e9ed7a5
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 5537966..01da8d4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -1604,6 +1604,7 @@ "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifact_expander", "//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/analysis/platform", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/exec:spawn_log_context",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java index f6ca7b9..389f60b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.actions.ActionResult; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactExpander; +import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -242,6 +243,7 @@ } } + maybeInjectMetadata(actionExecutionContext); return ActionResult.EMPTY; } @@ -326,6 +328,44 @@ } } + /** + * Propagates metadata from the input artifact if possible. + * + * <p>This is an optimization that saves filesystem operations - we know the output is just a + * symlink to the input, so we may be able to skip constructing its metadata from the filesystem. + * + * <p>In addition to reducing filesystem operations, this allows us to provide richer information + * for the symlink metadata. For example, if the input metadata is a {@link + * com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue}, the output + * metadata will be as well. + * + * <p>In cases where propagating the input metadata is incorrect ({@linkplain Artifact#isDirectory + * directory artifacts}) or cases where the input metadata cannot be obtained, this method does + * nothing. The output symlink will be read back from the filesystem after this action finishes + * executing. + */ + private void maybeInjectMetadata(ActionExecutionContext ctx) { + if (ctx.getActionFileSystem() != null) { + return; // Action filesystems are responsible for their own metadata injection. + } + if (targetType == TargetType.FILESET) { + return; + } + Artifact primaryInput = getPrimaryInput(); + if (primaryInput == null || primaryInput.isDirectory()) { + return; + } + FileArtifactValue metadata; + try { + metadata = ctx.getInputMetadataProvider().getInputMetadata(primaryInput); + } catch (IOException e) { + return; + } + if (metadata != null) { + ctx.getOutputMetadataStore().injectFile(getPrimaryOutput(), metadata); + } + } + private String printInputs() { if (getInputs().isEmpty()) { return inputPath.getPathString();
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java index 580e58f..7d5db9e 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java
@@ -17,10 +17,12 @@ import static com.google.common.truth.Truth.assertWithMessage; import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.NULL_ACTION_OWNER; import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionExecutionContext.LostInputsCheck; import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; +import com.google.devtools.build.lib.actions.cache.OutputMetadataStore; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.actions.util.DummyExecutor; import com.google.devtools.build.lib.analysis.actions.SymlinkAction; @@ -72,7 +74,7 @@ execRoot.getPathString(), execRoot.getFileSystem(), SyscallCache.NO_CACHE), ActionInputPrefetcher.NONE, actionKeyContext, - /* outputMetadataStore= */ null, + mock(OutputMetadataStore.class), /* rewindingEnabled= */ false, LostInputsCheck.NONE, outErr,
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java index 4f6f134..e7fa3fe 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java
@@ -15,6 +15,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.NULL_ACTION_OWNER; +import static org.mockito.Mockito.mock; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionExecutionContext; @@ -24,7 +25,9 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.DiscoveredModulesPruner; import com.google.devtools.build.lib.actions.Executor; +import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.ThreadStateReceiver; +import com.google.devtools.build.lib.actions.cache.OutputMetadataStore; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.events.StoredEventHandler; @@ -94,10 +97,10 @@ action.execute( new ActionExecutionContext( executor, - /* inputMetadataProvider= */ null, + mock(InputMetadataProvider.class), ActionInputPrefetcher.NONE, actionKeyContext, - /* outputMetadataStore= */ null, + mock(OutputMetadataStore.class), /* rewindingEnabled= */ false, LostInputsCheck.NONE, /* fileOutErr= */ null,
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java b/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java index 8167acc..91d4b29 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
@@ -20,6 +20,7 @@ import static com.google.common.base.Throwables.throwIfUnchecked; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.common.collect.MoreCollectors.onlyElement; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import static java.nio.charset.StandardCharsets.ISO_8859_1; @@ -39,6 +40,7 @@ import com.google.devtools.build.lib.actions.ActionGraph; 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.SourceArtifact; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.InlineFileArtifactValue; @@ -704,23 +706,52 @@ * Given a label (which has typically, but not necessarily, just been built), returns the * collection of files that it produces. * - * @param target the label of the target whose artifacts are requested. + * @param target the label of the target whose artifacts are requested */ protected ImmutableList<Artifact> getArtifacts(String target) - throws LabelSyntaxException, NoSuchPackageException, NoSuchTargetException, - InterruptedException, TransitionException, InvalidConfigurationException { + throws LabelSyntaxException, + NoSuchPackageException, + NoSuchTargetException, + InterruptedException, + TransitionException, + InvalidConfigurationException { return getFilesToBuild(getConfiguredTarget(target)).toList(); } /** + * Given a label (which has typically, but not necessarily, just been built), returns the file it + * produces ending with the given suffix. + * + * <p>It is an error if the given target produces multiple files with the given suffix. + * + * @param target the label of the target whose artifact is requested + * @param suffix suffix of the artifact requested + */ + protected Artifact getArtifact(String target, String suffix) + throws LabelSyntaxException, + NoSuchPackageException, + NoSuchTargetException, + InterruptedException, + TransitionException, + InvalidConfigurationException { + return getArtifacts(target).stream() + .filter(a -> a.getExecPathString().endsWith(suffix)) + .collect(onlyElement()); + } + + /** * Given a label (which has typically, but not necessarily, just been built), returns the * configured target for it using the request configuration. * * @param target the label of the requested target. */ protected ConfiguredTarget getConfiguredTarget(String target) - throws LabelSyntaxException, NoSuchPackageException, NoSuchTargetException, - InterruptedException, TransitionException, InvalidConfigurationException { + throws LabelSyntaxException, + NoSuchPackageException, + NoSuchTargetException, + InterruptedException, + TransitionException, + InvalidConfigurationException { getPackageManager().getTarget(events.reporter(), Label.parseCanonical(target)); return getSkyframeExecutor() .getConfiguredTargetForTesting(events.reporter(), label(target), getTargetConfiguration()); @@ -992,21 +1023,30 @@ } protected String readInlineOutput(Artifact output) throws IOException, InterruptedException { - assertThat(output).isInstanceOf(DerivedArtifact.class); + FileArtifactValue metadata = getOutputMetadata(output); + assertThat(metadata).isInstanceOf(InlineFileArtifactValue.class); + return new String( + FileSystemUtils.readContentAsLatin1(((InlineFileArtifactValue) metadata).getInputStream())); + } + protected FileArtifactValue getOutputMetadata(Artifact output) + throws IOException, InterruptedException { + assertThat(output).isInstanceOf(DerivedArtifact.class); SkyValue actionExecutionValue = getSkyframeExecutor() .getEvaluator() .getExistingValue(((DerivedArtifact) output).getGeneratingActionKey()); assertThat(actionExecutionValue).isInstanceOf(ActionExecutionValue.class); + return ((ActionExecutionValue) actionExecutionValue).getExistingFileArtifactValue(output); + } - FileArtifactValue fileArtifactValue = - ((ActionExecutionValue) actionExecutionValue).getExistingFileArtifactValue(output); - assertThat(fileArtifactValue).isInstanceOf(InlineFileArtifactValue.class); - - return new String( - FileSystemUtils.readContentAsLatin1( - ((InlineFileArtifactValue) fileArtifactValue).getInputStream())); + protected FileArtifactValue getSourceArtifactMetadata(Artifact sourceArtifact) + throws InterruptedException { + assertThat(sourceArtifact).isInstanceOf(SourceArtifact.class); + SkyValue sourceArtifactValue = + getSkyframeExecutor().getEvaluator().getExistingValue(sourceArtifact); + assertThat(sourceArtifactValue).isInstanceOf(FileArtifactValue.class); + return (FileArtifactValue) sourceArtifactValue; } /**
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD index b5a67f6..8229001 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -1593,6 +1593,7 @@ "//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/analysis:actions/spawn_action_template", + "//src/main/java/com/google/devtools/build/lib/analysis:actions/symlink_action", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/skyframe:action_template_expansion_value",
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 06f6c77..0be6c1d 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
@@ -46,6 +46,7 @@ import com.google.devtools.build.lib.actions.util.TestAction; import com.google.devtools.build.lib.actions.util.TestAction.DummyAction; import com.google.devtools.build.lib.analysis.actions.SpawnActionTemplate; +import com.google.devtools.build.lib.analysis.actions.SymlinkAction; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.events.Event; @@ -884,11 +885,33 @@ TreeFileArtifact.createTreeOutput(treeArtifact, "link/file")); } + @Test + public void symlinkActionToTreeArtifact() throws Exception { + SpecialArtifact tree1 = createTreeArtifact("tree1"); + registerAction( + new SimpleTestAction(/* output= */ tree1) { + @Override + void run(ActionExecutionContext context) throws IOException { + touchFile(tree1.getPath().getChild("file")); + } + }); + + SpecialArtifact tree2 = createTreeArtifact("tree2"); + registerAction( + SymlinkAction.toArtifact( + ActionsTestUtil.NULL_ACTION_OWNER, tree1, tree2, "Symlinking tree2 -> tree1")); + + // The SymlinkAction should produce a TreeArtifactValue with tree2 as the parent. + TreeArtifactValue tree2Value = buildArtifact(tree2); + assertThat(tree2Value.getChildren()) + .containsExactly(TreeFileArtifact.createTreeOutput(tree2, "file")); + } + private abstract static class SimpleTestAction extends TestAction { private final Button button; SimpleTestAction(Artifact output) { - this(/*inputs=*/ ImmutableList.of(), output); + this(/* inputs= */ ImmutableList.of(), output); } SimpleTestAction(Iterable<Artifact> inputs, Artifact output) {