Always create a ResolvedFileArtifactValue for an artifact materialized as a symlink to another.
This will be useful in upcoming changes extending the concept of lazy materialization to non-remote artifacts.
PiperOrigin-RevId: 723477378
Change-Id: I704f13782c0dbfeb65f9e13e2dbaede745ec2202
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java
index c55a589..7afddae 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java
@@ -302,10 +302,12 @@
}
}
- // Same rationale as for constructFileArtifactValue.
- if (anyRemote.get()
- && treeDir.isSymbolicLink()
- && stat instanceof FileStatusWithMetadata statWithMetadata) {
+ // If the artifact was materialized in the filesystem as as symlink to another artifact, record
+ // the real path in the metadata so that it can be recreated as such later.
+ // See {@link FileArtifactValue#getResolvedPath} for why this is useful.
+ // TODO(tjgq): Actually check whether the path matches one of the action inputs. The presence
+ // of a FileStatusWithMetadata happens to coincide, but seems a little brittle.
+ if (stat instanceof FileStatusWithMetadata statWithMetadata && treeDir.isSymbolicLink()) {
FileArtifactValue metadata = statWithMetadata.getMetadata();
PathFragment resolvedPath = metadata.getResolvedPath();
if (resolvedPath != null) {
@@ -409,21 +411,6 @@
artifact.isConstantMetadata() ? null : tsgm);
var value = statAndValue.fileArtifactValue();
- // If the artifact is stored remotely but it was materialized in the filesystem as a symlink,
- // store the original path in the metadata so it can later be recreated as such to avoid
- // downloading multiple copies.
- //
- // This only affects Bazel when building without the bytes. In Blaze, without an action
- // filesystem, metadata is always local (isRemote() is false); and with an action filesystem,
- // createSymbolicLink() is implemented as a file copy (isSymbolicLink() is false).
- if (value.isRemote()
- && value.getResolvedPath() == null
- && statAndValue.statNoFollow().isSymbolicLink()) {
- value =
- FileArtifactValue.createFromExistingWithResolvedPath(
- value, statAndValue.realPath().asFragment());
- }
-
// Ensure that we don't have both an injected digest and a digest from the filesystem.
byte[] fileDigest = value.getDigest();
@@ -556,6 +543,18 @@
FileStatusWithDigest realStatWithDigest = FileStatusWithDigestAdapter.maybeAdapt(realStat);
var fileArtifactValue =
fileArtifactValueFromStat(realRootedPath, realStatWithDigest, xattrProvider, tsgm);
+
+ // If the artifact was materialized in the filesystem as as symlink to another artifact, record
+ // the real path in the metadata so that it can be recreated as such later.
+ // See {@link FileArtifactValue#getResolvedPath} for why this is useful.
+ // TODO(tjgq): Actually check whether the path matches one of the action inputs. The presence
+ // of a FileStatusWithMetadata happens to coincide, but seems a little brittle.
+ if (realStat instanceof FileStatusWithMetadata && fileArtifactValue.getResolvedPath() == null) {
+ fileArtifactValue =
+ FileArtifactValue.createFromExistingWithResolvedPath(
+ fileArtifactValue, realRootedPath.asPath().asFragment());
+ }
+
return FileArtifactStatAndValue.create(pathNoFollow, realPath, statNoFollow, fileArtifactValue);
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java
index 1cfd0c0..8eaf7d2 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java
@@ -47,6 +47,7 @@
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.DigestUtils;
import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.OutputPermissions;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -67,27 +68,16 @@
@RunWith(TestParameterInjector.class)
public final class ActionOutputMetadataStoreTest {
+ private enum ArtifactType {
+ SOURCE,
+ OUTPUT
+ }
+
private enum ResolvedPathDepth {
SHALLOW,
DEEP
}
- private enum FileLocation {
- LOCAL,
- REMOTE
- }
-
- private enum TreeComposition {
- EMPTY,
- FULLY_LOCAL,
- FULLY_REMOTE,
- MIXED;
-
- boolean isPartiallyRemote() {
- return this == FULLY_REMOTE || this == MIXED;
- }
- }
-
private final Map<Path, Integer> chmodCalls = Maps.newConcurrentMap();
private final Scratch scratch =
@@ -392,97 +382,63 @@
}
@Test
- public void fileArtifactMaterializedAsSymlink(
- @TestParameter ResolvedPathDepth depth, @TestParameter FileLocation location)
- throws Exception {
- Artifact targetArtifact =
- ActionsTestUtil.createArtifactWithRootRelativePath(
- outputRoot, PathFragment.create("target"));
-
+ public void fileArtifactMaterializedAsSymlinkToNonArtifact() throws Exception {
+ Path resolvedPath = outputRoot.getRoot().getRelative("resolved");
Artifact outputArtifact =
ActionsTestUtil.createArtifactWithRootRelativePath(
outputRoot, PathFragment.create("output"));
- PathFragment preexistingPath =
- switch (depth) {
- case SHALLOW -> null;
- case DEEP -> outputRoot.getRoot().asPath().getRelative("preexisting").asFragment();
- };
-
- FileArtifactValue targetMetadata = createFileMetadataForSymlinkTest(location, preexistingPath);
- ActionInputMap inputMap = new ActionInputMap(0);
- inputMap.putWithNoDepOwner(targetArtifact, targetMetadata);
-
RemoteActionFileSystem actionFs =
- createRemoteActionFileSystem(inputMap, ImmutableSet.of(outputArtifact));
+ createRemoteActionFileSystem(new ActionInputMap(0), ImmutableSet.of(outputArtifact));
ActionOutputMetadataStore store = createStore(ImmutableSet.of(outputArtifact), actionFs);
store.prepareForActionExecution();
- // In a realistic scenario, files with local metadata should also exist on disk.
- // However, the action filesystem is expected to obtain their metadata from the input map.
actionFs
- .getPath(outputArtifact.getPath().getParentDirectory().getPathString())
+ .getPath(outputArtifact.getPath().asFragment())
+ .getParentDirectory()
.createDirectoryAndParents();
actionFs
- .getPath(outputArtifact.getPath().getPathString())
- .createSymbolicLink(targetArtifact.getPath().asFragment());
- if (preexistingPath != null) {
- actionFs
- .getPath(targetArtifact.getPath().getPathString())
- .createSymbolicLink(preexistingPath);
- }
-
- PathFragment expectedResolvedPath = null;
- if (location == FileLocation.REMOTE) {
- expectedResolvedPath =
- preexistingPath != null ? preexistingPath : targetArtifact.getPath().asFragment();
- }
+ .getPath(outputArtifact.getPath().asFragment())
+ .createSymbolicLink(resolvedPath.asFragment());
+ FileSystemUtils.writeContentAsLatin1(resolvedPath, "foo");
assertThat(store.getOutputMetadata(outputArtifact))
- .isEqualTo(createFileMetadataForSymlinkTest(location, expectedResolvedPath));
- }
-
- private static FileArtifactValue createFileMetadataForSymlinkTest(
- FileLocation location, @Nullable PathFragment resolvedPath) {
- switch (location) {
- case LOCAL:
- return FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 10);
- case REMOTE:
- {
- FileArtifactValue metadata =
- FileArtifactValue.createForRemoteFileWithMaterializationData(
- new byte[] {1, 2, 3}, 10, 1, null);
- if (resolvedPath != null) {
- metadata = FileArtifactValue.createFromExistingWithResolvedPath(metadata, resolvedPath);
- }
- return metadata;
- }
- }
- throw new AssertionError();
+ .isEqualTo(FileArtifactValue.createForTesting(resolvedPath));
}
@Test
- public void treeArtifactMaterializedAsSymlink(
- @TestParameter ResolvedPathDepth depth, @TestParameter TreeComposition composition)
+ public void fileArtifactMaterializedAsSymlinkToFileArtifact(
+ @TestParameter ArtifactType artifactType, @TestParameter ResolvedPathDepth depth)
throws Exception {
- SpecialArtifact targetArtifact =
- ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "target");
-
- SpecialArtifact outputArtifact =
- ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "output");
-
- PathFragment preexistingPath =
- switch (depth) {
- case SHALLOW -> null;
- case DEEP -> outputRoot.getRoot().asPath().getRelative("preexisting").asFragment();
+ ArtifactRoot root =
+ switch (artifactType) {
+ case SOURCE -> sourceRoot;
+ case OUTPUT -> outputRoot;
};
+ Artifact resolvedArtifact =
+ ActionsTestUtil.createArtifactWithRootRelativePath(root, PathFragment.create("resolved"));
+ Artifact inputArtifact =
+ switch (depth) {
+ case SHALLOW -> resolvedArtifact;
+ case DEEP ->
+ ActionsTestUtil.createArtifactWithRootRelativePath(
+ outputRoot, PathFragment.create("input"));
+ };
+ Artifact outputArtifact =
+ ActionsTestUtil.createArtifactWithRootRelativePath(
+ outputRoot, PathFragment.create("output"));
- TreeArtifactValue targetMetadata =
- createTreeMetadataForSymlinkTest(targetArtifact, composition, preexistingPath);
+ FileArtifactValue inputMetadata =
+ FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, null, 123L);
+ if (!resolvedArtifact.equals(inputArtifact)) {
+ inputMetadata =
+ FileArtifactValue.createFromExistingWithResolvedPath(
+ inputMetadata, resolvedArtifact.getPath().asFragment());
+ }
ActionInputMap inputMap = new ActionInputMap(0);
- inputMap.putTreeArtifact(targetArtifact, targetMetadata, /* depOwner= */ null);
+ inputMap.putWithNoDepOwner(inputArtifact, inputMetadata);
RemoteActionFileSystem actionFs =
createRemoteActionFileSystem(inputMap, ImmutableSet.of(outputArtifact));
@@ -490,66 +446,101 @@
ActionOutputMetadataStore store = createStore(ImmutableSet.of(outputArtifact), actionFs);
store.prepareForActionExecution();
- // In a realistic scenario, files with local metadata should also exist on disk.
- // However, the action filesystem is expected to obtain their metadata from the input map.
actionFs
- .getPath(outputArtifact.getPath().getParentDirectory().getPathString())
+ .getPath(outputArtifact.getPath().asFragment())
+ .getParentDirectory()
.createDirectoryAndParents();
- actionFs.getPath(targetArtifact.getPath().getPathString()).createDirectoryAndParents();
actionFs
- .getPath(outputArtifact.getPath().getPathString())
- .createSymbolicLink(targetArtifact.getPath().asFragment());
+ .getPath(outputArtifact.getPath().asFragment())
+ .createSymbolicLink(inputArtifact.getPath().asFragment());
- PathFragment expectedResolvedPath = null;
- if (composition.isPartiallyRemote()) {
- expectedResolvedPath =
- preexistingPath != null ? preexistingPath : targetArtifact.getPath().asFragment();
- }
+ assertThat(store.getOutputMetadata(outputArtifact))
+ .isEqualTo(
+ FileArtifactValue.createFromExistingWithResolvedPath(
+ inputMetadata, resolvedArtifact.getPath().asFragment()));
+ }
+
+ @Test
+ public void treeArtifactMaterializedAsSymlinkToNonArtifact() throws Exception {
+ Path resolvedPath = outputRoot.getRoot().getRelative("resolved");
+ SpecialArtifact outputArtifact =
+ ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "output");
+
+ RemoteActionFileSystem actionFs =
+ createRemoteActionFileSystem(new ActionInputMap(0), ImmutableSet.of(outputArtifact));
+
+ ActionOutputMetadataStore store = createStore(ImmutableSet.of(outputArtifact), actionFs);
+ store.prepareForActionExecution();
+
+ actionFs
+ .getPath(outputArtifact.getPath().asFragment())
+ .getParentDirectory()
+ .createDirectoryAndParents();
+ actionFs
+ .getPath(outputArtifact.getPath().asFragment())
+ .createSymbolicLink(resolvedPath.asFragment());
+ actionFs.getPath(resolvedPath.asFragment()).createDirectoryAndParents();
+ FileSystemUtils.writeContentAsLatin1(resolvedPath.getRelative("child"), "foo");
assertThat(store.getTreeArtifactValue(outputArtifact))
.isEqualTo(
- createTreeMetadataForSymlinkTest(outputArtifact, composition, expectedResolvedPath));
+ TreeArtifactValue.newBuilder(outputArtifact)
+ .putChild(
+ TreeFileArtifact.createTreeOutput(outputArtifact, "child"),
+ FileArtifactValue.createForTesting(resolvedPath.getRelative("child")))
+ .build());
}
- private static TreeArtifactValue createTreeMetadataForSymlinkTest(
- SpecialArtifact parent, TreeComposition composition, @Nullable PathFragment resolvedPath) {
- TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(parent);
+ @Test
+ public void treeArtifactMaterializedAsSymlinkToAnotherTreeArtifact(
+ @TestParameter ResolvedPathDepth depth) throws Exception {
+ SpecialArtifact resolvedArtifact =
+ ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "resolved");
+ SpecialArtifact inputArtifact =
+ switch (depth) {
+ case SHALLOW -> resolvedArtifact;
+ case DEEP -> ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "input");
+ };
+ SpecialArtifact outputArtifact =
+ ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "output");
- TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(parent, "child1");
- TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(parent, "child2");
-
- FileArtifactValue localMetadata1 =
- FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 10);
- FileArtifactValue localMetadata2 =
- FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 20);
-
- FileArtifactValue remoteMetadata1 =
- FileArtifactValue.createForRemoteFile(new byte[] {1, 2, 3}, 10, 1);
- FileArtifactValue remoteMetadata2 =
- FileArtifactValue.createForRemoteFile(new byte[] {4, 5, 6}, 20, 1);
-
- switch (composition) {
- case EMPTY:
- break;
- case FULLY_LOCAL:
- builder.putChild(child1, localMetadata1);
- builder.putChild(child2, localMetadata2);
- break;
- case FULLY_REMOTE:
- builder.putChild(child1, remoteMetadata1);
- builder.putChild(child2, remoteMetadata2);
- break;
- case MIXED:
- builder.putChild(child1, localMetadata1);
- builder.putChild(child2, remoteMetadata2);
- break;
+ TreeArtifactValue.Builder builder =
+ TreeArtifactValue.newBuilder(inputArtifact)
+ .putChild(
+ TreeFileArtifact.createTreeOutput(inputArtifact, "child"),
+ FileArtifactValue.createForNormalFile(
+ new byte[] {1, 2, 3}, /* proxy= */ null, 123L));
+ if (depth == ResolvedPathDepth.DEEP) {
+ builder.setResolvedPath(resolvedArtifact.getPath().asFragment());
}
+ TreeArtifactValue inputMetadata = builder.build();
- if (resolvedPath != null) {
- builder.setResolvedPath(resolvedPath);
- }
+ ActionInputMap inputMap = new ActionInputMap(0);
+ inputMap.putTreeArtifact(inputArtifact, inputMetadata, /* depOwner= */ null);
- return builder.build();
+ RemoteActionFileSystem actionFs =
+ createRemoteActionFileSystem(inputMap, ImmutableSet.of(outputArtifact));
+
+ ActionOutputMetadataStore store = createStore(ImmutableSet.of(outputArtifact), actionFs);
+ store.prepareForActionExecution();
+
+ actionFs
+ .getPath(outputArtifact.getPath().asFragment())
+ .getParentDirectory()
+ .createDirectoryAndParents();
+ actionFs
+ .getPath(outputArtifact.getPath().asFragment())
+ .createSymbolicLink(inputArtifact.getPath().asFragment());
+
+ assertThat(store.getTreeArtifactValue(outputArtifact))
+ .isEqualTo(
+ TreeArtifactValue.newBuilder(outputArtifact)
+ .putChild(
+ TreeFileArtifact.createTreeOutput(outputArtifact, "child"),
+ FileArtifactValue.createForNormalFile(
+ new byte[] {1, 2, 3}, /* proxy= */ null, 123L))
+ .setResolvedPath(resolvedArtifact.getPath().asFragment())
+ .build());
}
@Test
