Skip to content

Commit dce6ed7

Browse files
coeuvrecopybara-github
coeuvre
authored and
copybara-github
committed
Make bazel run works with minimal mode
Always use `ToplevelArtifactsDownloader` when building without the bytes. It checks the combination of current command (e.g. `build`, `run`, etc.) and download mode (e.g. `toplevel`, `minimal`) to decide whether download outputs for the toplevel targets or not. Also in the `RunCommand`, we wait for the background downloads before checking the local filesystem. Fixes bazelbuild#11920. Closes bazelbuild#16545. PiperOrigin-RevId: 487181884 Change-Id: I6b1a78a0d032d2cac8093eecf9c4d2e76f90380f
1 parent 3e1b5fc commit dce6ed7

File tree

8 files changed

+146
-39
lines changed

8 files changed

+146
-39
lines changed

‎src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -564,4 +564,8 @@ public void finalizeAction(Action action, MetadataHandler metadataHandler) {
564564
prefetchFiles(outputsToDownload, metadataHandler, Priority.LOW);
565565
}
566566
}
567+
568+
public void flushOutputTree() throws InterruptedException {
569+
downloadCache.awaitInProgressTasks();
570+
}
567571
}

‎src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java‎

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -956,22 +956,21 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
956956
remoteOutputService.setActionInputFetcher(actionInputFetcher);
957957
actionContextProvider.setActionInputFetcher(actionInputFetcher);
958958

959-
if (remoteOutputsMode.downloadToplevelOutputsOnly()) {
960-
toplevelArtifactsDownloader =
961-
new ToplevelArtifactsDownloader(
962-
env.getCommandName(),
963-
env.getSkyframeExecutor().getEvaluator(),
964-
actionInputFetcher,
965-
(path) -> {
966-
FileSystem fileSystem = path.getFileSystem();
967-
Preconditions.checkState(
968-
fileSystem instanceof RemoteActionFileSystem,
969-
"fileSystem must be an instance of RemoteActionFileSystem");
970-
return ((RemoteActionFileSystem) path.getFileSystem())
971-
.getRemoteMetadata(path.asFragment());
972-
});
973-
env.getEventBus().register(toplevelArtifactsDownloader);
974-
}
959+
toplevelArtifactsDownloader =
960+
new ToplevelArtifactsDownloader(
961+
env.getCommandName(),
962+
remoteOutputsMode.downloadToplevelOutputsOnly(),
963+
env.getSkyframeExecutor().getEvaluator(),
964+
actionInputFetcher,
965+
(path) -> {
966+
FileSystem fileSystem = path.getFileSystem();
967+
Preconditions.checkState(
968+
fileSystem instanceof RemoteActionFileSystem,
969+
"fileSystem must be an instance of RemoteActionFileSystem");
970+
return ((RemoteActionFileSystem) path.getFileSystem())
971+
.getRemoteMetadata(path.asFragment());
972+
});
973+
env.getEventBus().register(toplevelArtifactsDownloader);
975974
}
976975
}
977976

‎src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,13 @@ public ModifiedFileSet startBuild(
9595
return ModifiedFileSet.EVERYTHING_MODIFIED;
9696
}
9797

