Using stty is not a reliable or portable way to control the terminal state #154

New Issue

dancek · 2020-05-22T15:19:35Z

dancek commented

2020-05-22 15:19:35 +00:00

Hi!

On my machine, bombadillo exits when I try to enter command mode. I debugged the root cause down to the stty system command that happens in cui.SetLineMode. This is somehow because I have GNU Coreutils in my PATH before the BSD system utils, on MacOS 10.14.3 (ie. not the latest version). The stty works for me otherwise, so I'm not planning on investigating any further.

Now, I don't expect you to fix this. But I might try replacing all stty calls by tcsetattr/tcgetattr calls (termios) or perhaps some higher-level library. I've done something similar in C before. Would you be interested in a pull request?

Hi! On my machine, bombadillo exits when I try to enter command mode. I debugged the root cause down to the `stty` system command that happens in `cui.SetLineMode`. This is somehow because I have GNU Coreutils in my PATH before the BSD system utils, on MacOS 10.14.3 (ie. not the latest version). The `stty` works for me otherwise, so I'm not planning on investigating any further. Now, I don't expect you to fix this. But I might try replacing all `stty` calls by `tcsetattr`/`tcgetattr` calls ([termios](https://godoc.org/github.com/pkg/term/termios)) or perhaps some higher-level library. I've done something similar in C before. Would you be interested in a pull request?

sloum commented

2020-05-22 16:16:52 +00:00

Hi there :)

Thank you for bringing this to our attention. I'm not exactly sure I understand the setup or why it works out the way it does (with Bombadillo exiting). That said, I am bummed that it is happening and I definitely appreciate you going through the code to find the issue!

The terminal handling has been a problem since the beginning. I am deeply disappointed by the lack of inclusion of decent terminal handling in the Go core library. For Bombadillo I have had a policy of no outside dependencies, at least for the version we are currently on and before (the 0.x.x - 2.x.x series). I could potentially be convinced to bring something in for the major rewrite that will eventually be 3.0.0 (where I plan to do a lot more coroutines and channels to pass messages between different components - so it will be a big structural shift anyway).

I am open to a pull right now only if it can be done without external dependencies. There may be a possibility to using the, admittedly arcane and messy, syscall lib to achieve the end goal... but would be a monster of a task and I certainly dont expect anyone to take that on. A drop into CGO to use C's termios.h could also work (as it is pretty portable accross our target platforms)... but that also seems like a messy route that would change the build system a bit too.

If you have any ideas that could make this more robust without a user having to fetch libs (or have their build system do so) I would definitely be open to them (maybe there is a fallback subprocess that can be run if an issue is encountered?). I recognize this as a problem area for Bombadillo and in the long run something will have to shift, I would just like to put it off to version 3 if possible.

I'm sure you know already, but if you want to replace that panic on your local version or just ignore the error (in both SetLineMode and SetCharMode) then you would always be in line mode. So you'd have to hit enter after every command... which would be annoying, but at least it would run.

Sorry to have not been more help. I will definitely include a suggestion to use the termios go lib for version 3 when we start planning that release (an issue card should be popping up soon for discussion).

Hi there :) Thank you for bringing this to our attention. I'm not exactly sure I understand the setup or why it works out the way it does (with Bombadillo exiting). That said, I am bummed that it is happening and I definitely appreciate you going through the code to find the issue! The terminal handling has been a problem since the beginning. I am deeply disappointed by the lack of inclusion of decent terminal handling in the Go core library. For Bombadillo I have had a policy of no outside dependencies, at least for the version we are currently on and before (the 0.x.x - 2.x.x series). I could potentially be convinced to bring something in for the major rewrite that will eventually be 3.0.0 (where I plan to do a lot more coroutines and channels to pass messages between different components - so it will be a big structural shift anyway). I am open to a pull right now only if it can be done without external dependencies. There may be a possibility to using the, admittedly arcane and messy, syscall lib to achieve the end goal... but would be a monster of a task and I certainly dont expect anyone to take that on. A drop into CGO to use C's termios.h could also work (as it is pretty portable accross our target platforms)... but that also seems like a messy route that would change the build system a bit too. If you have any ideas that could make this more robust without a user having to fetch libs (or have their build system do so) I would definitely be open to them (maybe there is a fallback subprocess that can be run if an issue is encountered?). I recognize this as a problem area for Bombadillo and in the long run something will have to shift, I would just like to put it off to version 3 if possible. I'm sure you know already, but if you want to replace that panic on your local version or just ignore the error (in both SetLineMode and SetCharMode) then you would always be in line mode. So you'd have to hit enter after every command... which would be annoying, but at least it would run. Sorry to have not been more help. I will definitely include a suggestion to use the termios go lib for version 3 when we start planning that release (an issue card should be popping up soon for discussion).

