Repository: bazelbuild/bazel
Issue: feature request: A mode for --experimental_remote_download_outputs to download testlogs only
Labels: untriaged
team-Remote-Exec

### Description of the problem / feature request:

### Feature requests: what underlying problem are you trying to solve with this feature?

While setting up Bazel RBE with Envoy CI, we want the CI download less artifacts from RBE to save network bandwidth and disk, while we want to be able to inspect testlogs individually.

Currently the setup is --test_output=all and --experimental_remote_download_outputs=minimal so all test outputs are printed to console, the test log are already downloaded but not write to the disk, it would be nice bazel-testlogs is set up, and better with test.xml, so CI system can interpret them.

### What operating system are you running Bazel on?

Ubuntu 16.04

### What's the output of `bazel info release`?

`release 0.28.0`


Issue comments:
have you tried `--experimental_remote_download_outputs=toplevel`. I believe this does what you want :).  Please re-open this bug if it doesn't.
@buchgr No `toplevel` is not what I want. I don't want test binaries are downloaded because they are fat, in Envoy it is around 80GB and even larger for sanitizer builds. This caused the RBE CI slowdown. `minimal` doesn't produce anything while `toplevel` downloads too much.

Can you reopen the issue?
@lizan 

`bazel test //:foo --experimental_remote_download_outputs=toplevel` will only download test.log and test.xml. It won't download any binaries. Have you tested it and are seeing something else?
@buchgr Yes I did. Bazel downloads the binaries and it bursted CI. I had to add https://github.com/envoyproxy/envoy/blob/50fb561c23078e01efd4dc3852812efbf5ca8936/.bazelrc#L107.

I just tried now again and it downloaded binary. The command I ran:
```
bazel test -c opt --config=remote-clang --config=remote-ci --remote_instance_name=<rbe-instance>  --experimental_remote_download_outputs=toplevel //test/common/common:base64_test
```
at the commit above.
@lizan ahh I see what you mean. Thanks for the reproducer. Please note that with `minimal` it will download test.log and test.xml of failed tests but not of passed ones. I suppose this is not enough?

UPDATE: I am looking into a fix.
Yeah I'd like to setup CI Test Summary like [this](https://dev.azure.com/cncf/envoy/_build/results?buildId=6209&view=ms.vss-test-web.build-test-results-tab) without download test binaries. So it will be nice to have all `test.log` and `test.xml` that ran remotely.

I have sent out https://github.com/bazelbuild/bazel/pull/8947 to address this issue. Maybe you could try it out on envoy?
I tested it with the command line you provided and things work fine!
Thanks @buchgr I was able to verify in Envoy.

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD
index 020c2cb3755613..cb4ae3fe036b49 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD
@@ -19,6 +19,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:events",
         "//src/main/java/com/google/devtools/build/lib:io",
         "//src/main/java/com/google/devtools/build/lib:out-err",
+        "//src/main/java/com/google/devtools/build/lib:packages-internal",
         "//src/main/java/com/google/devtools/build/lib:runtime",
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib/actions",
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 a64c20bd522b13..96921b65b29348 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
@@ -19,6 +19,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionContext;
+import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ExecutionStrategy;
 import com.google.devtools.build.lib.actions.ExecutorInitException;
