If a TreeFileArtifact is not explicitly in ActionExecutionValue's metadata map, check its TreeArtifactValue cache and return a value if it's there. Much of the time, a TreeFileArtifact's metadata will be in the main map: if it was the output of a templated action, it will be in the main map. It will also be there if it was explicitly statted/injected, either from Google-internal remote execution or as part of local execution.
However, if the tree artifact was injected via ActionMetadataHandler#injectRemoteDirectory, it is not present in the main metadata map.
I considered two other options for this CL:
(1) Explicitly add TreeFileArtifact metadata in ActionMetadataHandler#injectRemoteDirectory. Leads to increased memory usage for tree artifacts with Bazel remote execution (though just on parity with Google-internal/local) and maintains this as a sharp edge that could lead to crashes in the future if another use case is added.
(2) Explicitly add TreeFileArtifact metadata inside OutputStore#putTreeArtifactData. Duplicates work for local/Google-internal remote execution, where the metadata was already added, and has memory disadvantage.
This change seemed better: it makes explicit that TreeFileArtifacts don't have to be added, and saves a bit of memory at the cost of three map lookups instead of one for TreeFileArtifacts with Bazel remote execution. If Google-internal remote execution supported directories natively, we would move in this direction there as well.
PiperOrigin-RevId: 298601895
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 5db4328..33fb340 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
@@ -124,7 +124,16 @@
*/
@Nullable
public FileArtifactValue getArtifactValue(Artifact artifact) {
- return artifactData.get(artifact);
+ FileArtifactValue result = artifactData.get(artifact);
+ if (result != null || !artifact.hasParent()) {
+ return result;
+ }
+ // In some cases, TreeFileArtifact metadata may not have been injected directly, and is only
+ // available via the parent. However, if this ActionExecutionValue corresponds to a templated
+ // action, as opposed to an action that created a tree artifact itself, the TreeFileArtifact
+ // metadata will be in artifactData, since this value will have no treeArtifactData.
+ TreeArtifactValue treeArtifactValue = treeArtifactData.get(artifact.getParent());
+ return treeArtifactValue == null ? null : treeArtifactValue.getChildValues().get(artifact);
}
TreeArtifactValue getTreeArtifactValue(Artifact artifact) {
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 3c1a85d..bbf13fe 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
@@ -436,6 +436,8 @@
throw new IOException(errorMessage, e);
}
+ // TODO(janakr): we don't actually want the metadata for this TreeFileArtifact stored in the
+ // main metadata cache as of cl/297927844. Refactor and remove.
// A minor hack: maybeStoreAdditionalData will force the data to be stored via
// store.putAdditionalOutputData, if the underlying OutputStore supports it.
fileMetadata = maybeStoreAdditionalData(treeFileArtifact, fileMetadata, null);
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 1cf4552..bc7bd53 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
@@ -404,6 +404,11 @@
assertThat(treeValue.getChildPaths())
.containsExactly(PathFragment.create("foo"), PathFragment.create("bar"));
assertThat(treeValue.getChildValues().values()).containsExactly(fooValue, barValue);
+ ActionExecutionValue actionExecutionValue =
+ ActionExecutionValue.createFromOutputStore(handler.getOutputStore(), null, null, false);
+ treeValue
+ .getChildren()
+ .forEach(t -> assertThat(actionExecutionValue.getArtifactValue(t)).isNotNull());
}
@Test
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index 822900f..0d97941 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -166,6 +166,36 @@
|| fail "Remote execution generated different result"
}
+function test_cc_tree() {
+ if [[ "$PLATFORM" == "darwin" ]]; then
+ # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
+ # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
+ # action executors in order to select the appropriate Xcode toolchain.
+ return 0
+ fi
+
+ mkdir -p a
+ cat > a/BUILD <<EOF
+load(":tree.bzl", "mytree")
+mytree(name = "tree")
+cc_library(name = "tree_cc", srcs = [":tree"])
+EOF
+ cat > a/tree.bzl <<EOF
+def _tree_impl(ctx):
+ tree = ctx.actions.declare_directory("file.cc")
+ ctx.actions.run_shell(outputs = [tree],
+ command = "mkdir -p %s && touch %s/one.cc" % (tree.path, tree.path))
+ return [DefaultInfo(files = depset([tree]))]
+
+mytree = rule(implementation = _tree_impl)
+EOF
+ bazel build \
+ --remote_executor=grpc://localhost:${worker_port} \
+ --remote_download_minimal \
+ //a:tree_cc >& "$TEST_log" \
+ || fail "Failed to build //a:tree_cc with minimal downloads"
+}
+
function test_cc_test() {
if [[ "$PLATFORM" == "darwin" ]]; then
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
