Remove ostentatious interleaving of action output and progress messages.
I traced the use of FileOutErr to only be in SkyframeActionExecutor. The other event types generally wouldn't be expected to have stdout/stderr in any case.
Detailed changes:
* Change writeToStream to never show the progress bar, the one place it was asked to show the progress bar now does so explicitly.
* Move the display of any stdout/stderr to inside the switch cases that can have stdout/stderr in the first place, specifically before we redraw the progress bar.
* Don't flush the terminal before printing stdout/stderr, I don't see a reason to do that.
* Assert that no other events have stdout/stderr in the first place.
PiperOrigin-RevId: 343589378
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java
index 6256704..e7962a1 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java
@@ -29,6 +29,7 @@
import com.google.devtools.build.lib.analysis.AnalysisPhaseCompleteEvent;
import com.google.devtools.build.lib.analysis.NoBuildEvent;
import com.google.devtools.build.lib.analysis.NoBuildRequestFinishedEvent;
+import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.buildeventstream.AnnounceBuildEventTransportsEvent;
import com.google.devtools.build.lib.buildeventstream.BuildEventTransport;
import com.google.devtools.build.lib.buildeventstream.BuildEventTransportClosedEvent;
@@ -302,11 +303,11 @@
stream.write(event.getMessageBytes());
stream.flush();
} else {
- writeToStream(
- stream,
- event.getKind(),
- event.getMessageBytes(),
- /*addBackProgressBar=*/ showProgress && cursorControl);
+ writeToStream(stream, event.getKind(), event.getMessageBytes());
+ if (showProgress && cursorControl) {
+ addProgressBar();
+ }
+ terminal.flush();
}
break;
case FATAL:
@@ -346,6 +347,14 @@
if (incompleteLine) {
crlf();
}
+ if (stderr != null) {
+ writeToStream(outErr.getErrorStream(), EventKind.STDERR, stderr);
+ outErr.getErrorStream().flush();
+ }
+ if (stdout != null) {
+ writeToStream(outErr.getOutputStream(), EventKind.STDOUT, stdout);
+ outErr.getOutputStream().flush();
+ }
if (showProgress && buildRunning && cursorControl) {
addProgressBar();
}
@@ -355,32 +364,19 @@
if (stateTracker.progressBarTimeDependent()) {
refresh();
}
- break;
+ // Fall through.
case START:
case FINISH:
case PASS:
case TIMEOUT:
case DEPCHECKER:
+ if (stdout != null || stderr != null) {
+ BugReport.sendBugReport(
+ new IllegalStateException(
+ "stdout/stderr should not be present for this event " + event));
+ }
break;
}
- if (stdout != null || stderr != null) {
- clearProgressBar();
- terminal.flush();
- if (stderr != null) {
- writeToStream(
- outErr.getErrorStream(), EventKind.STDERR, stderr, /*addBackProgressBar=*/ false);
- outErr.getErrorStream().flush();
- }
- if (stdout != null) {
- writeToStream(
- outErr.getOutputStream(), EventKind.STDOUT, stdout, /*addBackProgressBar=*/ false);
- outErr.getOutputStream().flush();
- }
- if (showProgress && cursorControl) {
- addProgressBar();
- }
- terminal.flush();
- }
}
}
@@ -456,8 +452,7 @@
handleInternal(event);
}
- private void writeToStream(
- OutputStream stream, EventKind eventKind, byte[] message, boolean addBackProgressBar)
+ private void writeToStream(OutputStream stream, EventKind eventKind, byte[] message)
throws IOException {
int eolIndex = Bytes.lastIndexOf(message, (byte) '\n');
ByteArrayOutputStream outLineBuffer =
@@ -478,10 +473,6 @@
stream.flush();
outLineBuffer.write(message, eolIndex + 1, message.length - eolIndex - 1);
- if (addBackProgressBar) {
- addProgressBar();
- terminal.flush();
- }
}
private void setEventKindColor(EventKind kind) throws IOException {
diff --git a/src/test/shell/integration/ui_test.sh b/src/test/shell/integration/ui_test.sh
index 7185411..55b5920 100755
--- a/src/test/shell/integration/ui_test.sh
+++ b/src/test/shell/integration/ui_test.sh
@@ -592,4 +592,44 @@
done
}
+function test_interleaved_errors_and_progress() {
+ # Background process necessary to interrupt Bazel doesn't go well on Windows.
+ [[ "$is_windows" == true ]] && return
+ mkdir -p foo
+ cat > foo/BUILD <<'EOF'
+genrule(name = 'sleep', outs = ['sleep.out'], cmd = 'sleep 10000')
+genrule(name = 'fail',
+ outs = ['fail.out'],
+ srcs = [':multiline.sh'],
+ cmd = '$(location :multiline.sh)'
+)
+EOF
+ cat > foo/multiline.sh <<'EOF'
+echo "This
+is
+a
+multiline error message
+before
+failure"
+false
+EOF
+ chmod +x foo/multiline.sh
+ bazel build -k //foo:all --curses=yes >& "$TEST_log" &
+ pid="$!"
+ while ! grep -q "multiline error message" "$TEST_log" ; do
+ sleep 1
+ done
+ while ! grep -q "Executing genrule //foo:sleep" "$TEST_log" ; do
+ sleep 1
+ done
+ kill -SIGINT "$pid"
+ wait "$pid" || exit_code="$?"
+ [[ "$exit_code" == 8 ]] || fail "Should have been interrupted: $exit_code"
+ tr -s <"$TEST_log" '\n' '@' |
+ grep -q 'Executing genrule //foo:fail failed:[^@]*@This@is@a@multiline error message@before@failure@\[2 / 3\] Executing genrule //foo:sleep;' \
+ || fail "Unified genrule error message not found"
+ # Make sure server is still usable.
+ bazel info server_pid >& "$TEST_log" || fail "Couldn't use server"
+}
+
run_suite "Integration tests for ${PRODUCT_NAME}'s UI"
