Carry tree artifacts' self data through aggregating middlemen.

This CL is a followup to c868c47. It is
intended to fix #5296. The immediate
problem in that issue is that remote caching finds a tree artifact in an input
runfiles tree without metadata in the cache. The metadata isn't there because
the runfiles artifacts are now all behind a middleman action, and
ArtifactFunction didn't propagate tree artifact values through middlemen. This
change fixes the problem by adding a tree artifact's self data value to the
values carried through to dependent actions.

Remote caching with tree artifact inputs seems to have been working mostly by
accident. (Evidence: it wasn't tested, and remote execution of actions with tree
artifact inputs results a Java traceback before and after
c868c47.) This CL is only really a bandaid to
return to the old "working" state. If remote execution wants to start supporting
tree artifacts properly in the future, we'll likely need to carry the tree
artifact child data through middleman actions, too.

Closes #5299.

Change-Id: I2e07d4fca0f6d2d34d97b7edb27f9d0063776225
PiperOrigin-RevId: 199079382

3 files changed, 49 insertions, 7 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index 199916674d..bee72a2321 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -282,13 +282,15 @@ class ArtifactFunction implements SkyFunction {
       Artifact input = ArtifactSkyKey.artifact(entry.getKey());
       SkyValue inputValue = entry.getValue();
       Preconditions.checkNotNull(inputValue, "%s has null dep %s", artifact, input);
-      if (!(inputValue instanceof FileArtifactValue)) {
+      if (inputValue instanceof FileArtifactValue) {
+        inputs.add(Pair.of(input, (FileArtifactValue) inputValue));
+      } else if (inputValue instanceof TreeArtifactValue) {
+        inputs.add(Pair.of(input, ((TreeArtifactValue) inputValue).getSelfData()));
+      } else {
         // We do not recurse in aggregating middleman artifacts.
         Preconditions.checkState(!(inputValue instanceof AggregatingArtifactValue),
             "%s %s %s", artifact, action, inputValue);
-        continue;
       }
-      inputs.add(Pair.of(input, (FileArtifactValue) inputValue));
     }
     return (action.getActionType() == MiddlemanType.AGGREGATING_MIDDLEMAN)
         ? new AggregatingArtifactValue(inputs.build(), value)
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 96fd9fc9eb..4615783887 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -139,18 +139,25 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase {
     Artifact output = createMiddlemanArtifact("output");
     Artifact input1 = createSourceArtifact("input1");
     Artifact input2 = createDerivedArtifact("input2");
+    SpecialArtifact tree = createDerivedTreeArtifactWithAction("treeArtifact");
+    file(createFakeTreeFileArtifact(tree, "child1", "hello1").getPath(), "src1");
+    file(createFakeTreeFileArtifact(tree, "child2", "hello2").getPath(), "src2");
     Action action =
         new DummyAction(
-            ImmutableList.of(input1, input2), output, MiddlemanType.AGGREGATING_MIDDLEMAN);
+            ImmutableList.of(input1, input2, tree), output, MiddlemanType.AGGREGATING_MIDDLEMAN);
     actions.add(action);
     file(input2.getPath(), "contents");
     file(input1.getPath(), "source contents");
     evaluate(
         Iterables.toArray(
-            ArtifactSkyKey.mandatoryKeys(ImmutableSet.of(input2, input1, input2)), SkyKey.class));
+            ArtifactSkyKey.mandatoryKeys(ImmutableSet.of(input2, input1, input2, tree)),
+            SkyKey.class));
     SkyValue value = evaluateArtifactValue(output);
     assertThat(((AggregatingArtifactValue) value).getInputs())
-        .containsExactly(Pair.of(input1, create(input1)), Pair.of(input2, create(input2)));
+        .containsExactly(
+            Pair.of(input1, create(input1)),
+            Pair.of(input2, create(input2)),
+            Pair.of(tree, ((TreeArtifactValue) evaluateArtifactValue(tree)).getSelfData()));
   }
 
   /**
diff --git a/src/test/shell/bazel/remote/remote_execution_http_test.sh b/src/test/shell/bazel/remote/remote_execution_http_test.sh
index 8de00d2c4f..4b8741fcb2 100755
--- a/src/test/shell/bazel/remote/remote_execution_http_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_http_test.sh
@@ -249,7 +249,10 @@ def _gen_output_dir_impl(ctx):
       arguments = [output_dir.path],
   )
   return [
-      DefaultInfo(files=depset(direct=[output_dir])),
+      DefaultInfo(
+          files=depset(direct=[output_dir]),
+          data_runfiles=ctx.runfiles(files=[output_dir]),
+      ),
   ]
 
 gen_output_dir = rule(
@@ -274,7 +277,26 @@ genrule(
     outs = ["qux"],
     cmd = "mkdir $@ && paste -d\"\n\" $(location :output_dir)/foo.txt $(location :output_dir)/sub1/bar.txt > $@/out.txt",
 )
+
+sh_binary(
+    name = "a-tool",
+    srcs = ["a-tool.sh"],
+    data = [":output_dir"],
+)
+
+genrule(
+    name = "test2",
+    outs = ["test2-out.txt"],
+    cmd = "$(location :a-tool) > $@",
+    tools = [":a-tool"],
+)
+EOF
+
+  cat > a/a-tool.sh <<'EOF'
+#!/bin/sh -eu
+cat "$0".runfiles/main/a/dir/foo.txt "$0".runfiles/main/a/dir/sub1/bar.txt
 EOF
+  chmod u+x a/a-tool.sh
 
   cat > a/test_expected <<EOF
 Hello, world!
@@ -325,4 +347,15 @@ function test_directory_artifact_skylark_rest_cache() {
       || fail "Remote cache generated different result"
 }
 
+function test_directory_artifact_in_runfiles_skylark_rest_cache() {
+  set_directory_artifact_skylark_testfixtures
+
+  bazel build \
+      --remote_rest_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
+      //a:test2 >& $TEST_log \
+      || fail "Failed to build //a:test2 with remote REST cache"
+  diff bazel-genfiles/a/test2-out.txt a/test_expected \
+      || fail "Remote cache generated different result"
+}
+
 run_suite "Remote execution and remote cache tests"