Skip to content

Update rust-tui so "cargo run" will not output in terminal#288

Open
Chris Draper (Chris-Draper) wants to merge 2 commits into
getditto:mainfrom
Chris-Draper:fix-rust-tui-cli-output
Open

Update rust-tui so "cargo run" will not output in terminal#288
Chris Draper (Chris-Draper) wants to merge 2 commits into
getditto:mainfrom
Chris-Draper:fix-rust-tui-cli-output

Conversation

@Chris-Draper
Copy link
Copy Markdown

Hey! I am interviewing for a Product Manager - Networking job at Ditto. I was using your SDK to try out your product.

I wanted to submit a PR for a small quality of life improvement in the rust-tui app

  • I made an adjustment in main.rs so that "cargo run" won't output stderr into the terminal by default
  • Any stderr output is redirect to the log file unless the user redirected the output to somewhere else
  • Added libc as a dependency

Let me know what you think.

@Chris-Draper
Copy link
Copy Markdown
Author

The multi-line comment in main.rs might be overkill, but I figured it was better to include it in my initial PR to explain the changes to stderr

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to prevent Ditto SDK C-internal writes to stderr from garbling the Rust TUI during local runs by redirecting stderr to the app’s log file when appropriate.

Changes:

  • Redirect stderr to the configured log file on Unix when stderr is attached to a terminal.
  • Update rust-tui README to remove the 2>/dev/null workaround and instruct cargo run.
  • Add libc dependency to support isatty/dup2.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
rust-tui/src/bin/main.rs Adds Unix-only stderr redirection to the log file before initializing tracing.
rust-tui/README.md Removes the 2>/dev/null workaround and simplifies run instructions.
rust-tui/Cargo.toml Adds libc dependency used by the new Unix-only redirection code.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread rust-tui/src/bin/main.rs
Comment on lines +57 to +58
if libc::isatty(libc::STDERR_FILENO) == 1 {
libc::dup2(logfile.as_raw_fd(), libc::STDERR_FILENO);
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems pretty reasonable. I feel like it can't hurt to have additional error handling in this case. Will leave this comment up for anyone from the Ditto engineering team to take a look.

Comment thread rust-tui/README.md
Comment thread rust-tui/Cargo.toml
Comment thread rust-tui/src/bin/main.rs Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants