Repository: bazelbuild/bazel
Issue: support run command for experimental_remote_download_outputs
Labels: type: feature request
team-Remote-Exec

### Description of the problem / feature request:

experimental_remote_download_outputs does not properly download all runtime dependencies.

### Feature requests: what underlying problem are you trying to solve with this feature?
Attempting to use the new remote builds without the bytes (#6862)


### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.
https://github.com/phb/bazel_minimal_missingrunfiles
and specifically see 
https://github.com/phb/bazel_minimal_missingrunfiles/blob/master/EXAMPLE_OUTPUT

### What operating system are you running Bazel on?

macosx.

### What's the output of `bazel info release`?

Tried various versions including latest HEAD as of today.

This was initially reported in #6862 and split into it's own ticket. CC @buchgr 

Issue comments:
Thanks for filing the issue. I see what you mean now. You are indeed correct the run command is not supported (yet).
Hi @buchgr,

I just wanted to give a quick ping on this issue. Can you think of any workaround? We have a deploy target that we generally run from our CI, but we are unable to run it without turning off minimal downloads and trigger a full rebuild.


I wrote a patch that should make it work. Can you give it a try please: https://github.com/bazelbuild/bazel/pull/9298?

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
index 4f0fb92bef4be8..535bba18a467d6 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
@@ -49,7 +49,7 @@ final class RemoteActionContextProvider extends ActionContextProvider {
   private final DigestUtil digestUtil;
   @Nullable private final Path logDir;
   private final AtomicReference<SpawnRunner> fallbackRunner = new AtomicReference<>();
-  private ImmutableSet<ActionInput> topLevelOutputs = ImmutableSet.of();
+  private ImmutableSet<ActionInput> filesToDownload = ImmutableSet.of();
 
   private RemoteActionContextProvider(
       CommandEnvironment env,
@@ -103,7 +103,7 @@ public Iterable<? extends ActionContext> getActionContexts() {
               commandId,
               env.getReporter(),
               digestUtil,
-              topLevelOutputs);
+              filesToDownload);
       return ImmutableList.of(spawnCache);
     } else {
       RemoteSpawnRunner spawnRunner =
@@ -121,7 +121,7 @@ public Iterable<? extends ActionContext> getActionContexts() {
               retrier,
               digestUtil,
               logDir,
-              topLevelOutputs);
+              filesToDownload);
       return ImmutableList.of(new RemoteSpawnStrategy(env.getExecRoot(), spawnRunner));
     }
   }
@@ -171,8 +171,8 @@ AbstractRemoteActionCache getRemoteCache() {
     return cache;
   }
 
-  void setTopLevelOutputs(ImmutableSet<ActionInput> topLevelOutputs) {
-    this.topLevelOutputs = Preconditions.checkNotNull(topLevelOutputs, "topLevelOutputs");
+  void setFilesToDownload(ImmutableSet<ActionInput> topLevelOutputs) {
+    this.filesToDownload = Preconditions.checkNotNull(topLevelOutputs, "filesToDownload");
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java
index 7140b402bab535..54e60b7e4b2815 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java
@@ -20,10 +20,14 @@
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
+import com.google.devtools.build.lib.analysis.RunfilesSupport;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
@@ -316,44 +320,41 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
     }
   }
 