98+
@Override
99+
public void flushOutputTree() throws InterruptedException {
100+
if (actionInputFetcher != null) {
101+
actionInputFetcher.flushOutputTree();
102+
}
103+
}
104+
98105
@Override
99106
public void finalizeBuild(boolean buildSuccessful) {
100107
// Intentionally left empty.

‎src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java‎

Lines changed: 34 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,14 @@ private enum CommandMode {
6262
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
6363

6464
private final CommandMode commandMode;
65+
private final boolean downloadToplevel;
6566
private final MemoizingEvaluator memoizingEvaluator;
6667
private final AbstractActionInputPrefetcher actionInputPrefetcher;
6768
private final PathToMetadataConverter pathToMetadataConverter;
6869

6970
public ToplevelArtifactsDownloader(
7071
String commandName,
72+
boolean downloadToplevel,
7173
MemoizingEvaluator memoizingEvaluator,
7274
AbstractActionInputPrefetcher actionInputPrefetcher,
7375
PathToMetadataConverter pathToMetadataConverter) {
@@ -84,6 +86,7 @@ public ToplevelArtifactsDownloader(
8486
default:
8587
this.commandMode = CommandMode.UNKNOWN;
8688
}
89+
this.downloadToplevel = downloadToplevel;
8790
this.memoizingEvaluator = memoizingEvaluator;
8891
this.actionInputPrefetcher = actionInputPrefetcher;
8992
this.pathToMetadataConverter = pathToMetadataConverter;
@@ -133,6 +136,10 @@ public void onFailure(Throwable throwable) {
133136
@Subscribe
134137
@AllowConcurrentEvents
135138
public void onAspectComplete(AspectCompleteEvent event) {
139+
if (!shouldDownloadToplevelOutputs(event.getAspectKey().getBaseConfiguredTargetKey())) {
140+
return;
141+
}
142+
136143
if (event.failed()) {
137144
return;
138145
}
@@ -143,7 +150,7 @@ public void onAspectComplete(AspectCompleteEvent event) {
143150
@Subscribe
144151
@AllowConcurrentEvents
145152
public void onTargetComplete(TargetCompleteEvent event) {
146-
if (!shouldDownloadToplevelOutputsForTarget(event.getConfiguredTargetKey())) {
153+
if (!shouldDownloadToplevelOutputs(event.getConfiguredTargetKey())) {
147154
return;
148155
}
149156

@@ -156,28 +163,32 @@ public void onTargetComplete(TargetCompleteEvent event) {
156163
event.getExecutableTargetData().getRunfiles());
157164
}
158165

159-
private boolean shouldDownloadToplevelOutputsForTarget(ConfiguredTargetKey configuredTargetKey) {
160-
if (commandMode != CommandMode.TEST) {
161-
return true;
162-
}
163-
164-
// Do not download test binary in test mode.
165-
try {
166-
var configuredTargetValue =
167-
(ConfiguredTargetValue) memoizingEvaluator.getExistingValue(configuredTargetKey);
168-
if (configuredTargetValue == null) {
169-
return false;
170-
}
171-
ConfiguredTarget configuredTarget = configuredTargetValue.getConfiguredTarget();
172-
if (configuredTarget instanceof RuleConfiguredTarget) {
173-
var ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
174-
var isTestRule = isTestRuleName(ruleConfiguredTarget.getRuleClassString());
175-
return !isTestRule;
176-
}
177-
return true;
178-
} catch (InterruptedException ignored) {
179-
Thread.currentThread().interrupt();
180-
return false;
166+
private boolean shouldDownloadToplevelOutputs(ConfiguredTargetKey configuredTargetKey) {
167+
switch (commandMode) {
168+
case RUN:
169+
// Always download outputs of toplevel targets in RUN mode
170+
return true;
171+
case TEST:
172+
// Do not download test binary in test mode.
173+
try {
174+
var configuredTargetValue =
175+
(ConfiguredTargetValue) memoizingEvaluator.getExistingValue(configuredTargetKey);
176+
if (configuredTargetValue == null) {
177+
return false;
178+
}
179+
ConfiguredTarget configuredTarget = configuredTargetValue.getConfiguredTarget();
180+
if (configuredTarget instanceof RuleConfiguredTarget) {
181+
var ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
182+
var isTestRule = isTestRuleName(ruleConfiguredTarget.getRuleClassString());
183+
return !isTestRule;
184+
}
185+
return true;
186+
} catch (InterruptedException ignored) {
187+
Thread.currentThread().interrupt();
188+
return false;
189+
}
190+
default:
191+
return downloadToplevel;
181192
}
182193
}
183194

‎src/main/java/com/google/devtools/build/lib/remote/util/AsyncTaskCache.java‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,12 @@
2121
import io.reactivex.rxjava3.annotations.NonNull;
2222
import io.reactivex.rxjava3.core.Completable;
2323
import io.reactivex.rxjava3.core.CompletableEmitter;
24+
import io.reactivex.rxjava3.core.Flowable;
2425
import io.reactivex.rxjava3.core.Single;
2526
import io.reactivex.rxjava3.core.SingleObserver;
2627
import io.reactivex.rxjava3.disposables.Disposable;
2728
import io.reactivex.rxjava3.functions.Action;
29+
import io.reactivex.rxjava3.subjects.AsyncSubject;
2830
import java.util.ArrayList;
2931
import java.util.HashMap;
3032
import java.util.List;
@@ -131,6 +133,8 @@ class Execution extends Single<ValueT> implements SingleObserver<ValueT> {
131133
@GuardedBy("lock")
132134
private final List<SingleObserver<? super ValueT>> observers = new ArrayList<>();
133135

136+
private final AsyncSubject<ValueT> completion = AsyncSubject.create();
137+
134138
Execution(KeyT key, Single<ValueT> upstream) {
135139
this.key = key;
136140
this.upstream = upstream;
@@ -182,6 +186,9 @@ public void onSuccess(@NonNull ValueT value) {
182186
observer.onSuccess(value);
183187
}
184188

189+
completion.onNext(value);
190+
completion.onComplete();
191+
185192
maybeNotifyTermination();
186193
}
187194
}
@@ -198,6 +205,8 @@ public void onError(@NonNull Throwable error) {
198205
observer.onError(error);
199206
}
200207

208+
completion.onError(error);
209+
201210
maybeNotifyTermination();
202211
}
203212
}
@@ -348,6 +357,39 @@ public void shutdown() {
348357
}
349358
}
350359

360+
/**
361+
* Waits for the in-progress tasks to finish. Any tasks that are submitted after the call are not
362+
* waited.
363+
*/
364+
public void awaitInProgressTasks() throws InterruptedException {
365+
Completable completable =
366+
Completable.defer(
367+
() -> {
368+
ImmutableList<Execution> executions;
369+
synchronized (lock) {
370+
executions = ImmutableList.copyOf(inProgress.values());
371+
}
372+
373+
if (executions.isEmpty()) {
374+
return Completable.complete();
375+
}
376+
377+
return Completable.fromPublisher(
378+
Flowable.fromIterable(executions)
379+
.flatMapSingle(e -> Single.fromObservable(e.completion)));
380+
});
381+
382+
try {
383+
completable.blockingAwait();
384+
} catch (RuntimeException e) {
385+
Throwable cause = e.getCause();
386+
if (cause != null) {
387+
throwIfInstanceOf(cause, InterruptedException.class);
388+
}
389+
throw e;
390+
}
391+
}
392+
351393
/** Waits for the channel to become terminated. */
352394
public void awaitTermination() throws InterruptedException {
353395
Completable completable =
@@ -493,6 +535,14 @@ public void shutdown() {
493535
cache.shutdown();
494536
}
495537

538+
/**
539+
* Waits for the in-progress tasks to finish. Any tasks that are submitted after the call are
540+
* not waited.
541+
*/
542+
public void awaitInProgressTasks() throws InterruptedException {
543+
cache.awaitInProgressTasks();
544+
}
545+
496546
/** Waits for the cache to become terminated. */
497547
public void awaitTermination() throws InterruptedException {
498548
cache.awaitTermination();

‎src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,17 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
306306
return BlazeCommandResult.detailedExitCode(result.getDetailedExitCode());
307307
}
308308

309+
// If Bazel is using an output service (e.g. Build without the Bytes), the toplevel outputs
310+
// might still be downloading in the background. Flush the output tree to wait for all the
311+
// downloads complete.
312+
if (env.getOutputService() != null) {
313+
try {
314+
env.getOutputService().flushOutputTree();
315+
} catch (InterruptedException ignored) {
316+
Thread.currentThread().interrupt();
317+
}
318+
}
319+
309320
// Make sure that we have exactly 1 built target (excluding --run_under),
310321
// and that it is executable.
311322
// These checks should only fail if keepGoing is true, because we already did

‎src/main/java/com/google/devtools/build/lib/vfs/OutputService.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,9 @@ public default boolean shouldTrustRemoteArtifacts() {
117117
ModifiedFileSet startBuild(EventHandler eventHandler, UUID buildId, boolean finalizeActions)
118118
throws BuildFailedException, AbruptExitException, InterruptedException;
119119

120+
/** Flush and wait for in-progress downloads. */
121+
default void flushOutputTree() throws InterruptedException {}
122+
120123
/**
121124
* Finish the build.
122125
*

‎src/test/shell/bazel/remote/build_without_the_bytes_test.sh‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1331,4 +1331,26 @@ EOF
13311331
[[ ! -e "bazel-bin/a/liblib.jdeps" ]] || fail "bazel-bin/a/liblib.jdeps shouldn't exist"
13321332
}
13331333

1334+
function test_bazel_run_with_minimal() {
1335+
# Test that `bazel run` works in minimal mode.
1336+
mkdir -p a
1337+
1338+
cat > a/BUILD <<'EOF'
1339+
genrule(
1340+
name = 'bin',
1341+
srcs = [],
1342+
outs = ['bin.out'],
1343+
cmd = "echo 'echo bin-message' > $@",
1344+
executable = True,
1345+
)
1346+
EOF
1347+
1348+
bazel run \
1349+
--remote_executor=grpc://localhost:${worker_port} \
1350+
--remote_download_minimal \
1351+
//a:bin >& $TEST_log || fail "Failed to run //a:bin"
1352+
1353+
expect_log "bin-message"
1354+
}
1355+
13341356
run_suite "Build without the Bytes tests"

0 commit comments

Comments
 (0)