Preserve carriage returns in run_command output (closes #18)#32
Conversation
Subprocess output passed through Shell.run_command used to lose its in-place \r-based progress updates and gain spurious newlines. Two stacked bugs caused this: 1. subprocess.Popen was opened with universal_newlines=True, which makes Python rewrite every \r (and \r\n) to \n before we ever see the bytes. That alone destroyed progress lines from tools like pip and apt before any formatting layer ran. 2. The read loop called output.strip() on every chunk and then printed with end="\n\r". Even if \r had survived (1), the strip + hardcoded newline would have turned each progress tick into a separate line with its own group prefix. This change: * Drops universal_newlines=True and decodes the raw bytes manually (utf-8 with errors='replace') so carriage returns survive. * Replaces the per-chunk self.info/self.error calls with a small helper, _emit_stream_chunk, that writes chunks straight through while emitting the colored group prefix exactly once per logical \n-terminated line. Bare \r in the middle of a chunk is treated as an in-place update of the current line, not a new line, so progress percentages overwrite in place the way they do at a real terminal. * Maintains a per-stream at_line_start flag across reads so a line split across chunk boundaries still gets exactly one prefix at its real start. * Adds a post-loop drain so output from short-lived commands that exit between poll() calls isn't dropped. Behavior with no group set is unchanged: raw pass-through of the process's own bytes.
Visual verificationBuilt two identical shell.run_command("apt-get update")
shell.run_command("script -qfec 'pip3 download --no-deps --dest /tmp/wheels --progress-bar on numpy' /dev/null")(The Left (published lib): every progress tick became its own Repro script and Dockerfiles are in About the failing CI checkThe failure isn't from anything this PR changed.
Probably worth a separate small PR to bump |
makermelissa
left a comment
makermelissa
left a comment
There was a problem hiding this comment.
Perfect. This has been bugging me for quite a while.
Follow-up to adafruit#32. While that fix preserved \r-based progress updates end-to-end, two cosmetic glitches remained when running real apt / pip installs through run_command: 1. apt's progress UI sometimes emits a stray padding space after a \n, e.g. 'Fetched 52.4 MB in 30s (1,729 kB/s)\n Selecting...'. The previous logic treated the trailing space as the start of a fresh logical line, wrote the group prefix, then the space, then the real content -- producing output like: PITFT Fetched 52.4 MB in 30s (1,729 kB/s) PITFT Selecting previously unselected package ... ^ stray space, prefix pushed one column right 2. A bare \r-only line (e.g. apt's 'erase-progress-line' sequence) was being treated as 'still on the same logical line', so the next chunk's content would be concatenated onto the previous line's tail in pipe/log capture, and the prefix bookkeeping for the *next* \n boundary could drift. This change: * Splits chunks on either \n or runs of \r (\r+), so cursor-at-col-0 is the actual boundary, not just newline. * Treats only \n as a *prefix-re-emission* boundary. Bare \r is cursor-return: subsequent redraw frames are written without the prefix, which is how pip/apt progress UIs are designed to animate on a real terminal (each redraw overwrites the previous frame, including the prefix, so the terminal naturally shows the latest frame with no stale prefix dangling at column 0). * Strips leading horizontal whitespace (spaces and tabs) immediately after a \n boundary, so padding from the source process doesn't push the prefix right. * Suppresses the prefix entirely when a logical line is pure padding, but keeps the terminator (\r or \n) so the terminal still sees the cursor motion. Verified end-to-end via a deterministic harness that simulates terminal CR/LF cursor semantics against captured output, including the exact apt+pip patterns Melissa saw during the PiTFT install. ruff check + ruff format clean.
2 participants
Closes #18.
What was broken
Output streamed through
Shell.run_commandlost any\r-based in-place updates (e.g.pip/aptdownload progress bars) and gained extra newlines. The reporter (Melissa) saw progress percentages scrolling on separate lines instead of updating in place.There were actually two stacked bugs:
Popen(..., universal_newlines=True)— Python's universal newlines mode rewrites every\r(and\r\n) to\nbefore the read ever returns. So the carriage returns were destroyed before any formatting layer ran. Verifiable in isolation:self.info(output.strip(), end="\n\r")— Even if\rhad survived (1),.strip()would have removed it, and the hardcodedend="\n\r"would have forced a newline on every chunk, with the group prefix re-emitted each time. So a real progress sequence rendered as separate prefixed lines.The fix
universal_newlines=True; read raw bytes and decode manually asutf-8witherrors="replace"so a stray non-UTF-8 byte doesn't kill the whole run.self.info/self.errorcalls with a small helper,_emit_stream_chunk, that writes chunks straight through and emits the colored group prefix exactly once per logical\n-terminated line. A bare\rmid-chunk is treated as an in-place update of the current line (no re-prefix).at_line_startflag across reads so a line split across chunk boundaries still gets exactly one prefix at its real start.poll()calls isn't dropped.sys.stdoutexactly like the originalprint()-based path.When
self._groupisNone, the output is raw pass-through, matching the old behavior in that case.Verification
Ran a small harness against the real
Shell.run_command(capturedsys.stdoutto aStringIOto inspect bytes). Five scenarios, all assertions pass:'[install] line one\n[install] line two\n[install] line three\n'\r'[install] Downloading 12%\rDownloading 24%\rDownloading 100%\n[install] Done\n'\rpreserved, prefix once at line start, not after each\r✅'[install] stdout line\n[install] stderr line\n''first half '+ sleep +'second half\n')'[install] first half second half\n''plain one\nplain two\n'Also verified
return_output=Truestill returns astr(not bytes).blackclean,pylint10/10 with the project's--disable=consider-using-f-string.Downstream check
gh search code --owner=adafruit "run_command"confirms all downstream callers inRaspberry-Pi-Installer-Scripts(and elsewhere) useshell.run_command("...")for side effects; none parse the printed output for\n\rboundaries or the.strip()behavior, so this is safe to land.