Skip to content

Implement PTY to handle both stdout, stderr with redirects #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Apr 10, 2025
Merged

Conversation

heaths
Copy link
Owner

@heaths heaths commented Feb 24, 2025

No description provided.

@heaths
Copy link
Owner Author

heaths commented Feb 24, 2025

Doesn't work on Windows because it uses AsFd et. al. for nix. This definitely indicates that to solve #29 I'll need to write my own, which also gives me an opportunity to split (perhaps tag like the current HEAD at this time?) stdout and stderr.

@heaths
Copy link
Owner Author

heaths commented Mar 16, 2025

I may need to fork the project or just start anew. Their repo site is giving an HTTP 502.

In any case, the question remains how to:

  1. Maintain any VT color sequences written by the child process.
  2. Split stdout, stderr according to what the user specified e.g., akv run -- child_proc 2> /dev/null.
  3. Mask any retrieved secrets in both stdout, stderr.

Considering https://www.man7.org/linux/man-pages/man3/forkpty.3.html and a .NET project I found at https://github.com/microsoft/vs-pty.net, maybe I'm overthinking this. If I start a process under a PTY, I don't need to actually handle stdout and stderr separately. I mask any secrets found regardless. So something like https://docs.rs/pty-process/0.5.1/pty_process/blocking/struct.Command.html or equivalent on Windows should work.

@heaths
Copy link
Owner Author

heaths commented Mar 17, 2025

This does actually work - well, almost. I still can't redirect stdout and/or stderr like I can with op, but perhaps if I wrap the stdout and stderr readers from pty-process in something that can "tag" them I could. I would "forward" the file descriptor check that isatty uses; though, IIRC, it's the FD that the command writes to directly so I don't know if that'll work.

Works:

op run -- target/release/examples/printenv 2> /dev/null

Doesn't work (doesn't redirect):

akv run -- target/release/examples/printenv 2> /dev/null

@heaths heaths changed the title WIP: Use pty_process for PTY xplat test Implement PTY to handle both stdout, stderr with redirects Apr 10, 2025
heaths and others added 12 commits April 10, 2025 00:30
If passed to `/bin/sh -c`, you can make redirects work if you put the whole command in quotes e.g., `akv run -- 'printenv 2> /dev/null'` but that's still pretty hacky.
Here's the winner: check first if either or both of stdout or stderr are redirected. Whichever are, redirect the `Command` streams accordingly.
It almost works but the read end isn't closing when the process terminates like it does when using `pty-process` despite doing basically the same thing.
...or at least `not(windows)`.
Added a custom implementation of the Debug trait for the Pty struct to allow for better formatting of the inner structure. This enhances the debugging experience by providing clearer output.
This feels pretty hacky, though. I effectively need to tie the lifetime of the pty fd to the child process, but it seems unless I write my own `Command` or `Child` wrapper I can't really do that.
Mostly solves the Windows problem, though I still need to refactor POSIX. But even though this flow is recommended, I'm still getting a heap corruption error most times. Can't figure out what's corrupt. Should check AppVerifier.
Fixes heap corruption issue on Windows
@heaths heaths marked this pull request as ready for review April 10, 2025 07:34
@heaths
Copy link
Owner Author

heaths commented Apr 10, 2025

Got this working with my own PTY implementation. Properly detects and shows colors, properly interleaves, and properly redirects stdout and/or stderr.

@heaths heaths enabled auto-merge (squash) April 10, 2025 07:35
@heaths heaths merged commit 45a5df9 into main Apr 10, 2025
4 checks passed
@heaths heaths deleted the pty-process branch April 10, 2025 07:44
@heaths heaths linked an issue Apr 10, 2025 that may be closed by this pull request
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.

TTY not defined when Stdio::piped()
1 participant