Stream build output live instead of swallowing it - #26
Merged
Conversation
container build's stdout+stderr used to be fully captured by Command::output() and discarded on success, so a multi-minute build looked completely silent/hung. Add util::run_streaming, a sibling to run_ok for commands whose output is shown rather than parsed: the child's stderr is inherited and its stdout is redirected to a dup of our own stderr (not a forwarding thread), so both stream live while pall8t's own stdout stays clean for `built <tag>` / `ls --json`. No flag — a silent build is simply wrong, so this is always on. Stdin is explicitly closed (Stdio::null()): Command::status()'s default of inheriting it, unlike output()'s default of closing it, would otherwise hand a piped prompt or the controlling TTY to the build child. container::build_image now goes through run_streaming instead of run_ok. run_ok itself is untouched; its parsing callers still need captured output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #13.
pall8t build, and the implicit rebuild insidepall8t run, used to go throughutil::run_ok— which captures all ofcontainer build's stdout+stderr viaCommand::output()and discards it on success. A multi-minute build looked completely silent, and the user had no way to tell it was making progress. No-v/--quietflag: a silent build is simply wrong, so streaming is always on.util::run_streaming(program, args) -> Result<()>, a sibling torun_okfor commands whose output is meant to be shown, not parsed. The child's stderr is inherited and its stdout is redirected to a dup of pall8t's own stderr fd (std::io::stderr().as_fd().try_clone_to_owned()+Stdio::from(...)) — not a forwarding thread — so both streams interleave live with pall8t's owneprintln!progress messages, while pall8t's own stdout stays untouched (pall8t buildstill printsbuilt <tag>there;pall8t ls --jsonand the herdr integration depend on that staying clean).container::build_imagenow callsrun_streaminginstead ofrun_ok.run_okitself is unchanged — its callers (container list, image list, git) still need captured, parseable output.Stdio::null()). This mattered more than it looked:Command::status()(used byrun_streaming) inherits stdin by default, unlikeCommand::output()(used byrun_ok), which closes it. Left alone, a piped prompt or the controlling TTY would have been handed to the build child — an adversarial review pass caught this before it shipped.image::try_build's "building..." banner tweaked (trailing:instead of…) now that real build output immediately follows it.README.md/docs/requirements.mdnote that build output streams to stderr by default, worded to describe this specific path rather than a blanket "pall8t's stdout is always clean" claim (a neighboring, pre-existing code path —container::system_start, invoked byensure_container_systembeforecmd_ls's JSON print — still fully inherits stdio and isn't covered by this change; flagged below, not fixed here as it's a separate, unrelated gap).Test plan
cargo fmt --checkcargo clippy --all-targets -- -D warningscargo test— 127 tests pass, including 3 new ones forrun_streaming's error contract (nonzero exit carries the command line, missing binary is anErr); the stdout/stderr-split property itself is not unit-tested — documented in the test module why an in-process test for it would be unsafe undercargo test's parallel harness (would require swapping this test binary's own fd 1, corrupting concurrently running tests' output) rather than forcing a fragile test/code-review(self) and/skeptical-review(adversarial agent) — findings addressed in the commit, most notably the stdin-inheritance issue aboveNote for a possible follow-up (not fixed in this PR, out of scope for #13)
container::system_start(src/container.rs) uses full stdio inheritance and is called byensure_container_system()beforecmd_lsprints its JSON — ifcontainer system startever writes to its own stdout, that would leak intopall8t ls --json's output the same way build output used to leak. Separate code path, separate bug, worth its own issue if it turns out to matter in practice.