@@ -49,7 +50,7 @@ final class RemoteActionContextProvider extends ActionContextProvider {
   private final DigestUtil digestUtil;
   @Nullable private final Path logDir;
   private final AtomicReference<SpawnRunner> fallbackRunner = new AtomicReference<>();
-  private ImmutableSet<Artifact> topLevelOutputs = ImmutableSet.of();
+  private ImmutableSet<ActionInput> topLevelOutputs = ImmutableSet.of();
 
   private RemoteActionContextProvider(
       CommandEnvironment env,
@@ -171,7 +172,7 @@ AbstractRemoteActionCache getRemoteCache() {
     return cache;
   }
 
-  void setTopLevelOutputs(ImmutableSet<Artifact> topLevelOutputs) {
+  void setTopLevelOutputs(ImmutableSet<ActionInput> topLevelOutputs) {
     this.topLevelOutputs = Preconditions.checkNotNull(topLevelOutputs, "topLevelOutputs");
   }
 
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 5d10ceeffc2e11..f3218690ef7b5f 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
@@ -22,10 +22,14 @@
 import com.google.common.collect.ImmutableSet;
 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.TopLevelArtifactContext;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
+import com.google.devtools.build.lib.analysis.test.TestProvider;
 import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
 import com.google.devtools.build.lib.authandtls.GoogleAuthUtils;
 import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader;
@@ -34,6 +38,7 @@
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.exec.ExecutorBuilder;
+import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.remote.logging.LoggingInterceptor;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
@@ -45,6 +50,7 @@
 import com.google.devtools.build.lib.runtime.Command;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.runtime.ServerBuilder;
+import com.google.devtools.build.lib.runtime.commands.TestCommand;
 import com.google.devtools.build.lib.skyframe.AspectValue;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.util.ExitCode;
@@ -321,27 +327,42 @@ public void afterAnalysis(
       ImmutableSet<AspectValue> aspects) {
     if (remoteOutputsMode != null && remoteOutputsMode.downloadToplevelOutputsOnly()) {
       Preconditions.checkState(actionContextProvider != null, "actionContextProvider was null");
-      // TODO(buchgr): Consider only storing the action owners instead of the artifacts
       // 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<Artifact> topLevelOutputsBuilder = ImmutableSet.builder();
+      ImmutableSet.Builder<ActionInput> topLevelOutputsBuilder = ImmutableSet.builder();
       for (ConfiguredTarget configuredTarget : configuredTargets) {
-        topLevelOutputsBuilder.addAll(
-            TopLevelArtifactHelper.getAllArtifactsToBuild(
-                    configuredTarget, request.getTopLevelArtifactContext())
-                .getImportantArtifacts());
+        topLevelOutputsBuilder.addAll(getTopLevelTargetOutputs(configuredTarget,
+            request.getTopLevelArtifactContext(), env.getCommandName()));
       }
+      actionContextProvider.setTopLevelOutputs(topLevelOutputsBuilder.build());
+    }
+  }
 
-      for (AspectValue aspect : aspects) {
-        topLevelOutputsBuilder.addAll(
-            TopLevelArtifactHelper.getAllArtifactsToBuild(
-                    aspect, request.getTopLevelArtifactContext())
-                .getImportantArtifacts());
+
+  /**
+   * 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());
+    }
+  }
 
-      actionContextProvider.setTopLevelOutputs(topLevelOutputsBuilder.build());
+  private static boolean isTestRule(ConfiguredTarget configuredTarget) {
+    if (configuredTarget instanceof RuleConfiguredTarget) {
+      RuleConfiguredTarget ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
+      return TargetUtils.isTestRuleName(ruleConfiguredTarget.getRuleClassString());
     }
+    return false;
   }
 
   private static void cleanAndCreateRemoteLogsDir(Path logDir) throws AbruptExitException {
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 e711b27e2c4b93..10c7facd07e913 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
@@ -90,7 +90,7 @@ final class RemoteSpawnCache implements SpawnCache {
    * <p>This set is empty unless {@link RemoteOutputsMode#TOPLEVEL} is specified. If so, this set is
    * used to decide whether to download an output.
    */
-  private final ImmutableSet<Artifact> topLevelOutputs;
+  private final ImmutableSet<ActionInput> topLevelOutputs;
 
   RemoteSpawnCache(
       Path execRoot,
@@ -100,7 +100,7 @@ final class RemoteSpawnCache implements SpawnCache {
       String commandId,
       @Nullable Reporter cmdlineReporter,
       DigestUtil digestUtil,
-      ImmutableSet<Artifact> topLevelOutputs) {
+      ImmutableSet<ActionInput> topLevelOutputs) {
     this.execRoot = execRoot;
     this.options = options;
     this.remoteCache = remoteCache;
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 2a09f3c878f7f4..639df0a95a38e3 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
@@ -118,7 +118,7 @@ public class RemoteSpawnRunner implements SpawnRunner {
    * <p>This set is empty unless {@link RemoteOutputsMode#TOPLEVEL} is specified. If so, this set is
    * used to decide whether to download an output.
    */
-  private final ImmutableSet<Artifact> topLevelOutputs;
+  private final ImmutableSet<ActionInput> topLevelOutputs;
 
   // Used to ensure that a warning is reported only once.
   private final AtomicBoolean warningReported = new AtomicBoolean();
@@ -137,7 +137,7 @@ public class RemoteSpawnRunner implements SpawnRunner {
       @Nullable RemoteRetrier retrier,
       DigestUtil digestUtil,
       Path logDir,
-      ImmutableSet<Artifact> topLevelOutputs) {
+      ImmutableSet<ActionInput> topLevelOutputs) {
     this.execRoot = execRoot;
     this.remoteOptions = remoteOptions;
     this.executionOptions = executionOptions;
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 818a4da181d830..ab97bfa7f3f745 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
@@ -100,7 +100,7 @@ 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<Artifact> topLevelOutputs) {
+      Collection<? extends ActionInput> outputs, ImmutableSet<ActionInput> topLevelOutputs) {
     if (topLevelOutputs.isEmpty()) {
       return false;
     }


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 5de687eb85c25c..a568c7bb406d2a 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -1058,8 +1058,62 @@ EOF
 
   [[ -f bazel-bin/a/foobar.txt ]] \
   || fail "Expected toplevel output bazel-bin/a/foobar.txt to be re-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
+  # a test then the test binary should be downloaded.
+
+  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
+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
+
+  # When invoking bazel test only test.log and test.xml should be downloaded.
+  bazel test \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --experimental_inmemory_jdeps_files \
+    --experimental_inmemory_dotd_files \
+    --experimental_remote_download_outputs=toplevel \
+    //a:test >& $TEST_log || fail "Failed to test //a:test with remote execution"
+
+  (! [[ -f bazel-bin/a/test ]]) \
+  || fail "Expected test binary bazel-bin/a/test to not be downloaded"
+
+  [[ -f bazel-testlogs/a/test/test.log ]] \
+  || fail "Expected toplevel output bazel-testlogs/a/test/test.log to be downloaded"
+
+  [[ -f bazel-testlogs/a/test/test.xml ]] \
+  || fail "Expected toplevel output bazel-testlogs/a/test/test.log to be downloaded"
+
+  bazel clean
+
+  # When invoking bazel build the test binary should be downloaded.
+  bazel build \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --experimental_inmemory_jdeps_files \
+    --experimental_inmemory_dotd_files \
+    --experimental_remote_download_outputs=toplevel \
+    //a:test >& $TEST_log || fail "Failed to build //a:test with remote execution"
 
+  ([[ -f bazel-bin/a/test ]]) \
+  || fail "Expected test binary bazel-bin/a/test to be downloaded"
 }
 
 function test_downloads_minimal_bep() {