base: fix pty view because crlf are lost in .lines() - #8332
Conversation
when we have pty view we have to preserve all control chars to make it work propely
There was a problem hiding this comment.
Code Review
This pull request updates BazelExecServiceImpl.kt to handle stderr output using raw character buffers when a PTY is active, ensuring control characters are preserved. The reviewer recommended applying a similar approach to stdout to prevent output mangling and suggested refactoring the stderr logic to reduce code duplication by moving the buffered reader initialization outside the conditional check.
| reader.lines().forEach { line -> | ||
| ctx.println(line) | ||
| LOG.debug("BAZEL ERR: $line") | ||
| if (usePty) { |
There was a problem hiding this comment.
While this change correctly handles stderr for PTY output by preserving control characters, the stdout stream (handled at line 145) is still being passed to a LineProcessingOutputStream in the build() method (line 252). This will cause stdout to still lose control characters and CRLF, as LineProcessingOutputStream and the subsequent ctx.println call will mangle the output by trimming and normalizing line endings. Consider applying a similar raw-output approach to stdout when usePty is true to ensure the PTY view works correctly for all output streams.
References
- A simpler, custom implementation can be preferable to a standard one if it avoids unwanted complexity for the specific use case.
| if (usePty) { | ||
| process.errorStream.bufferedReader().use { reader -> | ||
| val buf = CharArray(DEFAULT_BUFFER_SIZE) | ||
| while (true) { | ||
| val n = reader.read(buf) | ||
| if (n < 0) break | ||
| val text = String(buf, 0, n) | ||
| ctx.output(PrintOutput.process(text)) | ||
| // no log.debug because it's pty | ||
| } | ||
| } | ||
| } else { | ||
| process.errorStream.bufferedReader().use { reader -> | ||
| reader.lines().forEach { line -> | ||
| ctx.println(line) | ||
| LOG.debug("BAZEL ERR: $line") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for opening the bufferedReader on process.errorStream is duplicated in both branches of the if (usePty) check. You can simplify the code by moving the use block outside the conditional to improve maintainability.
process.errorStream.bufferedReader().use { reader ->
if (usePty) {
val buf = CharArray(DEFAULT_BUFFER_SIZE)
while (true) {
val n = reader.read(buf)
if (n < 0) break
ctx.output(PrintOutput.process(String(buf, 0, n)))
// no log.debug because it's pty
}
} else {
reader.lines().forEach { line ->
ctx.println(line)
LOG.debug("BAZEL ERR: $line")
}
}
}
when we have pty view we have to preserve all control chars to make it work propely