sloum commented

2020-05-22 16:18:51 +00:00

@asdf What do you think here? I know I have been really stubborn about the "no outside dependencies" thing. I want someone that does not know Go or even programming at all to be able to download the Go compiler and build this with no issues and I see dependency management as a problem for that. But am I being too stubborn? Does my statement above sound right to you? Or should we consider wavering on that in this case? I'd love your input.

sloum added the

bug

label 2020-05-22 16:19:33 +00:00

sloum added the

terminal

label 2020-05-22 16:20:36 +00:00

sloum added this to the 3.0.0 milestone 2020-05-22 16:24:00 +00:00

dancek commented

2020-05-22 18:12:27 +00:00

I'm not very familiar with Go, but I'll investigate some alternatives a bit. It's good to know your stance on dependencies. I don't know what the ecosystem in Go is like, but I've seen difficult build systems before and really appreciate your trying to be user friendly.

As it is, I can work around the issue by running with PATH=/bin bombadillo so it's not critical.

I'm not very familiar with Go, but I'll investigate some alternatives a bit. It's good to know your stance on dependencies. I don't know what the ecosystem in Go is like, but I've seen difficult build systems before and really appreciate your trying to be user friendly. As it is, I can work around the issue by running with `PATH=/bin bombadillo` so it's not critical.

dancek commented

2020-05-22 18:14:31 +00:00

Oh, I see you've written a kilo-inspired text editor too. So you probably know at least as much as I do about terminal states. :)

I'll still think about this for a bit.

Oh, I see you've written a kilo-inspired text editor too. So you probably know at least as much as I do about terminal states. :) I'll still think about this for a bit.

👍 1

sloum commented

2020-05-22 21:47:51 +00:00

Cool. Definitely let me know if you find anything. I believe I saw some chatter a long time ago about being able to use syscalls to accomplish this, but it is beyond my knowledge and skill level. I'm glad you have a work-around in the interim!

Yeah! Kilo was a cool base to work from. I have not done a ton of C programming so it was a way to get a little bit of experience in C in a medium size codebase without having to start from nothing. The terminal stuff in particular was really useful.

Cool. Definitely let me know if you find anything. I believe I saw some chatter a long time ago about being able to use syscalls to accomplish this, but it is beyond my knowledge and skill level. I'm glad you have a work-around in the interim! Yeah! Kilo was a cool base to work from. I have not done a _ton_ of C programming so it was a way to get a little bit of experience in C in a medium size codebase without having to start from nothing. The terminal stuff in particular was really useful.

sloum changed title from ~~Command mode is broken (on an esoteric setup)~~ to Using stty is not a reliable or portable way to control the terminal state

2020-05-22 21:48:37 +00:00

sloum commented

2020-05-22 21:49:22 +00:00

I think since I tagged this with a 3.0.0 milestone this issue will be around for awhile. I have changed the title to reflect the root issue of using stty for terminal control.

asdf commented

2020-05-23 06:17:12 +00:00

Thanks @dancek for all your work on this one.

Do I understand this correctly? We use external programs like stty and tput to manage the console state because there is no built in library that does this. These tools work, but are brittle in situations like this (but it might not be a common situation?). We would like to improve this, but we need to make some decisions about how and when this is done.

Firstly, this issue seems like it can withstand some delay because of the workaround. It seems like a big enough change to require some time testing, and having it on the cards for version 3 would allow for this.

Regarding dependencies on external libraries, it is worth considering because we are being limited by features not available in the standard library. With that said, I agree with the current approach. This case though looks to me like we already have an external dependency which is less than ideal. Some points on this and how it could be done well:

stty is already somewhat an external dependency. We currently rely on the user's system having stty installed and working in a certain way. Using a library would mean addressing how this dependency is managed, and ensure it works in the way we want it. This would be an improvement.
While we could write a new library to replace stty, using an existing library would mean we could spend more time on other features within bombadillo. Either is an option though.
Regarding ease of use, there are some approaches that might ensure we maintain the current install experience:
- The makefile could be used to seamlessly manage dependencies on a well written and tested library.
- The modules system might be an option as well.
- The library, or the parts of it we need, could be included in the repo.

Thanks @dancek for all your work on this one. Do I understand this correctly? We use external programs like stty and tput to manage the console state because there is no built in library that does this. These tools work, but are brittle in situations like this (but it might not be a common situation?). We would like to improve this, but we need to make some decisions about how and when this is done. Firstly, this issue seems like it can withstand some delay because of the workaround. It seems like a big enough change to require some time testing, and having it on the cards for version 3 would allow for this. Regarding dependencies on external libraries, it is worth considering because we are being limited by features not available in the standard library. With that said, I agree with the current approach. This case though looks to me like we already have an external dependency which is less than ideal. Some points on this and how it could be done well: - stty is already somewhat an external dependency. We currently rely on the user's system having stty installed and working in a certain way. Using a library would mean addressing how this dependency is managed, and ensure it works in the way we want it. This would be an improvement. - While we could write a new library to replace stty, using an existing library would mean we could spend more time on other features within bombadillo. Either is an option though. - Regarding ease of use, there are some approaches that might ensure we maintain the current install experience: - The makefile could be used to seamlessly manage dependencies on a well written and tested library. - The modules system might be an option as well. - The library, or the parts of it we need, could be included in the repo.

