Use ToplevelArtifactsDownloader to download toplevel artifacts
Previously, with `--remote_download_toplevel`, Bazel only downloads toplevel outputs during spawn execution. It has a drawback that, if the toplevel targets are changed in a following invocation, but the generating actions are not able to be executed because of skyframe/action cache, the outputs of new toplevel targets are not downloaded.
`ToplevelArtifactsDownloader` fixes that issue by listening to the `TargetCompleteEvent` (which is fired every time after the toplevel target is built event if it hit the cache) and download outputs in the background. Additionally, it can listen to more events during the build hence is more flexible to define additional outputs to be downloaded as toplevel outputs.
Working towards #12665.
Fixes #13625.
Fixes #11834.
Fixes #10525.
Closes #16524.
PiperOrigin-RevId: 483368093
Change-Id: I2184cbbb1d54548498eaa6caa07055a9336fd97e
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 521d92a..6a32924 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD
@@ -46,6 +46,7 @@
":ReferenceCountedChannel",
":Retrier",
":abstract_action_input_prefetcher",
+ ":toplevel_artifacts_downloader",
"//src/main/java/com/google/devtools/build/lib:build-request-options",
"//src/main/java/com/google/devtools/build/lib:runtime",
"//src/main/java/com/google/devtools/build/lib:runtime/command_line_path_factory",
@@ -59,14 +60,12 @@
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
- "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",
"//src/main/java/com/google/devtools/build/lib/analysis/platform:platform_utils",
"//src/main/java/com/google/devtools/build/lib/authandtls",
"//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper",
"//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader",
"//src/main/java/com/google/devtools/build/lib/buildeventstream",
"//src/main/java/com/google/devtools/build/lib/clock",
- "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/exec:abstract_spawn_strategy",
@@ -79,7 +78,6 @@
"//src/main/java/com/google/devtools/build/lib/exec:spawn_runner",
"//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry",
"//src/main/java/com/google/devtools/build/lib/exec/local",
- "//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/remote/common",
@@ -200,8 +198,12 @@
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
+ "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
+ "//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value",
+ "//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/remote/util",
"//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value",
+ "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key",
"//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/vfs",
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 c594d15..8666631 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
@@ -17,9 +17,7 @@
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableSet;
import com.google.common.util.concurrent.ListeningScheduledExecutorService;
-import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.exec.ExecutionOptions;
import com.google.devtools.build.lib.exec.ModuleActionContextRegistry;
import com.google.devtools.build.lib.exec.SpawnCache;
@@ -47,7 +45,6 @@
@Nullable private final ListeningScheduledExecutorService retryScheduler;
private final DigestUtil digestUtil;
@Nullable private final Path logDir;
- private ImmutableSet<ActionInput> filesToDownload = ImmutableSet.of();
private TempPathGenerator tempPathGenerator;
private RemoteExecutionService remoteExecutionService;
@Nullable private RemoteActionInputFetcher actionInputFetcher;
@@ -157,7 +154,6 @@
checkNotNull(env.getOptions().getOptions(RemoteOptions.class)),
remoteCache,
remoteExecutor,
- filesToDownload,
tempPathGenerator,
captureCorruptedOutputsDir);
env.getEventBus().register(remoteExecutionService);
@@ -213,15 +209,16 @@
return remoteExecutor;
}
- void setFilesToDownload(ImmutableSet<ActionInput> topLevelOutputs) {
- this.filesToDownload = Preconditions.checkNotNull(topLevelOutputs, "filesToDownload");
- }
-
void setTempPathGenerator(TempPathGenerator tempPathGenerator) {
this.tempPathGenerator = tempPathGenerator;
}
public void afterCommand() {
+ // actionInputFetcher uses remoteCache to prefetch inputs, so must shut it down before
+ // remoteCache.
+ if (actionInputFetcher != null) {
+ actionInputFetcher.shutdown();
+ }
if (remoteExecutionService != null) {
remoteExecutionService.shutdown();
} else {
@@ -232,9 +229,5 @@
remoteExecutor.close();
}
}
-
- if (actionInputFetcher != null) {
- actionInputFetcher.shutdown();
- }
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
index 80d22d7..4c35261 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
@@ -508,7 +508,7 @@
}
@Nullable
- private RemoteFileArtifactValue getRemoteMetadata(PathFragment path) {
+ protected RemoteFileArtifactValue getRemoteMetadata(PathFragment path) {
if (!isOutput(path)) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
index 196247f..149339b 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -25,8 +25,6 @@
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.grpcAwareErrorMessage;
-import static com.google.devtools.build.lib.remote.util.Utils.hasFilesToDownload;
-import static com.google.devtools.build.lib.remote.util.Utils.shouldDownloadAllSpawnOutputs;
import static com.google.devtools.build.lib.remote.util.Utils.shouldUploadLocalResultsToRemoteCache;
import static com.google.devtools.build.lib.remote.util.Utils.waitForBulkTransfer;
import static com.google.devtools.build.lib.util.StringUtil.decodeBytestringUtf8;
@@ -163,7 +161,6 @@
private final RemoteOptions remoteOptions;
@Nullable private final RemoteCache remoteCache;
@Nullable private final RemoteExecutionClient remoteExecutor;
- private final ImmutableSet<PathFragment> filesToDownload;
private final TempPathGenerator tempPathGenerator;
@Nullable private final Path captureCorruptedOutputsDir;
private final Cache<Object, MerkleTree> merkleTreeCache;
@@ -187,7 +184,6 @@
RemoteOptions remoteOptions,
@Nullable RemoteCache remoteCache,
@Nullable RemoteExecutionClient remoteExecutor,
- ImmutableSet<ActionInput> filesToDownload,
TempPathGenerator tempPathGenerator,
@Nullable Path captureCorruptedOutputsDir) {
this.reporter = reporter;
@@ -208,11 +204,6 @@
}
this.merkleTreeCache = merkleTreeCacheBuilder.build();
- ImmutableSet.Builder<PathFragment> filesToDownloadBuilder = ImmutableSet.builder();
- for (ActionInput actionInput : filesToDownload) {
- filesToDownloadBuilder.add(actionInput.getExecPath());
- }
- this.filesToDownload = filesToDownloadBuilder.build();
this.tempPathGenerator = tempPathGenerator;
this.captureCorruptedOutputsDir = captureCorruptedOutputsDir;
@@ -1019,10 +1010,11 @@
ImmutableList.builder();
RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
boolean downloadOutputs =
- shouldDownloadAllSpawnOutputs(
- remoteOutputsMode,
- /* exitCode = */ result.getExitCode(),
- hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload));
+ remoteOutputsMode.downloadAllOutputs()
+ ||
+ // In case the action failed, download all outputs. It might be helpful for debugging
+ // and there is no point in injecting output metadata of a failed action.
+ result.getExitCode() != 0;
// Download into temporary paths, then move everything at the end.
// This avoids holding the output lock while downloading, which would prevent the local branch
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 f4194a2..7f78d87 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
@@ -27,26 +27,17 @@
import com.google.common.base.Strings;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.GoogleLogger;
import com.google.common.util.concurrent.ListeningScheduledExecutorService;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
-import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
-import com.google.devtools.build.lib.actions.ActionGraph;
-import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.AnalysisResult;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
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;
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.CallCredentialsProvider;
import com.google.devtools.build.lib.authandtls.GoogleAuthUtils;
@@ -57,14 +48,12 @@
import com.google.devtools.build.lib.buildeventstream.LocalFilesArtifactUploader;
import com.google.devtools.build.lib.buildtool.BuildRequest;
import com.google.devtools.build.lib.buildtool.BuildRequestOptions;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.exec.ExecutionOptions;
import com.google.devtools.build.lib.exec.ExecutorBuilder;
import com.google.devtools.build.lib.exec.ModuleActionContextRegistry;
import com.google.devtools.build.lib.exec.SpawnStrategyRegistry;
-import com.google.devtools.build.lib.packages.TargetUtils;
import com.google.devtools.build.lib.remote.RemoteServerCapabilities.ServerCapabilitiesRequirement;
import com.google.devtools.build.lib.remote.common.RemoteCacheClient;
import com.google.devtools.build.lib.remote.common.RemoteExecutionClient;
@@ -111,10 +100,8 @@
import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
-import java.util.HashSet;
import java.util.List;
import java.util.Optional;
-import java.util.Set;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.LinkedBlockingQueue;
@@ -140,6 +127,7 @@
@Nullable private ExecutorService executorService;
@Nullable private RemoteActionContextProvider actionContextProvider;
@Nullable private RemoteActionInputFetcher actionInputFetcher;
+ @Nullable private ToplevelArtifactsDownloader toplevelArtifactsDownloader;
@Nullable private RemoteOptions remoteOptions;
@Nullable private RemoteOutputService remoteOutputService;
@Nullable private TempPathGenerator tempPathGenerator;
@@ -715,77 +703,12 @@
remoteExecutionCode));
}
- 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();
- }
- ImmutableList.Builder<Artifact> runfilesBuilder = ImmutableList.builder();
- for (Artifact runfile : runfilesSupport.getRunfiles().getArtifacts().toList()) {
- 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 NestedSet<Artifact> 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,
BuildRequest request,
BuildOptions buildOptions,
AnalysisResult analysisResult) {
- // The actionContextProvider may be null if remote execution is disabled or if there was an
- // error during initialization.
- if (remoteOptions != null
- && remoteOptions.remoteOutputsMode.downloadToplevelOutputsOnly()
- && actionContextProvider != null) {
- boolean isTestCommand = env.getCommandName().equals("test");
- TopLevelArtifactContext artifactContext = request.getTopLevelArtifactContext();
- Set<ActionInput> filesToDownload = new HashSet<>();
- for (ConfiguredTarget configuredTarget : analysisResult.getTargetsToBuild()) {
- if (isTestCommand && isTestRule(configuredTarget)) {
- // When running a test download the test.log and test.xml. These are never symlinks.
- filesToDownload.addAll(getTestOutputs(configuredTarget));
- } else {
- fetchSymlinkDependenciesRecursively(
- analysisResult.getActionGraph(),
- filesToDownload,
- getArtifactsToBuild(configuredTarget, artifactContext).toList());
- fetchSymlinkDependenciesRecursively(
- analysisResult.getActionGraph(), filesToDownload, getRunfiles(configuredTarget));
- }
- }
- actionContextProvider.setFilesToDownload(ImmutableSet.copyOf(filesToDownload));
- }
-
if (remoteOptions != null
&& remoteOptions.remoteBuildEventUploadMode == RemoteBuildEventUploadMode.ALL
&& remoteOptions.incompatibleRemoteBuildEventUploadRespectNoCache) {
@@ -826,44 +749,6 @@
}
}
- // This is a short-term fix for top-level outputs that are symlinks. Unfortunately, we cannot
- // reliably tell after analysis whether actions will create symlinks (the RE protocol allows any
- // action to generate and return symlinks), but at least we can handle basic C++ rules with this
- // change.
- // TODO(ulfjack): I think we should separate downloading files from action execution. That would
- // also resolve issues around action invalidation - we currently invalidate actions to trigger
- // downloads of top-level outputs when the top-level targets change.
- private static void fetchSymlinkDependenciesRecursively(
- ActionGraph actionGraph, Set<ActionInput> builder, List<Artifact> inputs) {
- for (Artifact input : inputs) {
- // Only fetch recursively if we don't have the file to avoid visiting nodes multiple times.
- if (builder.add(input)) {
- fetchSymlinkDependenciesRecursively(actionGraph, builder, input);
- }
- }
- }
-
- private static void fetchSymlinkDependenciesRecursively(
- ActionGraph actionGraph, Set<ActionInput> builder, Artifact artifact) {
- if (!(actionGraph.getGeneratingAction(artifact) instanceof ActionExecutionMetadata)) {
- // The top-level artifact could be a tree artifact, in which case the generating action may
- // be an ActionTemplate, which does not implement ActionExecutionMetadata. We don't handle
- // this case right now, so exit.
- return;
- }
- ActionExecutionMetadata action =
- (ActionExecutionMetadata) actionGraph.getGeneratingAction(artifact);
- if (action.mayInsensitivelyPropagateInputs()) {
- List<Artifact> inputs = action.getInputs().toList();
- if (inputs.size() > 5) {
- logger.atWarning().log(
- "Action with a lot of inputs insensitively propagates them; this could be performance"
- + " problem");
- }
- fetchSymlinkDependenciesRecursively(actionGraph, builder, inputs);
- }
- }
-
private static void cleanAndCreateRemoteLogsDir(Path logDir) throws AbruptExitException {
try {
// Clean out old logs files.
@@ -921,6 +806,7 @@
remoteDownloaderSupplier.set(null);
actionContextProvider = null;
actionInputFetcher = null;
+ toplevelArtifactsDownloader = null;
remoteOptions = null;
remoteOutputService = null;
tempPathGenerator = null;
@@ -1042,6 +928,23 @@
builder.setActionInputPrefetcher(actionInputFetcher);
remoteOutputService.setActionInputFetcher(actionInputFetcher);
actionContextProvider.setActionInputFetcher(actionInputFetcher);
+
+ if (remoteOutputsMode.downloadToplevelOutputsOnly()) {
+ toplevelArtifactsDownloader =
+ new ToplevelArtifactsDownloader(
+ env.getCommandName(),
+ env.getSkyframeExecutor().getEvaluator(),
+ actionInputFetcher,
+ (path) -> {
+ FileSystem fileSystem = path.getFileSystem();
+ Preconditions.checkState(
+ fileSystem instanceof RemoteActionFileSystem,
+ "fileSystem must be an instance of RemoteActionFileSystem");
+ return ((RemoteActionFileSystem) path.getFileSystem())
+ .getRemoteMetadata(path.asFragment());
+ });
+ env.getEventBus().register(toplevelArtifactsDownloader);
+ }
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java
index 9b2db1c..1136569 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java
@@ -17,6 +17,7 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.util.concurrent.Futures.addCallback;
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
+import static com.google.devtools.build.lib.packages.TargetUtils.isTestRuleName;
import com.google.common.collect.ImmutableMap;
import com.google.common.eventbus.AllowConcurrentEvents;
@@ -28,13 +29,17 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.analysis.AspectCompleteEvent;
+import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
import com.google.devtools.build.lib.analysis.Runfiles;
import com.google.devtools.build.lib.analysis.TargetCompleteEvent;
import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
+import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.analysis.test.TestAttempt;
import com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher.Priority;
import com.google.devtools.build.lib.remote.util.StaticMetadataProvider;
import com.google.devtools.build.lib.skyframe.ActionExecutionValue;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.Path;
@@ -47,16 +52,38 @@
* in the background.
*/
public class ToplevelArtifactsDownloader {
+ private enum CommandMode {
+ UNKNOWN,
+ BUILD,
+ TEST,
+ RUN;
+ }
+
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
+ private final CommandMode commandMode;
private final MemoizingEvaluator memoizingEvaluator;
private final AbstractActionInputPrefetcher actionInputPrefetcher;
private final PathToMetadataConverter pathToMetadataConverter;
public ToplevelArtifactsDownloader(
+ String commandName,
MemoizingEvaluator memoizingEvaluator,
AbstractActionInputPrefetcher actionInputPrefetcher,
PathToMetadataConverter pathToMetadataConverter) {
+ switch (commandName) {
+ case "build":
+ this.commandMode = CommandMode.BUILD;
+ break;
+ case "test":
+ this.commandMode = CommandMode.TEST;
+ break;
+ case "run":
+ this.commandMode = CommandMode.RUN;
+ break;
+ default:
+ this.commandMode = CommandMode.UNKNOWN;
+ }
this.memoizingEvaluator = memoizingEvaluator;
this.actionInputPrefetcher = actionInputPrefetcher;
this.pathToMetadataConverter = pathToMetadataConverter;
@@ -116,6 +143,10 @@
@Subscribe
@AllowConcurrentEvents
public void onTargetComplete(TargetCompleteEvent event) {
+ if (!shouldDownloadToplevelOutputsForTarget(event.getConfiguredTargetKey())) {
+ return;
+ }
+
if (event.failed()) {
return;
}
@@ -125,6 +156,31 @@
event.getExecutableTargetData().getRunfiles());
}
+ private boolean shouldDownloadToplevelOutputsForTarget(ConfiguredTargetKey configuredTargetKey) {
+ if (commandMode != CommandMode.TEST) {
+ return true;
+ }
+
+ // Do not download test binary in test mode.
+ try {
+ var configuredTargetValue =
+ (ConfiguredTargetValue) memoizingEvaluator.getExistingValue(configuredTargetKey);
+ if (configuredTargetValue == null) {
+ return false;
+ }
+ ConfiguredTarget configuredTarget = configuredTargetValue.getConfiguredTarget();
+ if (configuredTarget instanceof RuleConfiguredTarget) {
+ var ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
+ var isTestRule = isTestRuleName(ruleConfiguredTarget.getRuleClassString());
+ return !isTestRule;
+ }
+ return true;
+ } catch (InterruptedException ignored) {
+ Thread.currentThread().interrupt();
+ return false;
+ }
+ }
+
private void downloadTargetOutputs(
ImmutableMap<String, ArtifactsInOutputGroup> outputGroups, @Nullable Runfiles runfiles) {
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 1bc8d3d..95b85d6 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
@@ -25,7 +25,6 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
import com.google.common.util.concurrent.AsyncCallable;
import com.google.common.util.concurrent.FluentFuture;
import com.google.common.util.concurrent.Futures;
@@ -45,7 +44,6 @@
import com.google.devtools.build.lib.remote.common.OutputDigestMismatchException;
import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
import com.google.devtools.build.lib.remote.options.RemoteOptions;
-import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -73,7 +71,6 @@
import java.text.DecimalFormatSymbols;
import java.time.Instant;
import java.util.Arrays;
-import java.util.Collection;
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
@@ -190,37 +187,6 @@
return Instant.ofEpochSecond(timestamp.getSeconds(), timestamp.getNanos());
}
- /** Returns {@code true} if all spawn outputs should be downloaded to disk. */
- public static boolean shouldDownloadAllSpawnOutputs(
- RemoteOutputsMode remoteOutputsMode, int exitCode, boolean hasTopLevelOutputs) {
- return remoteOutputsMode.downloadAllOutputs()
- ||
- // In case the action failed, download all outputs. It might be helpful for debugging
- // and there is no point in injecting output metadata of a failed action.
- exitCode != 0
- ||
- // If one output of a spawn is a top level output then download all outputs. Spawns
- // are typically structured in a way that either all or no outputs are top level and
- // it's much simpler to implement under this assumption.
- (remoteOutputsMode.downloadToplevelOutputsOnly() && hasTopLevelOutputs);
- }
-
- /** Returns {@code true} if outputs contains one or more top level outputs. */
- public static boolean hasFilesToDownload(
- Collection<? extends ActionInput> outputs, ImmutableSet<PathFragment> filesToDownload) {
- if (filesToDownload.isEmpty()) {
- return false;
- }
-
- for (ActionInput output : outputs) {
- if (filesToDownload.contains(output.getExecPath())) {
- return true;
- }
- }
-
- return false;
- }
-
private static String statusName(int code) {
// 'convert_underscores' to 'Convert Underscores'
String name = Code.forNumber(code).getValueDescriptor().getName();
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
index dc69fbe..8341d8c 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
@@ -23,7 +23,6 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.Assert.assertThrows;
import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doReturn;
@@ -111,7 +110,6 @@
import com.google.protobuf.ByteString;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
-import java.util.Collection;
import java.util.Random;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
@@ -925,35 +923,6 @@
}
@Test
- public void downloadOutputs_outputFilesWithTopLevel_download() throws Exception {
- // arrange
- Digest d1 = cache.addContents(remoteActionExecutionContext, "content1");
- ActionResult r =
- ActionResult.newBuilder()
- .setExitCode(0)
- .addOutputFiles(OutputFile.newBuilder().setPath("outputs/file1").setDigest(d1))
- .build();
-
- RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(r));
- Spawn spawn = newSpawnFromResult(result);
- RemoteActionFileSystem actionFileSystem = mock(RemoteActionFileSystem.class);
- FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn, actionFileSystem);
- RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class);
- remoteOptions.remoteOutputsMode = RemoteOutputsMode.TOPLEVEL;
- RemoteExecutionService service =
- newRemoteExecutionService(remoteOptions, spawn.getOutputFiles());
- RemoteAction action = service.buildRemoteAction(spawn, context);
-
- // act
- service.downloadOutputs(action, result);
-
- // assert
- verify(actionFileSystem, never()).injectRemoteFile(any(), any(), anyLong(), any());
- assertThat(digestUtil.compute(execRoot.getRelative("outputs/file1"))).isEqualTo(d1);
- assertThat(context.isLockOutputFilesCalled()).isTrue();
- }
-
- @Test
public void downloadOutputs_outputFilesWithoutTopLevel_inject() throws Exception {
// arrange
Digest d1 = cache.addContents(remoteActionExecutionContext, "content1");
@@ -1985,11 +1954,6 @@
}
private RemoteExecutionService newRemoteExecutionService(RemoteOptions remoteOptions) {
- return newRemoteExecutionService(remoteOptions, ImmutableList.of());
- }
-
- private RemoteExecutionService newRemoteExecutionService(
- RemoteOptions remoteOptions, Collection<? extends ActionInput> topLevelOutputs) {
return new RemoteExecutionService(
directExecutor(),
reporter,
@@ -2002,7 +1966,6 @@
remoteOptions,
cache,
executor,
- ImmutableSet.copyOf(topLevelOutputs),
tempPathGenerator,
null);
}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
index fbeb680..59f8b8a 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -240,7 +240,6 @@
options,
remoteCache,
null,
- ImmutableSet.of(),
tempPathGenerator,
/* captureCorruptedOutputsDir= */ null));
return new RemoteSpawnCache(execRoot, options, /* verboseFailures=*/ true, service);
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
index 00f2dee..623f2fe 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -53,8 +53,6 @@
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.ArtifactRoot;
-import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
@@ -64,7 +62,6 @@
import com.google.devtools.build.lib.actions.SpawnMetrics;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.SpawnResult.Status;
-import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.clock.JavaClock;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -1050,7 +1047,6 @@
remoteOptions,
cache,
executor,
- ImmutableSet.of(),
tempPathGenerator,
/* captureCorruptedOutputsDir= */ null);
RemoteSpawnRunner runner =
@@ -1188,37 +1184,6 @@
}
@Test
- public void testDownloadTopLevel() throws Exception {
- // arrange
- RemoteOptions options = Options.getDefaults(RemoteOptions.class);
- options.remoteOutputsMode = RemoteOutputsMode.TOPLEVEL;
-
- ArtifactRoot outputRoot = ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, "outs");
- Artifact topLevelOutput =
- ActionsTestUtil.createArtifact(outputRoot, outputRoot.getRoot().getRelative("foo.bin"));
-
- RemoteActionResult cachedActionResult =
- RemoteActionResult.createFromCache(
- CachedActionResult.remote(ActionResult.newBuilder().setExitCode(0).build()));
-
- RemoteSpawnRunner runner = newSpawnRunner(ImmutableSet.of(topLevelOutput));
- RemoteExecutionService service = runner.getRemoteExecutionService();
- doReturn(cachedActionResult).when(service).lookupCache(any());
- doReturn(null).when(service).downloadOutputs(any(), any());
-
- Spawn spawn = newSimpleSpawn(topLevelOutput);
- FakeSpawnExecutionContext policy = getSpawnContext(spawn);
-
- // act
- SpawnResult result = runner.exec(spawn, policy);
- assertThat(result.exitCode()).isEqualTo(0);
- assertThat(result.status()).isEqualTo(Status.SUCCESS);
-
- // assert
- verify(service).downloadOutputs(any(), eq(cachedActionResult));
- }
-
- @Test
public void testDigest() throws Exception {
RemoteSpawnRunner runner = newSpawnRunner();
RemoteExecutionService service = runner.getRemoteExecutionService();
@@ -1595,21 +1560,15 @@
private RemoteSpawnRunner newSpawnRunner() {
return newSpawnRunner(
executor,
- /*topLevelOutputs=*/ ImmutableSet.of(),
RemotePathResolver.createDefault(execRoot));
}
- private RemoteSpawnRunner newSpawnRunner(ImmutableSet<ActionInput> topLevelOutputs) {
- return newSpawnRunner(executor, topLevelOutputs, RemotePathResolver.createDefault(execRoot));
- }
-
private RemoteSpawnRunner newSpawnRunner(RemotePathResolver remotePathResolver) {
- return newSpawnRunner(executor, /*topLevelOutputs=*/ ImmutableSet.of(), remotePathResolver);
+ return newSpawnRunner(executor, remotePathResolver);
}
private RemoteSpawnRunner newSpawnRunner(
@Nullable RemoteExecutionClient executor,
- ImmutableSet<ActionInput> topLevelOutputs,
RemotePathResolver remotePathResolver) {
RemoteExecutionService service =
spy(
@@ -1625,7 +1584,6 @@
remoteOptions,
cache,
executor,
- topLevelOutputs,
tempPathGenerator,
/*captureCorruptedOutputsDir=*/ null));
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java
index 7be0513..7088859 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java
@@ -318,7 +318,6 @@
remoteOptions,
remoteCache,
executor,
- ImmutableSet.of(),
tempPathGenerator,
/* captureCorruptedOutputsDir= */ null);
client =
diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
index 9a8d14f..32494fd 100755
--- a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
+++ b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
@@ -212,7 +212,7 @@
|| fail "Expected toplevel output bazel-bin/a/foobar.txt to be downloaded"
- # Delete the file to test that the action is re-run
+ # Delete the file to test that the toplevel output can be re-downloaded
rm -f bazel-bin/a/foobar.txt
bazel build \
@@ -221,12 +221,54 @@
--remote_download_toplevel \
//a:foobar >& $TEST_log || fail "Failed to build //a:foobar"
- expect_log "2 processes: 1 remote cache hit, 1 internal"
+ expect_log "1 process: 1 internal"
[[ -f bazel-bin/a/foobar.txt ]] \
|| fail "Expected toplevel output bazel-bin/a/foobar.txt to be re-downloaded"
}
+function test_downloads_toplevel_change_toplevel_targets() {
+ # Test that if a second invocation changes toplevel targets, the outputs of
+ # new target will be downloaded even if we hit a skyframe cache.
+ mkdir -p a
+ cat > a/BUILD <<'EOF'
+genrule(
+ name = "foo",
+ srcs = [],
+ outs = ["foo.txt"],
+ cmd = "echo \"foo\" > \"$@\"",
+)
+
+genrule(
+ name = "foobar",
+ srcs = [":foo"],
+ outs = ["foobar.txt"],
+ cmd = "cat $(location :foo) > \"$@\" && echo \"bar\" >> \"$@\"",
+)
+EOF
+
+ bazel build \
+ --remote_executor=grpc://localhost:${worker_port} \
+ --remote_download_toplevel \
+ //a:foobar >& $TEST_log || fail "Failed to build //a:foobar"
+
+ (! [[ -f bazel-bin/a/foo.txt ]]) \
+ || fail "Expected intermediate output bazel-bin/a/foo.txt to not be downloaded"
+
+ [[ -f bazel-bin/a/foobar.txt ]] \
+ || fail "Expected toplevel output bazel-bin/a/foobar.txt to be downloaded"
+
+ bazel build \
+ --remote_executor=grpc://localhost:${worker_port} \
+ --remote_download_toplevel \
+ //a:foo >& $TEST_log || fail "Failed to build //a:foobar"
+
+ expect_log "1 process: 1 internal"
+
+ [[ -f bazel-bin/a/foo.txt ]] \
+ || fail "Expected toplevel output bazel-bin/a/foo.txt to be downloaded"
+}
+
function test_downloads_toplevel_runfiles() {
# Test that --remote_download_toplevel fetches only the top level binaries
# and generated runfiles.
@@ -465,8 +507,6 @@
[[ -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} \
