download_queue: print fetch heading to stderr when stdout is not a TTY - #23406
download_queue: print fetch heading to stderr when stdout is not a TTY#23406carlocab wants to merge 2 commits into
Conversation
`brew info --installed --json=v2` emits `==> Downloading Homebrew API data` to stdout when the API data needs (re)downloading, corrupting the JSON for anything parsing it (e.g. piping to `jq`). The heading was added in 284cd0e (#23381) as an unconditional `oh1`, regressing the intent of 43137fe (#20980), which moved the queue's non-TTY per-download report lines to stderr for exactly this reason. Print the heading to stderr when stdout is not a TTY so it stays off parsed stdout and on the same stream as those report lines. TTY behaviour is unchanged. Use `$stderr.puts oh1_title(...)` rather than `$stderr.oh1` as RSpec's `to_stderr` matcher replaces `$stderr` with a plain `StringIO` that lacks the `Utils::Output::Mixin` extension.
|
@carlocab any AI disclosure here? |
There was a problem hiding this comment.
Pull request overview
This PR fixes machine-readable command output corruption by ensuring DownloadQueue prints its fetch heading to stderr when stdout is not a TTY (e.g. brew info --json=v2 | jq), while leaving TTY behavior unchanged.
Changes:
- Route
DownloadQueue#fetchheadings to stderr when@ttyis false, keeping parsed stdout clean. - Add/adjust RSpec coverage to assert heading stream behavior (stderr for non-TTY, stdout for TTY).
- Update cask reinstall specs to expect the “Fetching downloads…” heading on stderr.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Library/Homebrew/download_queue.rb | Prints the fetch heading to stderr when stdout is non-TTY to avoid corrupting machine-readable stdout. |
| Library/Homebrew/test/download_queue_spec.rb | Adds specs covering heading output stream behavior for TTY vs non-TTY cases. |
| Library/Homebrew/test/cask/reinstall_spec.rb | Updates expectations so the fetch heading is asserted on stderr. |
Suppressed comments (1)
Library/Homebrew/test/download_queue_spec.rb:174
- This example also depends on stdout being non-TTY at
DownloadQueueconstruction time, but doesn't stub$stdout.tty?before callingenqueue(which instantiates the subject). On a real TTY this will print the heading to stdout, defeating the assertion.
it "keeps the heading off stdout when stdout is not a TTY" do
allow(retryable_download).to receive(:fetch).and_return(cached_download)
download_queue.enqueue(downloadable)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Yes. Looks like my sandbox is misconfigured. Hang on. |
This addresses a Copilot review comment that is reachable when tests are run as `brew tests --debug`.
Done now! |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks! Refactor opportunity I think!
| oh1 heading, truncate: false | ||
| $stdout.flush | ||
| else | ||
| # Keep the heading off parsed stdout (e.g. `brew info --json | jq`) | ||
| # and on the same stream as the non-TTY report lines below. | ||
| $stderr.puts oh1_title(heading, truncate: false) |
There was a problem hiding this comment.
Are there any other headings in this file/around the codebase where we do this (maybe not the same approach but pretty sure there are)? If so, a helper method for this would be nice.
brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?Claude Fable xhigh with manual review and testing.
Running
brew info --installed --json=v2with a stale cache printsto stdout, which breaks JSON parsing. Let's fix this the same way as #20980.