Fix output visibility for nix copy commands after subprocess update#626
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBumps workspace Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Parent as Parent Process
participant ExecFn as exec_with_streaming
participant Child as Child Process
participant StdoutRd as Stdout Reader Thread
participant StderrRd as Stderr Reader Thread
participant Terminal
User->>Parent: request operation that runs external command
Parent->>ExecFn: build Exec and call exec_with_streaming
ExecFn->>Child: spawn child with stdout/stderr pipes
ExecFn->>StdoutRd: spawn stdout reader thread
ExecFn->>StderrRd: spawn stderr reader thread
par stream stdout
StdoutRd->>Child: read stdout chunks
StdoutRd->>Terminal: write chunks live
StdoutRd->>ExecFn: accumulate stdout
and stream stderr
StderrRd->>Child: read stderr chunks
StderrRd->>Terminal: write chunks live
StderrRd->>ExecFn: accumulate stderr
end
Child->>ExecFn: exit with status
ExecFn->>StdoutRd: join
ExecFn->>StderrRd: join
StdoutRd->>ExecFn: return accumulated stdout
StderrRd->>ExecFn: return accumulated stderr
ExecFn->>Parent: return (exit_status, stdout, stderr)
Parent->>User: proceed based on exit_status and streamed output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I don't like the helper approach btw, but it's the cleanest way we can do this in a futureproof manner imho. Unless we decide to pull |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/nh-remote/src/remote.rs (1)
1043-1064:⚠️ Potential issue | 🟠 MajorSpinner and streamed child output will fight over the terminal.
ProgressBar::new_spinner()withenable_steady_tick(80ms)writes spinner frames to stderr on every tick, whileexec_with_streaming(cmd)simultaneously streams the child's stdout and stderr directly tostd::io::stdout()/std::io::stderr()in background threads.nix copyprogress output goes to stderr, so you'll get interleaving: child stderr lines breaking up spinner redraws, and spinner tick sequences (\r, cursor moves) garbling child output.Options to consider:
- Drop the spinner when streaming is enabled — the child's own progress output already tells the user things are happening.
- Or, suspend the spinner for the duration of the streamed exec (indicatif's
MultiProgress/suspendpattern), and only start ticking again after the child exits.- Or, keep capture-only behavior for
nix copy --to(where the spinner is justified) and only stream for the command sites that actually need live progress.As-is, the behavior on a real
nix copywill likely be visually worse than either the old capture+spinner path or a pure pass-through. Worth confirming interactively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh-remote/src/remote.rs` around lines 1043 - 1064, The spinner (ProgressBar::new_spinner + enable_steady_tick) races with exec_with_streaming(cmd) because both write to stderr; fix by suspending the spinner while the child is streamed: wrap the exec_with_streaming(cmd) call inside spinner.suspend(|| { ... }) (using ProgressBar::suspend) so the spinner's ticks are paused during streaming, then finish_and_clear the spinner and handle exit_status/stderr as before; alternatively, if you prefer no spinner during streaming, skip creating/enabling the spinner when calling exec_with_streaming.
🧹 Nitpick comments (2)
crates/nh-remote/src/remote.rs (1)
829-834: Repeatedexec_with_streaming+bail!block — consider extracting a helper.The same pattern
let (status, _out, stderr) = exec_with_streaming(cmd).wrap_err(ctx)?; if !status.success() { bail!("{} failed:\n{}", label, stderr); }appears four times in this file (plus the spinner variant). A small helper likerun_streamed(cmd, context, label)or afn check_status(status, label, stderr) -> Result<()>would de-duplicate and keep the error shape consistent.Non-blocking.
Also applies to: 977-982, 1054-1063, 1096-1106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh-remote/src/remote.rs` around lines 829 - 834, The code repeats the pattern of calling exec_with_streaming(...).wrap_err(...)? followed by checking exit_status.success() and bail!-ing with stderr (seen around exec_with_streaming usages and the spinner variant); extract a small helper function (e.g., run_streamed(cmd: Command, ctx_msg: &str, label: &str) -> Result<()>) that wraps exec_with_streaming, applies wrap_err(ctx_msg), checks the returned exit_status, and returns an Err with the same bail!-style message including stderr when not successful; replace each duplicated block (references: exec_with_streaming calls around lines that build the nix-copy-closure call and the other three occurrences) with calls to this helper so error shape and logging remain consistent.crates/nh-core/src/command.rs (1)
41-54:exec_with_streamingsilently overrides any redirection already set oncmd.Lines 42–43 unconditionally apply
stdout(Pipe).stderr(Pipe), which will clobber any priorstderr(Redirection::Merge),stdout(Redirection::None),stderr(Redirection::File)etc. the caller configured on the passed-inExec. Today's four call sites inremote.rshappen not to pre-set these, but the function ispuband the name doesn't hint at that contract. Please either:
- Document the redirection override behavior on the docstring, or
- Take only the pieces you need (e.g., a command + args builder) rather than an arbitrary
Exec, to make the override intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh-core/src/command.rs` around lines 41 - 54, exec_with_streaming currently unconditionally calls cmd.stdout(Redirection::Pipe).stderr(Redirection::Pipe) which clobbers any redirections the caller set on the passed-in Exec; change the API so this override is intentional: replace the pub fn exec_with_streaming(cmd: Exec, ...) signature with one that takes the command name and args (e.g., fn exec_with_streaming<S: AsRef<str>>(program: S, args: &[S], ...) or a small builder type) and construct the Exec inside the function so you control redirections, then update all call sites in remote.rs to pass program+args; alternatively (if you prefer minimal churn) detect whether stdout/stderr redirections are already set on the supplied Exec and only apply .stdout(Redirection::Pipe)/.stderr(Redirection::Pipe) when none exist, and update the exec_with_streaming docstring to clearly document the redirection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/nh-core/src/command.rs`:
- Around line 38-106: Command::run still drops stdout on success because it uses
cmd.capture() and never prints capture.stdout_str() in the success branch;
update Command::run (the method that checks self.show_output) to either call
exec_with_streaming when self.show_output is true or use Exec::join()/cmd.join()
instead of cmd.capture() so stdout is streamed/returned and the GC summary
(capture.stdout_str()) is preserved and printed on success; ensure the success
branch prints the captured stdout (capture.stdout_str()) or consumes the join
result so the "N store paths deleted, X MiB freed" summary is emitted.
- Around line 56-92: In both stdout_thread and stderr_thread replace the
per-chunk lossy decoding and the catch-all break on Err(_) by accumulating raw
bytes into a Vec<u8> (e.g., stdout_bytes / stderr_bytes) while still writing
each chunk to the terminal, and when the loop ends convert the entire Vec<u8>
once with String::from_utf8_lossy() into stdout_output / stderr_output; also
treat Ok(0) as EOF but on Err(e) call debug! (or processLogger equivalent) with
the error before breaking so real I/O errors aren’t silently swallowed
(referencing stdout_thread, stderr_thread, stdout_reader, stderr_reader,
stdout_output, stderr_output).
In `@crates/nh-remote/src/remote.rs`:
- Around line 829-834: The failure handlers that call exec_with_streaming
(capturing exit_status and stderr) should not re-print the child's stderr
because exec_with_streaming already streamed it; instead, update each bail! call
(the call sites that bind exit_status, _stdout, stderr from exec_with_streaming)
to omit stderr from the message and report the command exit status (e.g.,
include exit_status or exit_status.code()). Specifically, in the places using
exec_with_streaming and then bail!("… failed:\n{}", stderr) replace that message
with a concise message including the exit_status (or exit code) only so the
already-streamed stderr isn't duplicated.
---
Outside diff comments:
In `@crates/nh-remote/src/remote.rs`:
- Around line 1043-1064: The spinner (ProgressBar::new_spinner +
enable_steady_tick) races with exec_with_streaming(cmd) because both write to
stderr; fix by suspending the spinner while the child is streamed: wrap the
exec_with_streaming(cmd) call inside spinner.suspend(|| { ... }) (using
ProgressBar::suspend) so the spinner's ticks are paused during streaming, then
finish_and_clear the spinner and handle exit_status/stderr as before;
alternatively, if you prefer no spinner during streaming, skip creating/enabling
the spinner when calling exec_with_streaming.
---
Nitpick comments:
In `@crates/nh-core/src/command.rs`:
- Around line 41-54: exec_with_streaming currently unconditionally calls
cmd.stdout(Redirection::Pipe).stderr(Redirection::Pipe) which clobbers any
redirections the caller set on the passed-in Exec; change the API so this
override is intentional: replace the pub fn exec_with_streaming(cmd: Exec, ...)
signature with one that takes the command name and args (e.g., fn
exec_with_streaming<S: AsRef<str>>(program: S, args: &[S], ...) or a small
builder type) and construct the Exec inside the function so you control
redirections, then update all call sites in remote.rs to pass program+args;
alternatively (if you prefer minimal churn) detect whether stdout/stderr
redirections are already set on the supplied Exec and only apply
.stdout(Redirection::Pipe)/.stderr(Redirection::Pipe) when none exist, and
update the exec_with_streaming docstring to clearly document the redirection
behavior.
In `@crates/nh-remote/src/remote.rs`:
- Around line 829-834: The code repeats the pattern of calling
exec_with_streaming(...).wrap_err(...)? followed by checking
exit_status.success() and bail!-ing with stderr (seen around exec_with_streaming
usages and the spinner variant); extract a small helper function (e.g.,
run_streamed(cmd: Command, ctx_msg: &str, label: &str) -> Result<()>) that wraps
exec_with_streaming, applies wrap_err(ctx_msg), checks the returned exit_status,
and returns an Err with the same bail!-style message including stderr when not
successful; replace each duplicated block (references: exec_with_streaming calls
around lines that build the nix-copy-closure call and the other three
occurrences) with calls to this helper so error shape and logging remain
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7913b9e1-55e1-4513-a119-dcfea5dbca07
📒 Files selected for processing (3)
Cargo.tomlcrates/nh-core/src/command.rscrates/nh-remote/src/remote.rs
NotAShelf
force-pushed
the
notashelf/push-zlyknnslyqoz
branch
from
44dcf35 to
fdd4098
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/nh-core/src/command.rs (1)
827-859: Please pin theshow_outputfix with a regression test.This branch is the actual fix path for the visibility regression, but the test module still doesn't exercise
Command::run().show_output(true)/exec_with_streaming()on either success or failure. A focused regression test here would make#624/#620 much harder to reintroduce.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh-core/src/command.rs` around lines 827 - 859, Add a focused regression test that exercises the exec_with_streaming code path by calling Command::run().show_output(true); specifically create (1) a test case where the command succeeds to ensure the streaming-success branch (`exec_with_streaming`) is exercised and returns Ok, and (2) a test case where the command fails to ensure the failure branch returns an Err containing the message, exit status and streamed stderr; locate tests near the existing tests for Command::run()/command.rs, invoke the same command execution helpers used in production, and assert on the returned Result and any captured stderr to prevent future regressions of the show_output fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/nh-core/src/command.rs`:
- Around line 41-44: The streaming helper exec_with_streaming() currently forces
both stdout and stderr to Redirection::Pipe, which overwrites caller-configured
stderr (e.g., Redirection::Merge) and breaks ordering; update
exec_with_streaming() so it only sets stdout to Pipe but preserves the caller's
stderr redirection: detect the command's existing stderr redirection (or accept
it as a parameter) and avoid calling .stderr(Redirection::Pipe) when the caller
already set Redirection::Merge (or any non-default), ensuring stderr is left as
configured and only stdout is piped for streaming.
---
Nitpick comments:
In `@crates/nh-core/src/command.rs`:
- Around line 827-859: Add a focused regression test that exercises the
exec_with_streaming code path by calling Command::run().show_output(true);
specifically create (1) a test case where the command succeeds to ensure the
streaming-success branch (`exec_with_streaming`) is exercised and returns Ok,
and (2) a test case where the command fails to ensure the failure branch returns
an Err containing the message, exit status and streamed stderr; locate tests
near the existing tests for Command::run()/command.rs, invoke the same command
execution helpers used in production, and assert on the returned Result and any
captured stderr to prevent future regressions of the show_output fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7405c7de-4cc8-4a89-8b1e-af43e9e23e95
📒 Files selected for processing (3)
Cargo.tomlcrates/nh-core/src/command.rscrates/nh-remote/src/remote.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/nh-remote/src/remote.rs
NotAShelf
force-pushed
the
notashelf/push-zlyknnslyqoz
branch
from
fdd4098 to
9632e57
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/nh-core/src/command.rs (1)
827-835:⚠️ Potential issue | 🟠 MajorUse
join()forCommand::run(show_output = true)instead ofexec_with_streaming().The
show_outputbranch pipes stdout throughexec_with_streaming(), which forcesstdout(Redirection::Pipe)and buffers the entire output in memory (lines 53, 62, 77, 86), even though the captured output is immediately discarded (_stdoutand_stderrat line 828). This also changes TTY-sensitive behavior: the command is configured with inherited stdout (no explicit redirection at line 803-804), but piping it changes how the process sees its output stream.Using
cmd.join()instead—which is already available onExecand used elsewhere in the codebase (Build::run_nix)—avoids both the unnecessary buffering and the TTY behavior change, while maintaining the same error handling pattern.Suggested change
if self.show_output { - let (exit_status, _stdout, _stderr) = - exec_with_streaming(cmd).wrap_err(msg.clone())?; + let exit_status = cmd.join().wrap_err(msg.clone())?; if !exit_status.success() { return Err(eyre::eyre!(format!( "{} (exit status {:?})", msg, exit_status )));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh-core/src/command.rs` around lines 827 - 835, In the show_output branch of Command::run replace the exec_with_streaming call with cmd.join() so you don't force stdout/stderr to be piped and buffer output; specifically, where exec_with_streaming(cmd).wrap_err(msg.clone())? is used, call cmd.join().wrap_err(msg.clone())? (or otherwise await the Join result) and keep the same exit_status check and eyre error construction; reference exec_with_streaming, cmd.join, and the show_output conditional to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/nh-core/src/command.rs`:
- Around line 827-835: In the show_output branch of Command::run replace the
exec_with_streaming call with cmd.join() so you don't force stdout/stderr to be
piped and buffer output; specifically, where
exec_with_streaming(cmd).wrap_err(msg.clone())? is used, call
cmd.join().wrap_err(msg.clone())? (or otherwise await the Join result) and keep
the same exit_status check and eyre error construction; reference
exec_with_streaming, cmd.join, and the show_output conditional to locate the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b373680-75d3-48ec-9c53-e35a1ab3d7e5
📒 Files selected for processing (3)
Cargo.tomlcrates/nh-core/src/command.rscrates/nh-remote/src/remote.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/nh-remote/src/remote.rs
NotAShelf
force-pushed
the
notashelf/push-zlyknnslyqoz
branch
from
9632e57 to
74d53e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/nh-core/src/command.rs (1)
827-857: Please add a regression test for theshow_outputfix.Line 827 is the
#624fix path, but there is no coverage here proving that a successfulshow_output(true)command still emits stdout after the subprocess upgrade. A small integration-style test aroundecho/printfwould keep this from silently regressing again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/nh-core/src/command.rs` around lines 827 - 857, Add an integration-style regression test that exercises the show_output(true) code path (the branch using cmd.join() in the current diff) to ensure stdout is still emitted after the subprocess upgrade: spawn a simple command like echo or printf that writes to stdout, call the same API that sets show_output = true and runs the command (triggering cmd.join()), and assert that the test process receives/observes the emitted stdout (or that the command succeeds and its stdout is not empty). Place the test alongside other command tests, run the branch that uses cmd.capture() vs cmd.join() as appropriate, and ensure the test fails if stdout is suppressed to prevent future regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/nh-core/src/command.rs`:
- Around line 38-113: The exec_with_streaming helper currently accumulates all
stdout/stderr into memory (stdout_bytes/stderr_bytes) even when callers ignore
the returned strings; change it to avoid full-buffer capture by either adding a
capture flag or a separate status-only variant. Update exec_with_streaming (or
add exec_with_streaming_status_only) so that when capture_output is false the
reader threads write to stdout/stderr but do not extend
stdout_bytes/stderr_bytes and instead return early (or join with empty strings),
and ensure job.wait() and thread joins still happen to avoid races; adjust
callers to use the new flag/variant where they only need ExitStatus. Ensure
references to stdout_thread, stderr_thread, stdout_bytes, stderr_bytes and the
function name exec_with_streaming (or new exec_with_streaming_status_only) are
updated accordingly.
In `@crates/nh-remote/src/remote.rs`:
- Around line 1062-1068: Move the spinner.finish_and_clear() call to execute
immediately before invoking exec_with_streaming(cmd) so the spinner is cleared
before any child process output is streamed; specifically, locate the
exec_with_streaming(...) invocation and the spinner.finish_and_clear() call in
remote.rs and swap their order (ensure spinner.finish_and_clear() runs on the
current thread right before calling exec_with_streaming(cmd) and remove the old
call after the exec_with_streaming result handling).
---
Nitpick comments:
In `@crates/nh-core/src/command.rs`:
- Around line 827-857: Add an integration-style regression test that exercises
the show_output(true) code path (the branch using cmd.join() in the current
diff) to ensure stdout is still emitted after the subprocess upgrade: spawn a
simple command like echo or printf that writes to stdout, call the same API that
sets show_output = true and runs the command (triggering cmd.join()), and assert
that the test process receives/observes the emitted stdout (or that the command
succeeds and its stdout is not empty). Place the test alongside other command
tests, run the branch that uses cmd.capture() vs cmd.join() as appropriate, and
ensure the test fails if stdout is suppressed to prevent future regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 767f2b69-9d22-4d1e-9134-7289c593b5ec
📒 Files selected for processing (3)
Cargo.tomlcrates/nh-core/src/command.rscrates/nh-remote/src/remote.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
| let (exit_status, _stdout, _stderr) = exec_with_streaming(cmd) | ||
| .wrap_err("Failed to copy closure to remote host")?; | ||
|
|
||
| // We finish and *clear*, because the log line needs to come next. If we try | ||
| // to make the spinner change the text, we cannot reliably match the `info!` | ||
| // or `error!` style. | ||
| spinner.finish_and_clear(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and read the relevant lines
head -1070 crates/nh-remote/src/remote.rs | tail -20Repository: nix-community/nh
Length of output: 876
🏁 Script executed:
# Find the exec_with_streaming function definition
rg -n "fn exec_with_streaming" crates/nh-remote/Repository: nix-community/nh
Length of output: 42
🏁 Script executed:
# Look at the context around line 1062 for spinner usage
sed -n '1050,1080p' crates/nh-remote/src/remote.rsRepository: nix-community/nh
Length of output: 1047
🏁 Script executed:
# Search for exec_with_streaming definition across the codebase
rg -n "exec_with_streaming" crates/nh-remote/src/Repository: nix-community/nh
Length of output: 521
🏁 Script executed:
# Also check if it might be in another crate
rg -n "fn exec_with_streaming"Repository: nix-community/nh
Length of output: 121
🏁 Script executed:
# Look for where it's defined, might be in a different module
rg -B3 -A10 "exec_with_streaming"Repository: nix-community/nh
Length of output: 4980
🏁 Script executed:
# Read the exec_with_streaming function
sed -n '38,120p' crates/nh-core/src/command.rsRepository: nix-community/nh
Length of output: 2355
Move spinner.finish_and_clear() before exec_with_streaming() call.
exec_with_streaming streams live output from the child process by writing to stdout/stderr in real-time via background threads. The spinner continues redrawing every 80ms while the command runs, causing interleaved terminal output that degrades readability on interactive terminals. Clearing the spinner before streaming starts ensures clean output from the command.
Suggested fix
- let (exit_status, _stdout, _stderr) = exec_with_streaming(cmd)
- .wrap_err("Failed to copy closure to remote host")?;
-
- // We finish and *clear*, because the log line needs to come next. If we try
- // to make the spinner change the text, we cannot reliably match the `info!`
- // or `error!` style.
spinner.finish_and_clear();
+ let (exit_status, _stdout, _stderr) = exec_with_streaming(cmd)
+ .wrap_err("Failed to copy closure to remote host")?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (exit_status, _stdout, _stderr) = exec_with_streaming(cmd) | |
| .wrap_err("Failed to copy closure to remote host")?; | |
| // We finish and *clear*, because the log line needs to come next. If we try | |
| // to make the spinner change the text, we cannot reliably match the `info!` | |
| // or `error!` style. | |
| spinner.finish_and_clear(); | |
| spinner.finish_and_clear(); | |
| let (exit_status, _stdout, _stderr) = exec_with_streaming(cmd) | |
| .wrap_err("Failed to copy closure to remote host")?; | |
| // We finish and *clear*, because the log line needs to come next. If we try | |
| // to make the spinner change the text, we cannot reliably match the `info!` | |
| // or `error!` style. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/nh-remote/src/remote.rs` around lines 1062 - 1068, Move the
spinner.finish_and_clear() call to execute immediately before invoking
exec_with_streaming(cmd) so the spinner is cleared before any child process
output is streamed; specifically, locate the exec_with_streaming(...) invocation
and the spinner.finish_and_clear() call in remote.rs and swap their order
(ensure spinner.finish_and_clear() runs on the current thread right before
calling exec_with_streaming(cmd) and remove the old call after the
exec_with_streaming result handling).
I had hoped that it would fix the capture behaviour... Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I221b472749139d4292617c453cf467626a6a6964
Subprocess 1.0.x changed `.capture()` to redirect stdout/stderr to pipes by default, hiding progress output from `nix-copy-closure` and `nix copy` commands. Users would see no output during remote copies unless the command failed. Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I0e1fcc7e19b0fdba857e4f5822449b586a6a6964
NotAShelf
force-pushed
the
notashelf/push-zlyknnslyqoz
branch
from
74d53e3 to
3ab3f02
Compare
2 participants
Add
exec_with_streaming()helper to nh_core::command that uses.start()with piped stdout/stderr and spawns threads to stream output to the terminal in real-time. This captures output for error reporting on failure, and HOPEFULLY fixes the stdout/stderr bs that was happening after the Subprocess 1.0 upgrade.Fixes: #624, #620
Summary by CodeRabbit
New Features
Bug Fixes
Chores