Skip to content

Commit 51e6c38

Browse files
Wyveraldcopybara-github
Wyverald
authored and
copybara-github
committed
Properly report repo fetch progress during main repo mapping computation
This ends up as simple as firing an event and letting UiEventHandler know that the build has started. Fixes bazelbuild#16927. Fixes bazelbuild#16338. PiperOrigin-RevId: 511177491 Change-Id: I479b116cf896731b7238410ce14f7e249da6c390
1 parent 9b26a11 commit 51e6c38

File tree

8 files changed

+127
-40
lines changed

8 files changed

+127
-40
lines changed

‎src/main/java/com/google/devtools/build/lib/buildtool/buildevent/MainRepoMappingComputationStartingEvent.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// Copyright 2023 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.google.devtools.build.lib.buildtool.buildevent;
16+
17+
/**
18+
* Event fired right before main repo mapping computation starts (between the two rounds of option
19+
* parsing).
20+
*/
21+
public class MainRepoMappingComputationStartingEvent {}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import com.google.devtools.build.lib.bugreport.Crash;
3333
import com.google.devtools.build.lib.bugreport.CrashContext;
3434
import com.google.devtools.build.lib.buildeventstream.BuildEventProtocolOptions;
35+
import com.google.devtools.build.lib.buildtool.buildevent.MainRepoMappingComputationStartingEvent;
3536
import com.google.devtools.build.lib.buildtool.buildevent.ProfilerStartedEvent;
3637
import com.google.devtools.build.lib.clock.BlazeClock;
3738
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
@@ -547,6 +548,7 @@ private BlazeCommandResult execExclusively(
547548

548549
// Compute the repo mapping of the main repo and re-parse options so that we get correct
549550
// values for label-typed options.
551+
env.getEventBus().post(new MainRepoMappingComputationStartingEvent());
550552
try {
551553
RepositoryMapping mainRepoMapping =
552554
env.getSkyframeExecutor().getMainRepoMapping(reporter);

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ enum BuildStatus {
3535
// We explicitly define a starting status, which can be used to determine what to display in
3636
// cases before the build has started.
3737
BUILD_NOT_STARTED,
38+
COMPUTING_MAIN_REPO_MAPPING,
3839
BUILD_STARTED,
3940
TARGET_PATTERN_PARSING,
4041
LOADING_COMPLETE,
@@ -77,6 +78,9 @@ synchronized void writeProgressBar(
7778
switch (buildStatus) {
7879
case BUILD_NOT_STARTED:
7980
return;
81+
case COMPUTING_MAIN_REPO_MAPPING:
82+
writeBaseProgress("Computing main repo mapping", "", terminalWriter);
83+
break;
8084
case BUILD_STARTED:
8185
writeBaseProgress("Loading", "", terminalWriter);
8286
break;
@@ -151,6 +155,11 @@ void writeLoadingAnalysisPhaseProgress(
151155
}
152156
}
153157

158+
@Override
159+
void mainRepoMappingComputationStarted() {
160+
buildStatus = BuildStatus.COMPUTING_MAIN_REPO_MAPPING;
161+
}
162+
154163
@Override
155164
void buildStarted() {
156165
buildStatus = BuildStatus.BUILD_STARTED;

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import com.google.devtools.build.lib.buildtool.buildevent.BuildCompleteEvent;
4141
import com.google.devtools.build.lib.buildtool.buildevent.BuildStartingEvent;
4242
import com.google.devtools.build.lib.buildtool.buildevent.ExecutionProgressReceiverAvailableEvent;
43+
import com.google.devtools.build.lib.buildtool.buildevent.MainRepoMappingComputationStartingEvent;
4344
import com.google.devtools.build.lib.buildtool.buildevent.TestFilteringCompleteEvent;
4445
import com.google.devtools.build.lib.clock.Clock;
4546
import com.google.devtools.build.lib.events.Event;
@@ -517,7 +518,7 @@ private void setEventKindColor(EventKind kind) throws IOException {
517518
}
518519

519520
@Subscribe
520-
public void buildStarted(BuildStartingEvent event) {
521+
public void mainRepoMappingComputationStarted(MainRepoMappingComputationStartingEvent event) {
521522
synchronized (this) {
522523
buildRunning = true;
523524
}
@@ -526,6 +527,16 @@ public void buildStarted(BuildStartingEvent event) {
526527
// As a new phase started, inform immediately.
527528
ignoreRefreshLimitOnce();
528529
refresh();
530+
startUpdateThread();
531+
}
532+
533+
@Subscribe
534+
public void buildStarted(BuildStartingEvent event) {
535+
maybeAddDate();
536+
stateTracker.buildStarted();
537+
// As a new phase started, inform immediately.
538+
ignoreRefreshLimitOnce();
539+
refresh();
529540
}
530541

531542
@Subscribe

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,11 @@ void setProgressSampleSize(int sampleSize) {
409409
this.sampleSize = Math.max(1, sampleSize);
410410
}
411411

412+
void mainRepoMappingComputationStarted() {
413+
status = "Computing main repo mapping";
414+
additionalMessage = "";
415+
}
416+
412417
void buildStarted() {
413418
status = "Loading";
414419
additionalMessage = "";

‎src/test/java/com/google/devtools/build/lib/runtime/UiEventHandlerStdOutAndStdErrTest.java‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import com.google.devtools.build.lib.buildtool.BuildResult;
2424
import com.google.devtools.build.lib.buildtool.buildevent.BuildCompleteEvent;
2525
import com.google.devtools.build.lib.buildtool.buildevent.BuildStartingEvent;
26+
import com.google.devtools.build.lib.buildtool.buildevent.MainRepoMappingComputationStartingEvent;
2627
import com.google.devtools.build.lib.events.Event;
2728
import com.google.devtools.build.lib.events.EventKind;
2829
import com.google.devtools.build.lib.testutil.ManualClock;
@@ -73,23 +74,24 @@ private void createUiEventHandler(UiOptions uiOptions) {
7374
OutErr outErr = null;
7475
switch (testedOutput) {
7576
case STDOUT:
76-
outErr = OutErr.create(/*out=*/ output, /*err=*/ mock(OutputStream.class));
77+
outErr = OutErr.create(/* out= */ output, /* err= */ mock(OutputStream.class));
7778
eventKind = EventKind.STDOUT;
7879
break;
7980
case STDERR:
80-
outErr = OutErr.create(/*out=*/ mock(OutputStream.class), /*err=*/ output);
81+
outErr = OutErr.create(/* out= */ mock(OutputStream.class), /* err= */ output);
8182
eventKind = EventKind.STDERR;
8283
break;
8384
}
8485

8586
uiEventHandler =
86-
new UiEventHandler(outErr, uiOptions, new ManualClock(), /*workspacePathFragment=*/ null);
87+
new UiEventHandler(outErr, uiOptions, new ManualClock(), /* workspacePathFragment= */ null);
88+
uiEventHandler.mainRepoMappingComputationStarted(new MainRepoMappingComputationStartingEvent());
8789
uiEventHandler.buildStarted(
8890
BuildStartingEvent.create(
8991
"outputFileSystemType",
90-
/*usesInMemoryFileSystem=*/ false,
92+
/* usesInMemoryFileSystem= */ false,
9193
mock(BuildRequest.class),
92-
/*workspace=*/ null,
94+
/* workspace= */ null,
9395
"/pwd"));
9496
}
9597

@@ -205,7 +207,7 @@ public void noChangeOnUnflushedWrite() {
205207
uiOptions.eventFilters = ImmutableList.of();
206208
createUiEventHandler(uiOptions);
207209
if (testedOutput == TestedOutput.STDERR) {
208-
assertThat(output.flushed).hasSize(1);
210+
assertThat(output.flushed).hasSize(2);
209211
output.flushed.clear();
210212
}
211213
// Unterminated strings are saved in memory and not pushed out at all.
@@ -226,9 +228,9 @@ public void buildCompleteMessageDoesntOverrideError() {
226228
uiEventHandler.handle(Event.error("Show me this!"));
227229
uiEventHandler.afterCommand(new AfterCommandEvent());
228230

229-
assertThat(output.flushed.size()).isEqualTo(4);
230-
assertThat(output.flushed.get(2)).contains("Show me this!");
231-
assertThat(output.flushed.get(3)).doesNotContain("\033[1A\033[K");
231+
assertThat(output.flushed.size()).isEqualTo(5);
232+
assertThat(output.flushed.get(3)).contains("Show me this!");
233+
assertThat(output.flushed.get(4)).doesNotContain("\033[1A\033[K");
232234
}
233235

234236
private Event output(String message) {

‎src/test/java/com/google/devtools/build/lib/runtime/UiStateTrackerTest.java‎

Lines changed: 60 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1289,59 +1289,89 @@ public void testSuffix() {
12891289
}
12901290

12911291
@Test
1292-
public void testDownloadShown() throws Exception {
1293-
// Verify that, whenever a single download is running in loading face, it is shown in the status
1294-
// bar.
1292+
public void testDownloadShown_duringLoading() throws Exception {
1293+
// Verify that, whenever a single download is running in loading phase, it is shown in the
1294+
// status bar.
12951295
ManualClock clock = new ManualClock();
1296-
clock.advanceMillis(TimeUnit.SECONDS.toMillis(1234));
1297-
UiStateTracker stateTracker = getUiStateTracker(clock, /*targetWidth=*/ 80);
1296+
clock.advance(Duration.ofSeconds(1234));
1297+
UiStateTracker stateTracker = getUiStateTracker(clock, /* targetWidth= */ 80);
12981298

12991299
URL url = new URL("http://example.org/first/dep");
13001300

13011301
stateTracker.buildStarted();
13021302
stateTracker.downloadProgress(new DownloadProgressEvent(url));
1303-
clock.advanceMillis(TimeUnit.SECONDS.toMillis(6));
1303+
clock.advance(Duration.ofSeconds(6));
13041304

1305-
LoggingTerminalWriter terminalWriter = new LoggingTerminalWriter(/*discardHighlight=*/ true);
1305+
LoggingTerminalWriter terminalWriter = new LoggingTerminalWriter(/* discardHighlight= */ true);
13061306
stateTracker.writeProgressBar(terminalWriter);
13071307
String output = terminalWriter.getTranscript();
13081308

1309-
assertWithMessage("Progress bar should contain '" + url.toString() + "', but was:\n" + output)
1310-
.that(output.contains(url.toString()))
1311-
.isTrue();
1312-
assertWithMessage("Progress bar should contain '6s', but was:\n" + output)
1313-
.that(output.contains("6s"))
1314-
.isTrue();
1309+
assertThat(output).contains(url.toString());
1310+
assertThat(output).contains("6s");
13151311

13161312
// Progress on the pending download should be reported appropriately
1317-
clock.advanceMillis(TimeUnit.SECONDS.toMillis(1));
1313+
clock.advance(Duration.ofSeconds(1));
13181314
stateTracker.downloadProgress(new DownloadProgressEvent(url, 256));
13191315

1320-
terminalWriter = new LoggingTerminalWriter(/*discardHighlight=*/ true);
1316+
terminalWriter = new LoggingTerminalWriter(/* discardHighlight= */ true);
13211317
stateTracker.writeProgressBar(terminalWriter);
13221318
output = terminalWriter.getTranscript();
13231319

1324-
assertWithMessage("Progress bar should contain '" + url.toString() + "', but was:\n" + output)
1325-
.that(output.contains(url.toString()))
1326-
.isTrue();
1327-
assertWithMessage("Progress bar should contain '7s', but was:\n" + output)
1328-
.that(output.contains("7s"))
1329-
.isTrue();
1330-
assertWithMessage("Progress bar should contain '256', but was:\n" + output)
1331-
.that(output.contains("256"))
1332-
.isTrue();
1320+
assertThat(output).contains(url.toString());
1321+
assertThat(output).contains("7s");
1322+
assertThat(output).contains("256");
13331323

13341324
// After finishing the download, it should no longer be reported.
1335-
clock.advanceMillis(TimeUnit.SECONDS.toMillis(1));
1325+
clock.advance(Duration.ofSeconds(1));
13361326
stateTracker.downloadProgress(new DownloadProgressEvent(url, 256, true));
13371327

1338-
terminalWriter = new LoggingTerminalWriter(/*discardHighlight=*/ true);
1328+
terminalWriter = new LoggingTerminalWriter(/* discardHighlight= */ true);
13391329
stateTracker.writeProgressBar(terminalWriter);
13401330
output = terminalWriter.getTranscript();
13411331

1342-
assertWithMessage("Progress bar should not contain url, but was:\n" + output)
1343-
.that(output.contains("example.org"))
1344-
.isFalse();
1332+
assertThat(output).doesNotContain("example.org");
1333+
}
1334+
1335+
@Test
1336+
public void testDownloadShown_duringMainRepoMappingComputation() throws Exception {
1337+
ManualClock clock = new ManualClock();
1338+
clock.advance(Duration.ofSeconds(1234));
1339+
UiStateTracker stateTracker = getUiStateTracker(clock, /* targetWidth= */ 80);
1340+
1341+
URL url = new URL("http://example.org/first/dep");
1342+
1343+
stateTracker.mainRepoMappingComputationStarted();
1344+
stateTracker.downloadProgress(new DownloadProgressEvent(url));
1345+
clock.advance(Duration.ofSeconds(6));
1346+
1347+
LoggingTerminalWriter terminalWriter = new LoggingTerminalWriter(/* discardHighlight= */ true);
1348+
stateTracker.writeProgressBar(terminalWriter);
1349+
String output = terminalWriter.getTranscript();
1350+
1351+
assertThat(output).contains(url.toString());
1352+
assertThat(output).contains("6s");
1353+
1354+
// Progress on the pending download should be reported appropriately
1355+
clock.advance(Duration.ofSeconds(1));
1356+
stateTracker.downloadProgress(new DownloadProgressEvent(url, 256));
1357+
1358+
terminalWriter = new LoggingTerminalWriter(/* discardHighlight= */ true);
1359+
stateTracker.writeProgressBar(terminalWriter);
1360+
output = terminalWriter.getTranscript();
1361+
1362+
assertThat(output).contains(url.toString());
1363+
assertThat(output).contains("7s");
1364+
assertThat(output).contains("256");
1365+
1366+
// After finishing the download, it should no longer be reported.
1367+
clock.advance(Duration.ofSeconds(1));
1368+
stateTracker.downloadProgress(new DownloadProgressEvent(url, 256, true));
1369+
1370+
terminalWriter = new LoggingTerminalWriter(/* discardHighlight= */ true);
1371+
stateTracker.writeProgressBar(terminalWriter);
1372+
output = terminalWriter.getTranscript();
1373+
1374+
assertThat(output).doesNotContain("example.org");
13451375
}
13461376

13471377
@Test
@@ -1350,7 +1380,7 @@ public void testDownloadOutputLength() throws Exception {
13501380
// Also verify that the length is respected, even if only a download sample is shown.
13511381
ManualClock clock = new ManualClock();
13521382
clock.advanceMillis(TimeUnit.SECONDS.toMillis(1234));
1353-
UiStateTracker stateTracker = getUiStateTracker(clock, /*targetWidth=*/ 60);
1383+
UiStateTracker stateTracker = getUiStateTracker(clock, /* targetWidth= */ 60);
13541384
URL url = new URL("http://example.org/some/really/very/very/long/path/filename.tar.gz");
13551385

13561386
stateTracker.buildStarted();

‎src/test/java/com/google/devtools/build/lib/testutil/ManualClock.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
package com.google.devtools.build.lib.testutil;
1616

1717
import com.google.devtools.build.lib.clock.Clock;
18+
import com.google.errorprone.annotations.CanIgnoreReturnValue;
19+
import java.time.Duration;
1820
import java.util.concurrent.TimeUnit;
1921
import java.util.concurrent.atomic.AtomicLong;
2022

@@ -43,4 +45,9 @@ public long nanoTime() {
4345
public long advanceMillis(long time) {
4446
return currentTimeMillis.addAndGet(time);
4547
}
48+
49+
@CanIgnoreReturnValue
50+
public long advance(Duration duration) {
51+
return advanceMillis(duration.toMillis());
52+
}
4653
}

0 commit comments

Comments
 (0)