`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) {
