-
Notifications
You must be signed in to change notification settings - Fork 25
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
stdin
broken since release 0.2.3
#63
Comments
I've written a snippet to demonstrate the problem: fn main() {
let sh = xshell::Shell::new().unwrap();
let cmd = xshell::cmd!(sh, "test -t 0");
let result = cmd.run();
println!("test if stdin is open and associated with a terminal: {result:?}");
let cmd = xshell::cmd!(sh, "cat");
let result = cmd.run();
println!("{result:?}");
} xshell 0.2.3, doesn't wait for input and gives:
xshell 0.2.2, does wait for input and outputs:
As expected I had to All functions using |
Stdin is intentionally not inherited by default. xshell is primarily oriented towards non-interactive usage in scripts or larger programs, and there accidentally inheriting stdin might cause a latent bug. But adding a method to control this behavior, to enable inheriting of stdin, would be useful! There's also a bigger issue that our overall semantics with inheriting/dev/nulling standard streams seems alll over the place. Eg, the So, it seems like it might make sense to overhaul the overall streams behavior here, to make it more predicatble and robust. The following would make sense to me:
The
We might actually extend this split to non-run commands like let pkgid = cmd!(sh, "cargo pkgid").read()?; In my CI, I'd probably would love to both:
This requires some fancy tee-ing of the stdout. This large overhaul would require 0.3 of course. Toggling just stding inheritance would be a minor patch though! |
Some programs use whether the stdin is a tty to decide whether to turn on color output. I had to stay on 0.2.2 for this. |
It would be great if this could be added with a simple method. |
Chiming in to mention this is important for e.g. CLIs like |
I've since learned that checking for empty While you do generally expect For example, it might turn out that
stderr printouts from that version manager. |
Since
0.2.3
therun
function now usesoutput_impl
andoutput_impl
is missing the option to allowstdin
to beStdio::inherit()
, which I guess should have been the default behaviour. In practice this breaks interaction with commands being executed byrun
.I think https://github.com/matklad/xshell/blob/master/src/lib.rs#L1011-L1014 should account for the case where one wants
stdin
to be inherited. So possiblyoutput_impl
also needs aignore_stdin
argument to switchstdin
toStdio::none()
.If you want I can write a piece of code to demonstrate the bug.The text was updated successfully, but these errors were encountered: