Skip to content
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

Support usage in scenarios where stdin / stdout are pipes (but stderr is a TTY) #129

Open
cipriancraciun opened this issue Apr 12, 2020 · 6 comments

Comments

@cipriancraciun
Copy link

Usually in almost all circumstances /dev/sdterr is a TTY; meanwhile /dev/stdin and/or /dev/stdout can be redirected to files or pipes. (In fact if /dev/stderr is not a TTY, most likely the other two are inherited or poorly set-up, and this process isn't meant to handle the terminal.)

At the moment liner checks if any of stdin or stdout are redirected, and if so degrades to a "dumb" terminal:

  • liner/input.go

    Lines 38 to 49 in abb5e9d

    s.terminalSupported = TerminalSupported()
    if m, err := TerminalMode(); err == nil {
    s.origMode = *m.(*termios)
    } else {
    s.inputRedirected = true
    }
    if _, err := getMode(syscall.Stdout); err != 0 {
    s.outputRedirected = true
    }
    if s.inputRedirected && s.outputRedirected {
    s.terminalSupported = false
    }

My proposal (which I could implement if accepted) is to either:

  • (preferably) always use /dev/stderr for both input and output and check that itself is a TTY;
  • alternatively if either stdin or stdout seems to be redirected, check if perhaps stderr is a TTY and use that;

I currently use liner in such a scenario, and for the moment I replace syscall.Stdin / syscall.Stdout with syscall.Stderr and it seems to do the trick. However this hack works in my limited case, but as said I'm willing to code this proposed feature.

@peterh
Copy link
Owner

peterh commented Apr 14, 2020

Usually when stdout is redirected, the user does not want a command prompt on stderr. I'm not likely to make that the default behaviour.

@cipriancraciun
Copy link
Author

Take for example the following simple use-case where my-tool asks for an input, does something, and prints out the result; however the user wants to catch that in a variable (as part of a larger script):

VAR="$( ./my-tool )"

At the moment, the user stares at a blank screen, because liner writes the prompt to stdout, but waits inputs from stdin. After I end my input, the value of VAR actually contains >> some-input \n some-output (i.e. some-input is what I entered, >> is the prompter, and some-output is the actual thing it should be in there.)

Similar examples could be:

dump-some-db | ./my-encryption-tool >./some-db.enc

At the moment liner is broken even for these simple use-cases.

Now regarding my proposal of always using stderr as input and output and check if that is an actual TTY, would work in all these cases, and I can't think of any draw-backs. (Except as said when stderr is redirected to some file, but in that case, stdout is most likely redirected too.)

@peterh
Copy link
Owner

peterh commented Apr 19, 2020

Sorry, you can't use stderr as input. It's output only. So your dump-some-db | ./my-liner-tool | ./new.db example can only work if the tool has a non-interactive mode.

There may be a way to attach input to the current TTY (looked up from stderr) instead of using stdin, but that's outside of the scope of liner (which is trying to be both as simple and as cross-platform as possible)

@peterh
Copy link
Owner

peterh commented Apr 19, 2020

...I thought I left the following comment earlier, but I must have forgotten to press the Comment button:

In addition to the above, your suggested change breaks anyone who has a workflow of "./my-liner-tool < command_list > output". Even if I wanted to make this change, I cannot change behaviour that wildly without also changing the package name (or major version number, same thing).

@cipriancraciun
Copy link
Author

cipriancraciun commented Apr 19, 2020

Sorry, you can't use stderr as input. It's output only. So your dump-some-db | ./my-liner-tool | ./new.db example can only work if the tool has a non-interactive mode.

Yes you can. I have already done this for my usage of liner:

https://github.com/cipriancraciun/z-run/blob/master/sources/lib/input.go#L52-L58

The basic idea is this: if the process is attached to a TTY then, unless the process is part of a pipeline, all stdin, stdout and stderr are actually dup of /dev/tty, and thus you can technically use any of them to read / write. In case it is part of a pipeline, then stderr usually remains the only descriptor pointing to /dev/tty, thus can be used for both reading and writing.

For example running lsof -p $$ (i.e. the open file descriptors for the current shell) yields:

bash    9534 ciprian    0u   CHR  136,5      0t0       8 /dev/pts/5
bash    9534 ciprian    1u   CHR  136,5      0t0       8 /dev/pts/5
bash    9534 ciprian    2u   CHR  136,5      0t0       8 /dev/pts/5

There may be a way to attach input to the current TTY (looked up from stderr) instead of using stdin, but that's outside of the scope of liner (which is trying to be both as simple and as cross-platform as possible)

Yes, that can be achieved by opening /dev/tty, which is tied to the TTY of the process group the current process is part of. However as you've noted if /dev/stderr is not a TTY, then the user really doesn't want it to interact with the console, thus in such a case an error should be issued.


In addition to the above, your suggested change breaks anyone who has a workflow of "./my-liner-tool < command_list > output". Even if I wanted to make this change, I cannot change behaviour that wildly without also changing the package name (or major version number, same thing).

I agree that backward compatibility should be kept. Thus perhaps a new constructor could be added that takes the input / output streams to be used. (And perhaps a wrapper that supplies /dev/stderr if it is a TTY or errors out.)

@peterh
Copy link
Owner

peterh commented Apr 20, 2020

I agree that backward compatibility should be kept. Thus perhaps a new constructor could be added that takes the input / output streams to be used.

This seems like a reasonable suggestion.

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

No branches or pull requests

2 participants