Handle null action inputs in TreeNodeRepository

The SpawnInputExpander can return null action inputs to indicate that we
should create an empty file at the corresponding location, without a
corresponding input file.

PiperOrigin-RevId: 154832564

7 files changed, 192 insertions, 13 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/remote/Chunker.java b/src/main/java/com/google/devtools/build/lib/remote/Chunker.java
index afccdc68f4..a9bdd6dc13 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/Chunker.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/Chunker.java
@@ -18,12 +18,14 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterators;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
+import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.remote.RemoteProtocol.BlobChunk;
 import com.google.devtools.build.lib.remote.RemoteProtocol.ContentDigest;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.protobuf.ByteString;
 import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
@@ -103,7 +105,7 @@ public final class Chunker {
       chunk.setOffset(offset);
     }
     if (bytesLeft > 0) {
-      byte[] blob = new byte[(int) Math.min(bytesLeft, (long) chunkSize)];
+      byte[] blob = new byte[(int) Math.min(bytesLeft, chunkSize)];
       currentStream.read(blob);
       chunk.setData(ByteString.copyFrom(blob));
       bytesLeft -= blob.length;
@@ -145,6 +147,9 @@ public final class Chunker {
 
   static Item toItem(
       final ActionInput input, final ActionInputFileCache inputCache, final Path execRoot) {
+    if (input instanceof VirtualActionInput) {
+      return toItem((VirtualActionInput) input);
+    }
     return new Item() {
       @Override
       public ContentDigest getDigest() throws IOException {
@@ -158,6 +163,22 @@ public final class Chunker {
     };
   }
 
+  static Item toItem(final VirtualActionInput input) {
+    return new Item() {
+      @Override
+      public ContentDigest getDigest() throws IOException {
+        return ContentDigests.computeDigest(input);
+      }
+
+      @Override
+      public InputStream getInputStream() throws IOException {
+        ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+        input.writeTo(buffer);
+        return new ByteArrayInputStream(buffer.toByteArray());
+      }
+    };
+  }
+
   /**
    * Create a Chunker from a given ActionInput, taking its digest from the provided
    * ActionInputFileCache.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/ContentDigests.java b/src/main/java/com/google/devtools/build/lib/remote/ContentDigests.java
index d192598bd2..21a739c468 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/ContentDigests.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/ContentDigests.java
@@ -18,12 +18,14 @@ import com.google.common.hash.HashCode;
 import com.google.common.hash.Hashing;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
+import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.remote.RemoteProtocol.Action;
 import com.google.devtools.build.lib.remote.RemoteProtocol.ContentDigest;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.protobuf.ByteString;
 import com.google.protobuf.Message;
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 
 /** Helper methods relating to computing ContentDigest messages for remote execution. */
@@ -35,11 +37,16 @@ public final class ContentDigests {
     return buildDigest(Hashing.sha1().hashBytes(blob).asBytes(), blob.length);
   }
 
-  // TODO(olaola): cache these in ActionInputFileCache!
   public static ContentDigest computeDigest(Path file) throws IOException {
     return buildDigest(file.getSHA1Digest(), file.getFileSize());
   }
 
+  public static ContentDigest computeDigest(VirtualActionInput input) throws IOException {
+    ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+    input.writeTo(buffer);
+    return computeDigest(buffer.toByteArray());
+  }
+
   /**
    * Computes a digest of the given proto message. Currently, we simply rely on message output as
    * bytes, but this implementation relies on the stability of the proto encoding, in particular
diff --git a/src/main/java/com/google/devtools/build/lib/remote/EmptyActionInput.java b/src/main/java/com/google/devtools/build/lib/remote/EmptyActionInput.java
new file mode 100644
index 0000000000..8d25a0b6be
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/remote/EmptyActionInput.java
@@ -0,0 +1,52 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.remote;
+
+import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
+import com.google.devtools.build.lib.util.Preconditions;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import java.io.IOException;
+import java.io.OutputStream;
+
+/**
+ * In some cases, we want empty files in the runfiles tree that have no corresponding artifact. We
+ * use instances of this class to represent those files.
+ */
+final class EmptyActionInput implements VirtualActionInput {
+  private final PathFragment execPath;
+
+  public EmptyActionInput(PathFragment execPath) {
+    this.execPath = Preconditions.checkNotNull(execPath);
+  }
+
+  @Override
+  public String getExecPathString() {
+    return execPath.getPathString();
+  }
+
+  @Override
+  public PathFragment getExecPath() {
+    return execPath;
+  }
+
+  @Override
+  public void writeTo(OutputStream out) throws IOException {
+    // Write no content - it's an empty file.
+  }
+
+  @Override
+  public String toString() {
+    return "EmptyActionInput: " + execPath;
+  }
+}
\ No newline at end of file
diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
index 6eb90fa4a1..3c0e136101 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
@@ -17,6 +17,7 @@ package com.google.devtools.build.lib.remote;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
+import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.remote.ContentDigests.ActionKey;
 import com.google.devtools.build.lib.remote.RemoteProtocol.ActionResult;
@@ -31,6 +32,7 @@ import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.protobuf.ByteString;
 import com.google.protobuf.InvalidProtocolBufferException;
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.util.ArrayList;
@@ -60,6 +62,7 @@ public final class SimpleBlobStoreActionCache implements RemoteActionCache {
     for (FileNode fileNode : repository.treeToFileNodes(root)) {
       uploadBlob(fileNode.toByteArray());
     }
+    // TODO(ulfjack): Only upload files that aren't in the CAS yet?
     for (TreeNode leaf : repository.leaves(root)) {
       uploadFileContents(leaf.getActionInput(), execRoot, repository.getInputFileCache());
     }
@@ -89,6 +92,12 @@ public final class SimpleBlobStoreActionCache implements RemoteActionCache {
       ActionInput input, Path execRoot, ActionInputFileCache inputCache)
       throws IOException, InterruptedException {
     // This unconditionally reads the whole file into memory first!
+    if (input instanceof VirtualActionInput) {
+      ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+      ((VirtualActionInput) input).writeTo(buffer);
+      byte[] blob = buffer.toByteArray();
+      return uploadBlob(blob, ContentDigests.computeDigest(blob));
+    }
     return uploadBlob(
         ByteString.readFrom(execRoot.getRelative(input.getExecPathString()).getInputStream())
             .toByteArray(),
diff --git a/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java b/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java
index 0850fec75a..fa9eb7d6ee 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java
@@ -24,9 +24,11 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.TreeTraverser;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
+import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.exec.SpawnInputExpander;
 import com.google.devtools.build.lib.remote.RemoteProtocol.ContentDigest;
 import com.google.devtools.build.lib.remote.RemoteProtocol.FileNode;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -183,6 +185,8 @@ public final class TreeNodeRepository extends TreeTraverser<TreeNodeRepository.T
   private final Map<TreeNode, ContentDigest> treeNodeDigestCache = new HashMap<>();
   private final Map<ContentDigest, TreeNode> digestTreeNodeCache = new HashMap<>();
   private final Map<TreeNode, FileNode> fileNodeCache = new HashMap<>();
+  private final Map<VirtualActionInput, ContentDigest> virtualInputDigestCache = new HashMap<>();
+  private final Map<ContentDigest, VirtualActionInput> digestVirtualInputCache = new HashMap<>();
 
   public TreeNodeRepository(Path execRoot, ActionInputFileCache inputFileCache) {
     this.execRoot = execRoot;
@@ -242,13 +246,20 @@ public final class TreeNodeRepository extends TreeTraverser<TreeNodeRepository.T
     for (PathFragment path : sortedMap.keySet()) {
       segments.add(path.getSegments());
     }
-    ImmutableList<ActionInput> inputs = ImmutableList.copyOf(sortedMap.values());
+    List<ActionInput> inputs = new ArrayList<>();
+    for (Map.Entry<PathFragment, ActionInput> e : sortedMap.entrySet()) {
+      if (e.getValue() == SpawnInputExpander.EMPTY_FILE) {
+        inputs.add(new EmptyActionInput(e.getKey()));
+      } else {
+        inputs.add(e.getValue());
+      }
+    }
     return buildParentNode(inputs, segments.build(), 0, inputs.size(), 0);
   }
 
   @SuppressWarnings("ReferenceEquality") // Segments are interned.
   private TreeNode buildParentNode(
-      ImmutableList<ActionInput> inputs,
+      List<ActionInput> inputs,
       ImmutableList<ImmutableList<String>> segments,
       int inputsStart,
       int inputsEnd,
@@ -285,9 +296,22 @@ public final class TreeNodeRepository extends TreeTraverser<TreeNodeRepository.T
       FileNode.Builder b = FileNode.newBuilder();
       if (node.isLeaf()) {
         ActionInput input = node.getActionInput();
-        b.getFileMetadataBuilder()
-            .setDigest(ContentDigests.getDigestFromInputCache(input, inputFileCache))
-            .setExecutable(execRoot.getRelative(input.getExecPathString()).isExecutable());
+        if (input instanceof VirtualActionInput) {
+          VirtualActionInput virtualInput = (VirtualActionInput) input;
+          ContentDigest digest = ContentDigests.computeDigest(virtualInput);
+          virtualInputDigestCache.put(virtualInput, digest);
+          // There may be multiple inputs with the same digest. In that case, we don't care which
+          // one we get back from the digestVirtualInputCache later.
+          digestVirtualInputCache.put(digest, virtualInput);
+          b.getFileMetadataBuilder()
+              .setDigest(digest)
+              // We always declare virtual action inputs as non-executable for now.
+              .setExecutable(false);
+        } else {
+          b.getFileMetadataBuilder()
+              .setDigest(ContentDigests.getDigestFromInputCache(input, inputFileCache))
+              .setExecutable(execRoot.getRelative(input.getExecPathString()).isExecutable());
+        }
       } else {
         for (TreeNode.ChildEntry entry : node.getChildEntries()) {
           ContentDigest childDigest = treeNodeDigestCache.get(entry.getChild());
@@ -314,8 +338,10 @@ public final class TreeNodeRepository extends TreeTraverser<TreeNodeRepository.T
       }
     }
     if (root.isLeaf()) {
-      // Load the digest into the ActionInputFileCache.
-      inputFileCache.getDigest(root.getActionInput());
+      ActionInput input = root.getActionInput();
+      if (!(input instanceof VirtualActionInput)) {
+        inputFileCache.getDigest(input);
+      }
     } else {
       for (TreeNode child : children(root)) {
         computeMerkleDigests(child);
@@ -341,12 +367,19 @@ public final class TreeNodeRepository extends TreeTraverser<TreeNodeRepository.T
     for (TreeNode node : descendants(root)) {
       digests.add(Preconditions.checkNotNull(treeNodeDigestCache.get(node)));
       if (node.isLeaf()) {
-        digests.add(ContentDigests.getDigestFromInputCache(node.getActionInput(), inputFileCache));
+        digests.add(actionInputToDigest(node.getActionInput()));
       }
     }
     return digests.build();
   }
 
+  private ContentDigest actionInputToDigest(ActionInput input) throws IOException {
+    if (input instanceof VirtualActionInput) {
+      return Preconditions.checkNotNull(virtualInputDigestCache.get(input));
+    }
+    return ContentDigests.getDigestFromInputCache(input, inputFileCache);
+  }
+
   /**
    * Serializes all of the subtree to the file node list. TODO(olaola): add a version that only
    * copies a part of the tree that we are interested in. Should only be used after
@@ -354,7 +387,7 @@ public final class TreeNodeRepository extends TreeTraverser<TreeNodeRepository.T
    */
   // Note: this is not, strictly speaking, thread safe. If someone is deleting cached Merkle hashes
   // while this is executing, it will trigger an exception. But I think this is WAI.
-  public ImmutableList<FileNode> treeToFileNodes(TreeNode root) throws IOException {
+  public ImmutableList<FileNode> treeToFileNodes(TreeNode root) {
     ImmutableList.Builder<FileNode> fileNodes = ImmutableList.builder();
     for (TreeNode node : descendants(root)) {
       fileNodes.add(Preconditions.checkNotNull(fileNodeCache.get(node)));
@@ -372,9 +405,15 @@ public final class TreeNodeRepository extends TreeTraverser<TreeNodeRepository.T
       TreeNode treeNode = digestTreeNodeCache.get(digest);
       if (treeNode != null) {
         nodes.add(Preconditions.checkNotNull(fileNodeCache.get(treeNode)));
-      } else { // If not there, it must be an ActionInput.
+      } else {
+        // If not there, it must be an ActionInput.
         ByteString hexDigest = ByteString.copyFromUtf8(ContentDigests.toHexString(digest));
-        actionInputs.add(Preconditions.checkNotNull(inputFileCache.getInputFromDigest(hexDigest)));
+        ActionInput input = inputFileCache.getInputFromDigest(hexDigest);
+        if (input == null) {
+          // ... or a VirtualActionInput.
+          input = digestVirtualInputCache.get(digest);
+        }
+        actionInputs.add(Preconditions.checkNotNull(input));
       }
     }
   }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java b/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java
index db4e55dae1..4bb8883269 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java
@@ -29,7 +29,10 @@ import com.google.devtools.build.lib.testutil.Scratch;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
+import java.util.SortedMap;
+import java.util.TreeMap;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -124,4 +127,29 @@ public class TreeNodeRepositoryTest {
     // Reusing same node for the "foo" subtree: only need the root, root child, foo, and contents:
     assertThat(repo.getAllDigests(root)).hasSize(4);
   }
+
+  @Test
+  public void testNullArtifacts() throws Exception {
+    Artifact foo = new Artifact(scratch.file("/exec/root/a/foo", "1"), rootDir);
+    SortedMap<PathFragment, ActionInput> inputs = new TreeMap<>();
+    inputs.put(foo.getExecPath(), foo);
+    inputs.put(PathFragment.create("a/bar"), null);
+    TreeNodeRepository repo = createTestTreeNodeRepository();
+    TreeNode root = repo.buildFromActionInputs(inputs);
+    repo.computeMerkleDigests(root);
+
+    TreeNode aNode = root.getChildEntries().get(0).getChild();
+    TreeNode fooNode = aNode.getChildEntries().get(1).getChild(); // foo > bar in sort order!
+    TreeNode barNode = aNode.getChildEntries().get(0).getChild();
+    ImmutableCollection<ContentDigest> digests = repo.getAllDigests(root);
+    ContentDigest rootDigest = repo.getMerkleDigest(root);
+    ContentDigest aDigest = repo.getMerkleDigest(aNode);
+    ContentDigest fooDigest = repo.getMerkleDigest(fooNode);
+    ContentDigest fooContentsDigest = ContentDigests.computeDigest(foo.getPath());
+    ContentDigest barDigest = repo.getMerkleDigest(barNode);
+    ContentDigest barContentsDigest = ContentDigests.computeDigest(new byte[0]);
+    assertThat(digests)
+        .containsExactly(
+            rootDigest, aDigest, barDigest, barContentsDigest, fooDigest, fooContentsDigest);
+  }
 }
diff --git a/src/test/shell/bazel/remote_execution_test.sh b/src/test/shell/bazel/remote_execution_test.sh
index f3514d2593..66a99a6654 100755
--- a/src/test/shell/bazel/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote_execution_test.sh
@@ -189,6 +189,29 @@ EOF
     || fail "Remote cache generated different result"
 }
 
+function test_py_test() {
+  mkdir -p a
+  cat > a/BUILD <<EOF
+package(default_visibility = ["//visibility:public"])
+py_test(
+name = 'test',
+srcs = [ 'test.py' ],
+)
+EOF
+  cat > a/test.py <<EOF
+import sys
+if __name__ == "__main__":
+    sys.exit(0)
+EOF
+  bazel --host_jvm_args=-Dbazel.DigestFunction=SHA1 test \
+      --spawn_strategy=remote \
+      --remote_worker=localhost:${worker_port} \
+      --remote_cache=localhost:${worker_port} \
+      --test_output=errors \
+      //a:test >& $TEST_log \
+      || fail "Failed to run //a:test with remote execution"
+}
+
 # TODO(alpha): Add a test that fails remote execution when remote worker
 # supports sandbox.