add driver for setting up tty file descriptors

[progrium]

with docs! fixes #148

[progrium]

I guess syscall is not available on Windows, duh. I can hardcode a value there I think, right? It doesn't actually matter what the value is there if I recall.

[codecov]

Codecov Report

Merging #232 into master will decrease coverage by 0.21%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
- Coverage   20.88%   20.67%   -0.22%     
==========================================
  Files          17       18       +1     
  Lines        1441     1456      +15     
==========================================
  Hits          301      301              
- Misses       1124     1139      +15     
  Partials       16       16

Impacted Files	Coverage Δ
driver.go	0% <0%> (ø)
tscreen_linux.go	0% <0%> (ø)	⬆️
tscreen.go	0% <0%> (ø)	⬆️
cell.go	73.75% <0%> (+2.66%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86147f1...07af071. Read the comment docs.

[progrium]

Good to go?

[gdamore]

I would prefer this be symbolic, because 0x1c isn't going to be correct on all platforms. This needs to be the signal SIGWINCH. Or just not provided by default, but provided by linux, posix, etc. drivers.

[gdamore]

Perhaps the platform code should be responsible for setting up this signal handler?

[progrium]

Thanks for the feedback! I wish it could be platform code, but we need to not do this in the case of SSH. You're totally right it shouldn't be hardcoded, I was assuming it would only matter if it were the platforms that it was correct for. Maybe we put this in a variable defined in platform code and reference that here?

[gdamore]

We're close, but the hard coded value for SIGWINCH isn't acceptable. This is the correct value on macOS, and probably Linux (not confirmed), but for example it is not correct for illumos (where the value is 20, i.e. 0x14).

[progrium]

Would you like me to push the approach I suggested? Maybe it's easier to see it.

[gdamore]

So I'm sort of confused.

You want to change the "platform" handler for SSH, right?

So the default platform handler for other platforms should bring in the value. Maybe the problem here is what we mean by platform....

To me a platform is Windows, Linux, macOS, etc... these should probably each have a default implementation.

For your SSH case, I'd imagine you would replace this implementation with something that is platform-neutral, right?

If you have to expose this sort of stuff via variables, it's an excellent sign that we haven't captured the correct interface boundary. In that case may be should rethink?

[progrium]

Correct. The needs of SSH don't map cleanly to the platform abstraction here, on top of the fact the platforms are selected using build flags, which SSH can't do. So yes, it's kind of a mess.

Off the top of my head if I recall what I was thinking would best capture platforms and SSH is to replace the platform specific methods entirely and make them all TermDrivers and then each platform file sets the DefaultDriver to its TermDriver.

I didn't do this because I thought a smaller change would be more likely to get in easily. I guess I was wrong.

[progrium]

Tell me how that sounds so that if I decide I want to do it that it won't be a waste of time.

[gdamore]

So right now are selecting the platform via build flags, but I think that's a mistake in hindsight.

I do think instead the build flags should select which default platform is built. Then for example your SSH platform can be swapped out. I'm guessing that this "SSH" platform is really only applicable to Windows, where someone is SSHing into a system running on Windows, because on any other system the underlying platform would provide POSIX semantics with a pty provided by the SSH server.

Thinking about this in more detail, I think the thing that needs to be abstracted is the "terminal" interface, so other folks that might want to provide different backing implementations can. (Imagine something that operates over a REST API for example.)

The terminal interface would need to have the following APIs:

1. Entry points to allocate and destroy the terminal handle
2. Entry point to send bytes to the terminal (raw bytes, including ANSI escapes, etc.)
3. Entry point to poll for bytes from the terminal (maybe blocking API, need to think about this)
4. An entry point to set the tty into "raw" raw mode, and to reset it when done. (maybe same as allocate and destroy?)
5. An entry point to get the window size (optional)
6. A function provided by the "Screen" implementation for the implementation to call when a window change occurs

I also need to think about suspend/resume interactions here. Maybe additional entry points there, I'm not sure.

[progrium]

That all sounds fine. To start it can just wrap and own the in/out file descriptors. I don't think suspend/resume or any of that matters. The minimal version of this that can be iterated on as needed is taking what's there and replacing it with this abstraction. No need to over think it now.

And the SSH driver is not applicable to Windows, in fact, I'm not sure it'll work for Windows because it still depends on POSIX PTY (I don't know if Windows emulates this).

Let me try to re-explain this whole thing. It's very simple: a custom SSH server that does not want to shell out to run a text GUI based on tcell cannot use tcell (or most of these libraries) because they all assume (in some cases hardcode) TTY or STDIO. SSH needs to send it over a socket (and also not handle signals the same way). This is why there was an issue open to have a io.ReadWriteCloser instead of file descriptors. The problem with that is that it wouldn't be enough for the use case they're interested in, which is 100% this SSH use case. We still need to set tty mode etc for it to behave properly. So running it through ptmx (I call pty pair) lets us operate on file descriptors like it already does for POSIX platforms, but we can pipe the other end over the SSH session. So it shares a lot with the POSIX platforms, but can't assume /dev/tty or handle signals, which is why the current PR is the way it is.

I'm willing to take care of this for you and set up an abstraction that can grow as needed, but I need to know we're not going to spend a lot more time bikeshedding or over engineering this. I feel like I understand what you're looking for, and I can mostly give it to you, but while this is an abstract generality to you, it is a concrete need for us (people writing SSH servers in Go).

[gdamore]

I understand.

My thought is that this interface should be simple and clean enough that you can implement what you need (the particulars for SSH) as a sort of "external plugin" (platform driver?) for tcell.

The problem I have with repeated iteration of this is that if this becomes an external interface that people depend upon, then we can't repeatedly break it. It's better to spend a little time up front to get it "right", so that we won't have to bust people's code with follow up changes.

However, we can iterate in an experimental branch, if that is more to your liking. Then people will know that the interface is subject to change.

As you have a concrete need now, and I do not, and I do not have any sponsorship or clients paying for work on tcell at the moment, it's likely that you will be able to put together these changes faster than I will. Otherwise I'll try to find time soon, but if you're working on it I will wait so that we don't double-do.

I have been contemplating larger changes to the input/output loop as well, so having a cleaner API here might be extraordinarily helpful. Other people have a need relating to suspend/resume of tcell, which is something that doesn't work at all right now.

It's likely that if we do this right we can kill several birds with this work.

[progrium]

I see, this touches on stuff you want to do. I feel you. Well if you're willing to take my SSH driver implementation, we can keep the interface private while still letting people use it. The only reason it needed to be public was so I could write an implementation. And I'm pretty highly confident that this is the only weird case scenario people are likely to want to use that isn't just another platform, so it doesn't need to be a public interface.

[gdamore]

So at the moment I'm not sure what it is you're asking for me to agree to integrate?

I don't think I'm ready to accept an "ssh-server-specific platform", as an addition here, but maybe I'd change my mind if I saw what you had in mind. (Partly because I think the problem you want to solve actually is broader in scope than maybe you realize.)

Frankly, I think at this point for your work, we should fork the project (or create a working branch), you can use it for your stuff for now, and then we can work on a real solution that is closer to meeting the needs generally. This would let you proceed, without creating a long term interface promise that I'm likely to regret making later.

(Btw, if you have a commercial need for this, it is possible to pay for me to do this work to accelerate it. If that's something that interests you, send a message to info@staysail.tech.)

[progrium]

Alright, whatever you say.

[zanona]

Go open-source!! 🏁