feat(cmd): send verbose make logs to stderr#115
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the make command to redirect verbose build logs to stderr while reserving stdout for the final metadata line, allowing callers to capture the output separately. The test suite has been updated to capture both streams concurrently, and a new test has been added to verify this behavior. The review feedback highlights critical issues in the tests: first, modifying global variables makeVerbose and makeOutput without restoring them can cause test pollution; second, ignoring errors from os.Pipe() and failing to close the read ends of the pipes in runMakeCmdStreams and captureBuildModuleStreams leads to potential nil pointer dereferences and file descriptor leaks.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #115 +/- ##
==========================================
+ Coverage 80.43% 80.46% +0.02%
==========================================
Files 37 37
Lines 2116 2129 +13
==========================================
+ Hits 1702 1713 +11
- Misses 300 301 +1
- Partials 114 115 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
MeteorsLiu
force-pushed
the
codex/make-output-contract
branch
from
45a7ce7 to
56d8f94
Compare
|
/gemini review |
|
/review |
MeteorsLiu
changed the title
There was a problem hiding this comment.
Code Review
This pull request refactors build output redirection in cmd/llar/internal/make.go by extracting the logic into a redirectBuildOutput helper, which redirects stdout to stderr in verbose mode and discards it in silent mode. It also updates the testing framework to capture both stdout and stderr concurrently using pipes and adds a new integration test for verbose output. The review feedback highlights several issues: 1) os.Stdout and os.Stderr should be restored before closing devNull to prevent panics from concurrent writes; 2) the read ends of the pipes in runMakeCmdStreams and captureProcessStreams should be closed to prevent file descriptor leaks; and 3) the global makeVerbose variable modified in tests should be restored using t.Cleanup to avoid test flakiness.
| return func() { | ||
| _ = devNull.Close() | ||
| os.Stdout = savedStdout | ||
| os.Stderr = savedStderr | ||
| }, nil |
There was a problem hiding this comment.
Restore os.Stdout and os.Stderr before closing devNull. If devNull is closed first, any concurrent or immediate writes to os.Stdout or os.Stderr before they are restored will fail or panic because they still point to the closed devNull file descriptor.
| return func() { | |
| _ = devNull.Close() | |
| os.Stdout = savedStdout | |
| os.Stderr = savedStderr | |
| }, nil | |
| return func() { | |
| os.Stdout = savedStdout | |
| os.Stderr = savedStderr | |
| _ = devNull.Close() | |
| }, nil |
| oldStdout := os.Stdout | ||
| stdoutR, stdoutW, _ := os.Pipe() | ||
| os.Stdout = stdoutW | ||
| defer func() { os.Stdout = oldStdout }() | ||
|
|
||
| oldStderr := os.Stderr | ||
| stderrR, stderrW, _ := os.Pipe() | ||
| os.Stderr = stderrW | ||
| defer func() { os.Stderr = oldStderr }() |
There was a problem hiding this comment.
Close the read ends of the pipes (stdoutR and stderrR) when they are no longer needed to prevent file descriptor leaks during test execution.
| oldStdout := os.Stdout | |
| stdoutR, stdoutW, _ := os.Pipe() | |
| os.Stdout = stdoutW | |
| defer func() { os.Stdout = oldStdout }() | |
| oldStderr := os.Stderr | |
| stderrR, stderrW, _ := os.Pipe() | |
| os.Stderr = stderrW | |
| defer func() { os.Stderr = oldStderr }() | |
| oldStdout := os.Stdout | |
| stdoutR, stdoutW, _ := os.Pipe() | |
| defer stdoutR.Close() | |
| os.Stdout = stdoutW | |
| defer func() { os.Stdout = oldStdout }() | |
| oldStderr := os.Stderr | |
| stderrR, stderrW, _ := os.Pipe() | |
| defer stderrR.Close() | |
| os.Stderr = stderrW | |
| defer func() { os.Stderr = oldStderr }() |
| return &stdoutBuf, &stderrBuf, func() { | ||
| _ = stdoutW.Close() | ||
| if copyErr := <-stdoutDone; copyErr != nil { | ||
| t.Fatalf("failed to capture stdout: %v", copyErr) | ||
| } | ||
| os.Stdout = oldStdout | ||
|
|
||
| _ = stderrW.Close() | ||
| if copyErr := <-stderrDone; copyErr != nil { | ||
| t.Fatalf("failed to capture stderr: %v", copyErr) | ||
| } | ||
| os.Stderr = oldStderr | ||
| } |
There was a problem hiding this comment.
Close the read ends of the pipes (stdoutR and stderrR) in the returned cleanup function to prevent file descriptor leaks.
return &stdoutBuf, &stderrBuf, func() {
_ = stdoutW.Close()
if copyErr := <-stdoutDone; copyErr != nil {
t.Fatalf("failed to capture stdout: %v", copyErr)
}
_ = stdoutR.Close()
os.Stdout = oldStdout
_ = stderrW.Close()
if copyErr := <-stderrDone; copyErr != nil {
t.Fatalf("failed to capture stderr: %v", copyErr)
}
_ = stderrR.Close()
os.Stderr = oldStderr
}| t.Fatalf("modules.Load() failed: %v", err) | ||
| } | ||
|
|
||
| makeVerbose = true |
There was a problem hiding this comment.
Restore the global makeVerbose variable to its original value after the test finishes using t.Cleanup. Modifying global variables in tests without cleanup can cause side effects and flakiness in other tests running in the same package.
| makeVerbose = true | |
| origVerbose := makeVerbose | |
| makeVerbose = true | |
| t.Cleanup(func() { makeVerbose = origVerbose }) |
There was a problem hiding this comment.
Review Summary
Clean refactoring that extracts output redirection into a well-structured helper with proper restore semantics. The new verbose-to-stderr behavior is a solid improvement for machine-parseable stdout. A few items below, mostly in the test code.
| t.Fatalf("modules.Load() failed: %v", err) | ||
| } | ||
|
|
||
| makeVerbose = true |
There was a problem hiding this comment.
Bug (test isolation): makeVerbose is set to true but never restored. If test execution order places another test after this one that assumes the default false, it will fail. Other tests in this file (e.g., TestBuildModule_SilentSuccess) properly defer-restore:
| makeVerbose = true | |
| makeVerbose = true | |
| defer func() { makeVerbose = false }() |
|
|
||
| var buf bytes.Buffer | ||
| copyDone := make(chan error, 1) | ||
| // flows through process-wide os.Stdout/os.Stderr (including nested build |
There was a problem hiding this comment.
Nit (duplication): The pipe-capture-and-drain pattern in runMakeCmdStreams (lines 336-374) is nearly identical to captureProcessStreams (lines 382-418). Consider having runMakeCmdStreams delegate to captureProcessStreams internally to maintain the capture logic in one place.
| // commands), redirect to pipes and drain concurrently to avoid blocking on | ||
| // full pipe buffers. | ||
| oldStdout := os.Stdout | ||
| stdoutR, stdoutW, _ := os.Pipe() |
There was a problem hiding this comment.
Nit: os.Pipe() errors are silently discarded here (and in captureProcessStreams at similar lines). While unlikely to fail, a t.Fatal(err) on pipe creation error would give a clear failure message instead of a confusing nil-pointer panic on the write end.
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit (doc accuracy): "discarded until the metadata line is printed" implies this function itself watches for a metadata line and then switches behavior. In reality, the caller is responsible for calling the restore function before printing metadata. Consider:
| return nil | |
| } | |
| // redirectBuildOutput reserves command stdout for final metadata. In verbose | |
| // mode, build stdout is redirected to stderr; in silent mode, all build output | |
| // is discarded. The caller must invoke the returned restore function before | |
| // writing final results to stdout. |
1 participant
Summary
Tests
Note: non-short go test ./cmd/llar/internal still hits existing real integration tests that write to the user cache and failed locally with permission denied.