-  @Override
-  public void afterAnalysis(
-      CommandEnvironment env,
-      BuildRequest request,
-      BuildOptions buildOptions,
-      Iterable<ConfiguredTarget> configuredTargets,
-      ImmutableSet<AspectValue> aspects) {
-    if (remoteOutputsMode != null && remoteOutputsMode.downloadToplevelOutputsOnly()) {
-      Preconditions.checkState(actionContextProvider != null, "actionContextProvider was null");
-      // Collect all top level output artifacts of regular targets as well as aspects. This
-      // information is used by remote spawn runners to decide whether to download an artifact
-      // if --experimental_remote_download_outputs=toplevel is set
-      ImmutableSet.Builder<ActionInput> topLevelOutputsBuilder = ImmutableSet.builder();
-      for (ConfiguredTarget configuredTarget : configuredTargets) {
-        topLevelOutputsBuilder.addAll(
-            getTopLevelTargetOutputs(
-                configuredTarget, request.getTopLevelArtifactContext(), env.getCommandName()));
+  private static ImmutableList<Artifact> getRunfiles(ConfiguredTarget buildTarget) {
+    FilesToRunProvider runfilesProvider = buildTarget.getProvider(FilesToRunProvider.class);
+    if (runfilesProvider == null) {
+      return ImmutableList.of();
+    }
+    RunfilesSupport runfilesSupport = runfilesProvider.getRunfilesSupport();
+    if (runfilesSupport == null) {
+      return ImmutableList.of();
+    }
+    boolean noPruningManifestsInBazel =
+        Iterables.isEmpty(runfilesSupport.getRunfiles().getPruningManifests());
+    Preconditions.checkState(noPruningManifestsInBazel,
+        "Bazel should not have pruning manifests. This is a bug.");
+    ImmutableList.Builder<Artifact> runfilesBuilder = ImmutableList.builder();
+    for (Artifact runfile : runfilesSupport.getRunfiles().getUnconditionalArtifacts()) {
+      if (runfile.isSourceArtifact()) {
+        continue;
       }
-      actionContextProvider.setTopLevelOutputs(topLevelOutputsBuilder.build());
+      runfilesBuilder.add(runfile);
     }
+    return runfilesBuilder.build();
   }
 
-  /** Returns a list of build or test outputs produced by the configured target. */
-  private ImmutableList<ActionInput> getTopLevelTargetOutputs(
-      ConfiguredTarget configuredTarget,
-      TopLevelArtifactContext topLevelArtifactContext,
-      String commandName) {
-    if (commandName.equals("test") && isTestRule(configuredTarget)) {
-      TestProvider testProvider = configuredTarget.getProvider(TestProvider.class);
-      if (testProvider == null) {
-        return ImmutableList.of();
-      }
-      return testProvider.getTestParams().getOutputs();
-    } else {
-      return ImmutableList.copyOf(
-          TopLevelArtifactHelper.getAllArtifactsToBuild(configuredTarget, topLevelArtifactContext)
-              .getImportantArtifacts());
+  private static ImmutableList<ActionInput> getTestOutputs(ConfiguredTarget testTarget) {
+    TestProvider testProvider = testTarget.getProvider(TestProvider.class);
+    if (testProvider == null) {
+      return ImmutableList.of();
     }
+    return testProvider.getTestParams().getOutputs();
+  }
+
+  private static Iterable<? extends ActionInput> getArtifactsToBuild(ConfiguredTarget buildTarget,
+      TopLevelArtifactContext topLevelArtifactContext) {
+    return TopLevelArtifactHelper.getAllArtifactsToBuild(buildTarget, topLevelArtifactContext)
+        .getImportantArtifacts();
   }
 
   private static boolean isTestRule(ConfiguredTarget configuredTarget) {
@@ -364,6 +365,31 @@ private static boolean isTestRule(ConfiguredTarget configuredTarget) {
     return false;
   }
 
+  @Override
+  public void afterAnalysis(
+      CommandEnvironment env,
+      BuildRequest request,
+      BuildOptions buildOptions,
+      Iterable<ConfiguredTarget> configuredTargets,
+      ImmutableSet<AspectValue> aspects) {
+    if (remoteOutputsMode != null && remoteOutputsMode.downloadToplevelOutputsOnly()) {
+      Preconditions.checkState(actionContextProvider != null, "actionContextProvider was null");
+      boolean isTestCommand = env.getCommandName().equals("test");
+      TopLevelArtifactContext artifactContext = request.getTopLevelArtifactContext();
+      ImmutableSet.Builder<ActionInput> filesToDownload = ImmutableSet.builder();
+      for (ConfiguredTarget configuredTarget : configuredTargets) {
+        if (isTestCommand && isTestRule(configuredTarget)) {
+          // When running a test download the test.log and test.xml.
+          filesToDownload.addAll(getTestOutputs(configuredTarget));
+        } else {
+          filesToDownload.addAll(getArtifactsToBuild(configuredTarget, artifactContext));
+          filesToDownload.addAll(getRunfiles(configuredTarget));
+        }
+      }
+      actionContextProvider.setFilesToDownload(filesToDownload.build());
+    }
+  }
+
   private static void cleanAndCreateRemoteLogsDir(Path logDir) throws AbruptExitException {
     try {
       // Clean out old logs files.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
index d24d5d5383cd79..4d738d57b5c49e 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -16,7 +16,7 @@
 import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.google.devtools.build.lib.remote.util.Utils.createSpawnResult;
 import static com.google.devtools.build.lib.remote.util.Utils.getInMemoryOutputPath;
-import static com.google.devtools.build.lib.remote.util.Utils.hasTopLevelOutputs;
+import static com.google.devtools.build.lib.remote.util.Utils.hasFilesToDownload;
 import static com.google.devtools.build.lib.remote.util.Utils.shouldDownloadAllSpawnOutputs;
 
 import build.bazel.remote.execution.v2.Action;
@@ -85,12 +85,10 @@ final class RemoteSpawnCache implements SpawnCache {
   private final DigestUtil digestUtil;
 
   /**
-   * Set of artifacts that are top level outputs
-   *
-   * <p>This set is empty unless {@link RemoteOutputsMode#TOPLEVEL} is specified. If so, this set is
-   * used to decide whether to download an output.
+   * If {@link RemoteOutputsMode#TOPLEVEL} is specified it contains the artifacts that should be
+   * downloaded.
    */
-  private final ImmutableSet<ActionInput> topLevelOutputs;
+  private final ImmutableSet<ActionInput> filesToDownload;
 
   RemoteSpawnCache(
       Path execRoot,
@@ -100,7 +98,7 @@ final class RemoteSpawnCache implements SpawnCache {
       String commandId,
       @Nullable Reporter cmdlineReporter,
       DigestUtil digestUtil,
-      ImmutableSet<ActionInput> topLevelOutputs) {
+      ImmutableSet<ActionInput> filesToDownload) {
     this.execRoot = execRoot;
     this.options = options;
     this.remoteCache = remoteCache;
@@ -108,7 +106,7 @@ final class RemoteSpawnCache implements SpawnCache {
     this.buildRequestId = buildRequestId;
     this.commandId = commandId;
     this.digestUtil = digestUtil;
-    this.topLevelOutputs = Preconditions.checkNotNull(topLevelOutputs, "topLevelOutputs");
+    this.filesToDownload = Preconditions.checkNotNull(filesToDownload, "filesToDownload");
   }
 
   @Override
@@ -164,7 +162,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
               shouldDownloadAllSpawnOutputs(
                   remoteOutputsMode,
                   /* exitCode = */ 0,
-                  hasTopLevelOutputs(spawn.getOutputFiles(), topLevelOutputs));
+                  hasFilesToDownload(spawn.getOutputFiles(), filesToDownload));
           if (downloadOutputs) {
             try (SilentCloseable c =
                 prof.profile(ProfilerTask.REMOTE_DOWNLOAD, "download outputs")) {
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
index 8ff410f230c67d..641212257d4d31 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -20,7 +20,7 @@
 import static com.google.devtools.build.lib.remote.util.Utils.createSpawnResult;
 import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture;
 import static com.google.devtools.build.lib.remote.util.Utils.getInMemoryOutputPath;
-import static com.google.devtools.build.lib.remote.util.Utils.hasTopLevelOutputs;
+import static com.google.devtools.build.lib.remote.util.Utils.hasFilesToDownload;
 import static com.google.devtools.build.lib.remote.util.Utils.shouldDownloadAllSpawnOutputs;
 
 import build.bazel.remote.execution.v2.Action;
@@ -107,12 +107,10 @@ public class RemoteSpawnRunner implements SpawnRunner {
   private final Path logDir;
 
   /**
-   * Set of artifacts that are top level outputs
-   *
-   * <p>This set is empty unless {@link RemoteOutputsMode#TOPLEVEL} is specified. If so, this set is
-   * used to decide whether to download an output.
+   * If {@link RemoteOutputsMode#TOPLEVEL} is specified it contains the artifacts that should be
+   * downloaded.
    */
-  private final ImmutableSet<ActionInput> topLevelOutputs;
+  private final ImmutableSet<ActionInput> filesToDownload;
 
   // Used to ensure that a warning is reported only once.
   private final AtomicBoolean warningReported = new AtomicBoolean();
@@ -131,7 +129,7 @@ public class RemoteSpawnRunner implements SpawnRunner {
       @Nullable RemoteRetrier retrier,
       DigestUtil digestUtil,
       Path logDir,
-      ImmutableSet<ActionInput> topLevelOutputs) {
+      ImmutableSet<ActionInput> filesToDownload) {
     this.execRoot = execRoot;
     this.remoteOptions = remoteOptions;
     this.executionOptions = executionOptions;
@@ -145,7 +143,7 @@ public class RemoteSpawnRunner implements SpawnRunner {
     this.retrier = retrier;
     this.digestUtil = digestUtil;
     this.logDir = logDir;
-    this.topLevelOutputs = Preconditions.checkNotNull(topLevelOutputs, "topLevelOutputs");
+    this.filesToDownload = Preconditions.checkNotNull(filesToDownload, "filesToDownload");
   }
 
   @Override
@@ -300,7 +298,7 @@ private SpawnResult downloadAndFinalizeSpawnResult(
         shouldDownloadAllSpawnOutputs(
             remoteOutputsMode,
             /* exitCode = */ actionResult.getExitCode(),
-            hasTopLevelOutputs(spawn.getOutputFiles(), topLevelOutputs));
+            hasFilesToDownload(spawn.getOutputFiles(), filesToDownload));
     InMemoryOutput inMemoryOutput = null;
     if (downloadOutputs) {
       try (SilentCloseable c = Profiler.instance().profile(REMOTE_DOWNLOAD, "download outputs")) {
diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOutputsMode.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOutputsMode.java
index d1bb3490a7906b..c163fe33799f34 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOutputsMode.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOutputsMode.java
@@ -27,9 +27,9 @@ public enum RemoteOutputsMode {
   MINIMAL,
 
   /**
-   * Downloads outputs of top level targets, but generally not intermediate outputs. The only
-   * intermediate outputs to be downloaded are .d and .jdeps files for C++ and Java compilation
-   * actions.
+   * Downloads outputs of top level targets. Top level targets are targets specified on the command
+   * line. If a top level target has runfile dependencies it will also download those. Intermediate
+   * outputs are generally not downloaded (See {@link #MINIMAL}.
    */
   TOPLEVEL;
 
diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java
index 422726a62d20c4..4d824e097f0b3a 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java
@@ -101,12 +101,12 @@ public static boolean shouldDownloadAllSpawnOutputs(
   }
 
   /** Returns {@code true} if outputs contains one or more top level outputs. */
-  public static boolean hasTopLevelOutputs(
-      Collection<? extends ActionInput> outputs, ImmutableSet<ActionInput> topLevelOutputs) {
-    if (topLevelOutputs.isEmpty()) {
+  public static boolean hasFilesToDownload(
+      Collection<? extends ActionInput> outputs, ImmutableSet<ActionInput> filesToDownload) {
+    if (filesToDownload.isEmpty()) {
       return false;
     }
-    return !Collections.disjoint(outputs, topLevelOutputs);
+    return !Collections.disjoint(outputs, filesToDownload);
   }
 
   public static String grpcAwareErrorMessage(IOException e) {


Test patch:
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index 2f0e0021f01530..3fcedff8ab956c 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -61,6 +61,23 @@ function tear_down() {
   rm -rf "${cas_path}"
 }
 
+case "$(uname -s | tr [:upper:] [:lower:])" in
+msys*|mingw*|cygwin*)
+  declare -r is_windows=true
+  ;;
+*)
+  declare -r is_windows=false
+  ;;
+esac
+
+if "$is_windows"; then
+  export MSYS_NO_PATHCONV=1
+  export MSYS2_ARG_CONV_EXCL="*"
+  declare -r EXE_EXT=".exe"
+else
+  declare -r EXE_EXT=""
+fi
+
 function test_remote_grpc_cache_with_protocol() {
   # Test that if 'grpc' is provided as a scheme for --remote_cache flag, remote cache works.
   mkdir -p a
@@ -1042,6 +1059,78 @@ EOF
   || fail "Expected toplevel output bazel-bin/a/foobar.txt to be re-downloaded"
 }
 
+function test_downloads_toplevel_runfiles() {
+  # Test that --experimental_remote_download_outputs=toplevel downloads the top level binary
+  # and generated runfiles.
+  mkdir -p a
+
+  cat > a/create_bar.tmpl <<'EOF'
+#!/bin/sh
+echo "bar runfiles"
+exit 0
+EOF
+
+  cat > a/foo.cc <<'EOF'
+#include <iostream>
+int main() { std::cout << "foo" << std::endl; return 0; }
+EOF
+
+  cat > a/BUILD <<'EOF'
+genrule(
+  name = "bar",
+  srcs = ["create_bar.tmpl"],
+  outs = ["create_bar.sh"],
+  cmd = "cat $(location create_bar.tmpl) > \"$@\"",
+)
+
+cc_binary(
+  name = "foo",
+  srcs = ["foo.cc"],
+  data = [":bar"],
+)
+EOF
+
+  bazel build \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --experimental_remote_download_toplevel \
+    //a:foo || fail "Failed to build //a:foobar"
+
+  [[ -f bazel-bin/a/foo${EXE_EXT} ]] \
+  || fail "Expected toplevel output bazel-bin/a/foo${EXE_EXT} to be downloaded"
+
+  [[ -f bazel-bin/a/create_bar.sh ]] \
+  || fail "Expected runfile bazel-bin/a/create_bar.sh to be downloaded"
+}
+
+function test_downloads_toplevel_src_runfiles() {
+  # Test that using --experimental_remote_download_outputs=toplevel with a non-generated (source)
+  # runfile dependency works.
+  mkdir -p a
+  cat > a/create_foo.sh <<'EOF'
+#!/bin/sh
+echo "foo runfiles"
+exit 0
+EOF
+  chmod +x a/create_foo.sh
+  cat > a/BUILD <<'EOF'
+genrule(
+  name = "foo",
+  srcs = [],
+  tools = ["create_foo.sh"],
+  outs = ["foo.txt"],
+  cmd = "./$(location create_foo.sh) > \"$@\"",
+)
+EOF
+
+  bazel build \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --experimental_remote_download_toplevel \
+    //a:foo || fail "Failed to build //a:foobar"
+
+  [[ -f bazel-bin/a/foo.txt ]] \
+  || fail "Expected toplevel output bazel-bin/a/foo.txt to be downloaded"
+}
+
 function test_download_toplevel_test_rule() {
   # Test that when using --experimental_remote_download_outputs=toplevel with bazel test only
   # the test.log and test.xml file are downloaded but not the test binary. However when building