Have remote_outputs=toplevel download runfiles. Fixes #8899
This change makes it so that when building a binary remotely and
having --experimental_remote_download_toplevel set the runfiles of the
binary will also be downloaded. This applies also to building a test
target. However, when executing a test remotely with bazel test neither
the test binary nor its runfiles are downloaded. Instead Bazel fetches
the test result: test.xml and test.log
This change also enables bazel run to be used with
--experimental_remote_download_toplevel.
Closes #9298.
PiperOrigin-RevId: 266781050
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 4f0fb92..535bba1 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 @@
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 @@
commandId,
env.getReporter(),
digestUtil,
- topLevelOutputs);
+ filesToDownload);
return ImmutableList.of(spawnCache);
} else {
RemoteSpawnRunner spawnRunner =
@@ -121,7 +121,7 @@
retrier,
digestUtil,
logDir,
- topLevelOutputs);
+ filesToDownload);
return ImmutableList.of(new RemoteSpawnStrategy(env.getExecRoot(), spawnRunner));
}
}
@@ -171,8 +171,8 @@
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 7140b40..fc7c658 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,6 +320,51 @@
}
}
+ 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;
+ }
+ runfilesBuilder.add(runfile);
+ }
+ return runfilesBuilder.build();
+ }
+
+ 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) {
+ if (configuredTarget instanceof RuleConfiguredTarget) {
+ RuleConfiguredTarget ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
+ return TargetUtils.isTestRuleName(ruleConfiguredTarget.getRuleClassString());
+ }
+ return false;
+ }
+
@Override
public void afterAnalysis(
CommandEnvironment env,
@@ -325,45 +374,22 @@
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();
+ boolean isTestCommand = env.getCommandName().equals("test");
+ TopLevelArtifactContext artifactContext = request.getTopLevelArtifactContext();
+ ImmutableSet.Builder<ActionInput> filesToDownload = ImmutableSet.builder();
for (ConfiguredTarget configuredTarget : configuredTargets) {
- topLevelOutputsBuilder.addAll(
- getTopLevelTargetOutputs(
- configuredTarget, request.getTopLevelArtifactContext(), env.getCommandName()));
+ 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.setTopLevelOutputs(topLevelOutputsBuilder.build());
+ actionContextProvider.setFilesToDownload(filesToDownload.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 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 {
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 d24d5d5..4d738d5 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 @@
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 @@
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 @@
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 @@
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 8ff410f..6412122 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 @@
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 @@
@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 @@
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 @@
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 d1bb349..c163fe3 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 @@
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 422726a..4d824e0 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 @@
}
/** 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) {
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index 2f0e002..3fcedff 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 @@
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 @@
|| 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
