Use SpawnInputExpander in the remote spawn strategy to expand runfile…

…s trees

This fixes remote test execution.

Fixes bazelbuild#1593.

--
PiperOrigin-RevId: 151030133
MOS_MIGRATED_REVID=151030133

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java

@@ -21,7 +21,6 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
-import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionStatusMessage;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ExecutionStrategy;
@@ -33,6 +32,7 @@
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.exec.SpawnInputExpander;
 import com.google.devtools.build.lib.remote.ContentDigests.ActionKey;
 import com.google.devtools.build.lib.remote.RemoteProtocol.Action;
 import com.google.devtools.build.lib.remote.RemoteProtocol.ActionResult;
@@ -43,9 +43,11 @@
 import com.google.devtools.build.lib.remote.RemoteProtocol.ExecutionStatus;
 import com.google.devtools.build.lib.remote.RemoteProtocol.Platform;
 import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
+import com.google.devtools.build.lib.rules.fileset.FilesetActionContext;
 import com.google.devtools.build.lib.standalone.StandaloneSpawnStrategy;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.protobuf.TextFormat;
 import com.google.protobuf.TextFormat.ParseException;
 import io.grpc.StatusRuntimeException;
@@ -54,6 +56,7 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.SortedMap;
 import java.util.TreeSet;
 
 /**
@@ -71,6 +74,7 @@ final class RemoteSpawnStrategy implements SpawnActionContext {
   private final RemoteOptions options;
   // TODO(olaola): This will be set on a per-action basis instead.
   private final Platform platform;
+  private final SpawnInputExpander spawnInputExpander = new SpawnInputExpander(/*strict=*/false);
 
   RemoteSpawnStrategy(
       Map<String, String> clientEnv,
@@ -135,7 +139,14 @@ private void execLocally(
     if (options.remoteLocalExecUploadResults && actionCache != null && actionKey != null) {
       ArrayList<Path> outputFiles = new ArrayList<>();
       for (ActionInput output : spawn.getOutputFiles()) {
-        outputFiles.add(execRoot.getRelative(output.getExecPathString()));
+        Path outputFile = execRoot.getRelative(output.getExecPathString());
+        // Ignore non-existent files.
+        // TODO(ulfjack): This is not ideal - in general, all spawn strategies should stat the
+        // output files and return a list of existing files. We shouldn't re-stat the files here.
+        if (!outputFile.exists()) {
+          continue;
+        }
+        outputFiles.add(outputFile);
       }
       try {
         ActionResult.Builder result = ActionResult.newBuilder();
@@ -223,10 +234,13 @@ public void exec(Spawn spawn, ActionExecutionContext actionExecutionContext)
     try {
       // Temporary hack: the TreeNodeRepository should be created and maintained upstream!
       TreeNodeRepository repository = new TreeNodeRepository(execRoot);
-      List<ActionInput> inputs =
-          ActionInputHelper.expandArtifacts(
-              spawn.getInputFiles(), actionExecutionContext.getArtifactExpander());
-      TreeNode inputRoot = repository.buildFromActionInputs(inputs);
+      SortedMap<PathFragment, ActionInput> inputMap =
+          spawnInputExpander.getInputMapping(
+              spawn,
+              actionExecutionContext.getArtifactExpander(),
+              actionExecutionContext.getActionInputFileCache(),
+              actionExecutionContext.getExecutor().getContext(FilesetActionContext.class));
+      TreeNode inputRoot = repository.buildFromActionInputs(inputMap);
       repository.computeMerkleDigests(inputRoot);
       Command command = buildCommand(spawn.getArguments(), spawn.getEnvironment());
       Action action =
@@ -261,7 +275,7 @@ public void exec(Spawn spawn, ActionExecutionContext actionExecutionContext)
           ExecuteRequest.newBuilder()
               .setAction(action)
               .setAcceptCached(acceptCachedResult)
-              .setTotalInputFileCount(inputs.size())
+              .setTotalInputFileCount(inputMap.size())
               .setTimeoutMillis(1000 * Spawns.getTimeoutSeconds(spawn, 120));
       // TODO(olaola): set sensible local and remote timouts.
       ExecuteReply reply = workExecutor.executeRemotely(request.build());

src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java

@@ -38,6 +38,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.SortedMap;
 import java.util.TreeMap;
 import javax.annotation.Nullable;
 
@@ -217,16 +218,20 @@ public boolean apply(TreeNode node) {
         });
   }
 
+  public TreeNode buildFromActionInputs(Iterable<? extends ActionInput> inputs) {
+    TreeMap<PathFragment, ActionInput> sortedMap = new TreeMap<>();
+    for (ActionInput input : inputs) {
+      sortedMap.put(new PathFragment(input.getExecPathString()), input);
+    }
+    return buildFromActionInputs(sortedMap);
+  }
+
   /**
    * This function is a temporary and highly inefficient hack! It builds the tree from a ready list
    * of input files. TODO(olaola): switch to creating and maintaining the TreeNodeRepository based
    * on the build graph structure.
    */
-  public TreeNode buildFromActionInputs(Iterable<ActionInput> actionInputs) {
-    TreeMap<PathFragment, ActionInput> sortedMap = new TreeMap<>();
-    for (ActionInput input : actionInputs) {
-      sortedMap.put(new PathFragment(input.getExecPathString()), input);
-    }
+  public TreeNode buildFromActionInputs(SortedMap<PathFragment, ActionInput> sortedMap) {
     ImmutableList.Builder<ImmutableList<String>> segments = ImmutableList.builder();
     for (PathFragment path : sortedMap.keySet()) {
       segments.add(path.getSegments());

src/test/shell/bazel/remote_execution_test.sh

@@ -23,18 +23,6 @@ source "${CURRENT_DIR}/../integration_test_setup.sh" \
   || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
 
 function set_up() {
-  mkdir -p a
-  cat > a/BUILD <<EOF
-package(default_visibility = ["//visibility:public"])
-cc_binary(
-name = 'test',
-srcs = [ 'test.cc' ],
-)
-EOF
-  cat > a/test.cc <<EOF
-#include <iostream>
-int main() { std::cout << "Hello world!" << std::endl; return 0; }
-EOF
   work_path=$(mktemp -d ${TEST_TMPDIR}/remote.XXXXXXXX)
   pid_file=$(mktemp -u ${TEST_TMPDIR}/remote.XXXXXXXX)
   worker_port=$(pick_random_unused_tcp_port) || fail "no port found"
@@ -64,6 +52,18 @@ function tear_down() {
 }
 
 function test_cc_binary() {
+  mkdir -p a
+  cat > a/BUILD <<EOF
+package(default_visibility = ["//visibility:public"])
+cc_binary(
+name = 'test',
+srcs = [ 'test.cc' ],
+)
+EOF
+  cat > a/test.cc <<EOF
+#include <iostream>
+int main() { std::cout << "Hello world!" << std::endl; return 0; }
+EOF
   bazel build //a:test >& $TEST_log \
     || fail "Failed to build //a:test without remote execution"
   cp bazel-bin/a/test ${TEST_TMPDIR}/test_expected
@@ -79,6 +79,28 @@ function test_cc_binary() {
     || fail "Remote execution generated different result"
 }
 
+function test_cc_test() {
+  mkdir -p a
+  cat > a/BUILD <<EOF
+package(default_visibility = ["//visibility:public"])
+cc_test(
+name = 'test',
+srcs = [ 'test.cc' ],
+)
+EOF
+  cat > a/test.cc <<EOF
+#include <iostream>
+int main() { std::cout << "Hello test!" << std::endl; return 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.