Prohibit injecting tree artifact children as files. They should only be injected via injectDirectory.
This is consistent with the check in ActionExecutionValue which ensures that tree artifact children are not stored outside of their containing TreeArtifactValue.
This change allows for a followup optimization in the way we handle chmod calls on outputs - tree artifact children were the only case where a file could be injected and still passed to setPathReadOnlyAndExecutable.
PiperOrigin-RevId: 319844401
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 ae9cec4..1ec3765 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
@@ -27,8 +27,12 @@
*
* <p>This can be used to save filesystem operations when the metadata is already known.
*
+ * <p>{@linkplain Artifact#isTreeArtifacts Tree artifacts} and their {@linkplain
+ * Artifact#isChildOfDeclaredDirectory children} must not be passed here. Instead, they should be
+ * passed to {@link #injectDirectory}.
+ *
* @param output a regular output file
- * @param metadata the remote file metadata
+ * @param metadata the file metadata
*/
void injectFile(Artifact output, FileArtifactValue metadata);
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 7b3ff64..bd20553 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
@@ -391,7 +391,9 @@
Preconditions.checkArgument(
isKnownOutput(output), "%s is not a declared output of this action", output);
Preconditions.checkArgument(
- !output.isTreeArtifact(), "injectFile must not be called on TreeArtifacts: %s", output);
+ !output.isTreeArtifact() && !output.isChildOfDeclaredDirectory(),
+ "Tree artifacts and their children must be injected via injectDirectory: %s",
+ output);
Preconditions.checkState(
executionMode.get(), "Tried to inject %s outside of execution", output);
store.injectOutputData(output, metadata);
@@ -402,7 +404,8 @@
SpecialArtifact output, Map<TreeFileArtifact, FileArtifactValue> children) {
Preconditions.checkArgument(
isKnownOutput(output), "%s is not a declared output of this action", output);
- Preconditions.checkArgument(output.isTreeArtifact(), "output must be a tree artifact");
+ Preconditions.checkArgument(
+ output.isTreeArtifact(), "Output must be a tree artifact: %s", output);
Preconditions.checkState(
executionMode.get(), "Tried to inject %s outside of execution", output);
store.putTreeArtifactData(output, TreeArtifactValue.create(children));
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 ca3d511..3f004c5 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
@@ -401,16 +401,11 @@
}
@Test
- public void injectRemoteTreeFileArtifactMetadata() throws Exception {
- scratch.file("/output/bin/foo/bar/child1", "child1");
- scratch.file("/output/bin/foo/bar/child2", "child2");
+ public void cannotInjectTreeArtifactChildIndividually() {
SpecialArtifact treeArtifact =
ActionsTestUtil.createTreeArtifactWithGeneratingAction(
outputRoot, PathFragment.create("bin/foo/bar"));
- TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(treeArtifact, "child1");
- TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(treeArtifact, "child2");
- assertThat(child1.getPath().exists()).isTrue();
- assertThat(child2.getPath().exists()).isTrue();
+ TreeFileArtifact child = TreeFileArtifact.createTreeOutput(treeArtifact, "child");
OutputStore store = new OutputStore();
ActionMetadataHandler handler =
@@ -425,21 +420,40 @@
outputRoot.getRoot().asPath());
handler.discardOutputMetadata();
- RemoteFileArtifactValue child1Value = new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1);
- RemoteFileArtifactValue child2Value = new RemoteFileArtifactValue(new byte[] {4, 5, 6}, 10, 1);
+ RemoteFileArtifactValue childValue = new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1);
- handler.injectFile(child1, child1Value);
- handler.injectFile(child2, child2Value);
+ assertThrows(IllegalArgumentException.class, () -> handler.injectFile(child, childValue));
+ assertThat(store.getAllArtifactData()).isEmpty();
+ assertThat(store.getAllTreeArtifactData()).isEmpty();
+ }
- FileArtifactValue treeMetadata = handler.getMetadata(treeArtifact);
- FileArtifactValue child1Metadata = handler.getMetadata(child1);
- FileArtifactValue child2Metadata = handler.getMetadata(child2);
- TreeArtifactValue tree = store.getTreeArtifactData(treeArtifact);
+ @Test
+ public void canInjectTemplateExpansionOutput() {
+ SpecialArtifact treeArtifact =
+ ActionsTestUtil.createTreeArtifactWithGeneratingAction(
+ outputRoot, PathFragment.create("bin/foo/bar"));
+ TreeFileArtifact output =
+ TreeFileArtifact.createTemplateExpansionOutput(
+ treeArtifact, "output", ActionsTestUtil.NULL_TEMPLATE_EXPANSION_ARTIFACT_OWNER);
- assertThat(tree.getMetadata()).isEqualTo(treeMetadata);
- assertThat(tree.getChildValues())
- .containsExactly(child1, child1Metadata, child2, child2Metadata);
- assertThat(store.getAllArtifactData()).isEmpty(); // All data should be in treeArtifactData.
+ OutputStore store = new OutputStore();
+ ActionMetadataHandler handler =
+ new ActionMetadataHandler(
+ /*inputArtifactData=*/ new ActionInputMap(1),
+ ImmutableMap.of(),
+ /*missingArtifactsAllowed=*/ false,
+ /*outputs=*/ ImmutableList.of(treeArtifact),
+ /*tsgm=*/ null,
+ ArtifactPathResolver.IDENTITY,
+ store,
+ outputRoot.getRoot().asPath());
+ handler.discardOutputMetadata();
+
+ RemoteFileArtifactValue value = new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1);
+ handler.injectFile(output, value);
+
+ assertThat(store.getAllArtifactData()).containsExactly(output, value);
+ assertThat(store.getAllTreeArtifactData()).isEmpty();
}
@Test