dancek commented

2020-05-23 08:51:01 +00:00

I have a working implementation on MacOS using syscall. It'll need a bit of work to support other platforms, though.

Let's see if I know how to make a PR on Gitea.

I have a working implementation on MacOS using `syscall`. It'll need a bit of work to support other platforms, though. Let's see if I know how to make a PR on Gitea.

dancek referenced this issue

2020-05-23 08:54:16 +00:00

replace calls to `stty` with syscalls #155

dancek commented

2020-05-23 11:24:18 +00:00

Oh, I didn't see the comment by @asdf when I submitted my last comment and the PR.

My commit isn't really about reimplementing stty, it's just replicating the relevant parts of termios.h (basically a couple of constants) and tcgetattr+tcsetattr. In C all this would have been a couple of standard library calls. There are slight variations on different platforms, but IMHO should be manageable. We'll just have to implement all the platforms we want once and they will work forever.

So basically I've read and reimplemented minor parts of the unix and term Go packages and copied the ECHO and ICANON constants from my local termios.h. (Note that cbreak in stty is the inverse of ICANON.)

I haven't looked at tput yet.

Oh, I didn't see the comment by @asdf when I submitted my last comment and the PR. My commit isn't really about reimplementing `stty`, it's just replicating the relevant parts of `termios.h` (basically a couple of constants) and `tcgetattr`+`tcsetattr`. In C all this would have been a couple of standard library calls. There are slight variations on different platforms, but IMHO should be manageable. We'll just have to implement all the platforms we want once and they will work forever. So basically I've read and reimplemented minor parts of the `unix` and `term` Go packages and copied the ECHO and ICANON constants from my local `termios.h`. (Note that `cbreak` in `stty` is the inverse of `ICANON`.) I haven't looked at `tput` yet.

dancek referenced a pull request that will close this issue

2020-05-23 13:07:40 +00:00

Replace calls to `stty` with <termios.h> cgo #156

dancek commented

2020-05-23 13:08:36 +00:00

I took another stab with cgo and it seems to be the better, easier way.

sloum commented

2020-05-23 23:23:04 +00:00

For those reading this issue that have not been following PR #155 it has been worked out via the syscall module to get this to happen! There is a compiletime branch to build for Linux vs everything else, but other than that it looks really good.

If anyone wants to try building on a BSD that would be great. Heck, I may remove Haiku from my second laptop and put GhostBSD back on just to try building this :)

I am hopeful that this will make it into the 2.3.0 release (and that that release will hopefully happen this weekend - it is a long weekend here, so I've got the extra time to shepherd it through and update the website and gopher).

For those reading this issue that have not been following PR #155 it has been worked out via the `syscall` module to get this to happen! There is a compiletime branch to build for Linux vs everything else, but other than that it looks really good. If anyone wants to try building on a BSD that would be great. Heck, I may remove Haiku from my second laptop and put GhostBSD back on just to try building this :) I am hopeful that this will make it into the `2.3.0` release (and that that release will hopefully happen this weekend - it is a long weekend here, so I've got the extra time to shepherd it through and update the website and gopher).

sloum removed this from the 3.0.0 milestone 2020-05-23 23:27:54 +00:00

asdf commented

2020-05-24 03:47:54 +00:00

Wow, this is all very cool. Great work everyone! Thanks @dancek for working this one through.

👍 1

sloum added the

in progress

label 2020-05-24 05:10:53 +00:00

sloum commented

2020-05-24 17:00:55 +00:00

I second that. Good group effort (though most credit to @dancek) getting this pushed through. I have completed the big release to master for version 2.3.0 with #160
I am closing this issue as a result of the fix being merged in and verified on all three platforms we officially support.

I will open a new issue with a focus on finding a way to remove tput as a dependency in some future version.

I second that. Good group effort (though most credit to @dancek) getting this pushed through. I have completed the big release to master for version 2.3.0 with #160 I am closing this issue as a result of the fix being merged in and verified on all three platforms we officially support. I will open a new issue with a focus on finding a way to remove `tput` as a dependency in some future version.

sloum removed the

in progress

label 2020-05-24 17:01:07 +00:00

sloum closed this issue

2020-05-24 17:01:12 +00:00

Sign in to join this conversation.