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

proposal: os: File.IsTerminal #28572

Closed
neild opened this issue Nov 3, 2018 · 16 comments
Closed

proposal: os: File.IsTerminal #28572

neild opened this issue Nov 3, 2018 · 16 comments

Comments

@neild
Copy link
Contributor

neild commented Nov 3, 2018

Add an IsTerminal method to os.File, equivalent to the Unix stdlib isatty():

// IsTerminal returns true if the file is associated with a terminal.
func (*File) IsTerminal() bool {}

The github.com/mattn/go-isatty package provides an equivalent function. It has 476 imports and 254 stars, which indicates that there is a fair degree of demand for this feature. I think the standard library should provide it.

@gopherbot gopherbot added this to the Proposal milestone Nov 3, 2018
@ianlancetaylor
Copy link
Contributor

At one time we discussed having a golang.org/x/terminal package to provide some more-or-less generic terminal handling capabilities.

I wonder if os.IsTerminal would be somewhat frustrating in the absence of anything portable you could do with a terminal, such as turning off echo. What would this be used for?

@neild
Copy link
Contributor Author

neild commented Nov 3, 2018

One thing I want it for is to let protoc-gen-go print an error if it's invoked interactively.

There's a progress bar library used fairly extensively inside Google which alters its output depending on whether it's writing to a terminal.

Looking through imports of the existing package would presumably find more uses.

A full terminal package would be nice, but I'd hate to make the perfect the enemy of the good. Plus, not all cases need a full terminal library.

@neild
Copy link
Contributor Author

neild commented Nov 3, 2018

As for what you can do with just this function: \r can get you a surprising way. Assuming the terminal supports the usual ANSI escape sequences if TERM is set and isatty(stderr) is fairly reasonable these days. (And points at why you might want IsTerminal, because ANSI escapes are more portable than a isatty implementation.)

I think it might be possible to write a full curses library using only existing functions in the os package--with the exception of this one.

@mvdan
Copy link
Member

mvdan commented Nov 3, 2018

I agree that just knowing if a file is a terminal can be useful on its own. I currently use https://godoc.org/golang.org/x/crypto/ssh/terminal#IsTerminal, like:

terminal.IsTerminal(int(f.Fd()))

If this is simple enough to add and maintain, my vote is in favor of adding this to the standard library.

@tklauser
Copy link
Member

tklauser commented Nov 5, 2018

If this is simple enough to add and maintain, my vote is in favor of adding this to the standard library.

I think this should be simple enough to add by adding a copy of IoctlGetTermios from x/sys/unix to internal/syscall/unix and add/define the correct TCGET* value per GOOS/GOARCH for IoctlReadTermios. IsTerminal for all unix geese would then be:

import "internal/syscall/unix"

func (f *File) IsTerminal() bool {
        _, err := unix.IoctlGetTermios(f.Fd(), unix.IoctlReadTermios)
        return err == nil
} 

For windows everything that is needed is already in syscall, so IsTerminal would just be:

func (f *File) IsTerminal() bool {
        var st uint32
        err := syscall.GetConsoleMode(syscall.Handle(f.Fd()), &st)
        return err == nil
}  

I'm not sure about plan9. IsTerminal from x/crypto/ssh/terminal is currently:

func IsTerminal(fd int) bool {
        return false
}

@tklauser
Copy link
Member

tklauser commented Nov 6, 2018

I'm not sure about plan9. IsTerminal from x/crypto/ssh/terminal is currently:

func IsTerminal(fd int) bool {
        return false
}

Looking at https://plan9.io/sources/plan9/sys/src/ape/lib/ap/plan9/isatty.c, I think the following might work for plan9:

func (f *File) IsTerminal() bool {
        return strings.HasSuffix(f.name, "/dev/cons") 
}

@rsc
Copy link
Contributor

rsc commented Nov 28, 2018

One thing I want it for is to let protoc-gen-go print an error if it's invoked interactively.

Why not print the error always? The main objection I have to isatty is programs that change behavior based on whether they are run on a tty. If you think protoc-gen-go should be invoked with a pipe you could always check that (os.Stdin.Fstat).

@rsc rsc added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 12, 2018
@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@neild
Copy link
Contributor Author

neild commented Jan 15, 2019

Why not print the error always? The main objection I have to isatty is programs that change behavior based on whether they are run on a tty. If you think protoc-gen-go should be invoked with a pipe you could always check that (os.Stdin.Fstat).

The error in this case is "don't run this interactively", so printing the error always isn't the right thing.

For the case of protoc-gen-go, it might be enough to stat os.Stdin and check for ModeNamedPipe (which I didn't realize is true for non-named pipes as well--very confusing).

Another common use of isatty where that would not, however, suffice is to determine whether to colorize or otherwise add additional formatting to output; in that case, you really do need to distinguish between a terminal and a file/pipe/any other destination.

@neild neild reopened this Jan 15, 2019
@mvdan mvdan removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 15, 2019
@mvdan mvdan reopened this Jan 15, 2019
@mvdan
Copy link
Member

mvdan commented Jan 15, 2019

@gopherbot tsk tsk.

@mvdan
Copy link
Member

mvdan commented Jan 15, 2019

While I generally agree with @rsc that programs shouldn't change their behavior depending on whether they're run interactively, I think developers will find a way to do that in any case. Worse even, they'll pull in x/sys/unix or mattn's small module just to get what would be a small method in the os package.

I think trying to discourage the use of isatty in most cases is understandable, but I don't think it's enough reason to keep it outside of the standard library. Granted that some developers will try to shoot themselves in the foot with an easily accessible IsTerminal, but there are plenty of other ways to do bad things with the standard library. We could always add a warning to the method's godoc.

One use case which I think is reasonable is whether or not to print a prompt in a shell-like program. For example:

$ dash <<EOF
> echo foo
> echo bar
> EOF
foo
bar
$ dash
$ echo foo
foo
$ echo bar
bar

I maintain such a program, and it's a shame that the only non-std dependency I pull is golang.org/x/crypto/ssh/terminal, just for this reason.

@rsc
Copy link
Contributor

rsc commented Feb 6, 2019

We looked for a little while at maybe having a FileInfo mode bit, but the bit seems expensive to calculate. For this:

One thing I want it for is to let protoc-gen-go print an error if it's invoked interactively.

Would it suffice to stat os.Stdin and check for ModeCharDevice?

@neild
Copy link
Contributor Author

neild commented Feb 6, 2019

Would it suffice to stat os.Stdin and check for ModeCharDevice?

That's definitely not the same thing as isatty(), but maybe it suffices on Unix systems? Probably not anywhere else, though.

tklauser added a commit to tklauser/go that referenced this issue Mar 18, 2019
Just a proof of concept implementation for golang#28572, proposal needs to be
decided first.

DO NOT REVIEW
DO NOT SUBMIT

Change-Id: I360c6a01650629e938f9454cb5180f45c67b82b7
@andybons
Copy link
Member

Per discussion with @golang/proposal-review, if you want this function, it's likely you want much more (color libraries, ncurses helpers, etc.) and in that case you should rely on those packages to do this for you. It's unusual to want isatty alone, so it seems hard to justify adding just this to the stdlib.

@cespare
Copy link
Contributor

cespare commented Mar 27, 2019

Per discussion with @golang/proposal-review, if you want this function, it's likely you want much more (color libraries, ncurses helpers, etc.)

Did anyone actually check this empirically? Looking at my own work, I've used IsTerminal for about 8 different tools and only one of them uses any kind of auxiliary terminal package (for readline functionality).

Edit: additionally, those all have a dependency on golang.org/x/crypto/ssh/terminal which they otherwise wouldn't need, which seems silly.

@robpike
Copy link
Contributor

robpike commented Mar 27, 2019

I have a philosophical objection to most uses of isatty. For example, ls changes its output whether it's writing to a pipe or not, which means you can't see what its output looks like to a pipe by running it on a terminal. One learns this eventually, but I was offended by the property when BSD did this, and it remains surprising.

I doubt many share my view.

@golang golang locked and limited conversation to collaborators Mar 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants