replace calls to stty with syscalls #155

dancek · 2020-05-23T08:53:31Z

dancek commented

2020-05-23 08:53:31 +00:00

Note that this isn't ready yet and should not be merged. But I wanted to show the code. Any comments are welcome.

dancek commented

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

Related to #154.

dancek commented

2020-05-23 08:55:40 +00:00

This is the first time I'm writing Go, btw, so I'll also appreciate any pointers on what would be more idiomatic.

sloum commented

2020-05-23 15:40:30 +00:00

Wow! This is way simpler than I thought it would be. There is indeed some idiomatic cleanup that could be done, but it certainly isnt a big deal.

We can use preprocessor build commands to build out files with the correct constants and attribute naming that will only build when building for the given platform. So that would ease using this solution.

I have not tried this on a mac yet, but I have one avialable to me. Once my daughter goes down for a nap I'll pull it and verify (I, of course, trust that it works... but it is always good to have another set of eyes).

I'm going to take a look at the CGO implementation(s) you did and come back here and comment for comparrison. Or maybe I'll go tot he issue card and comment there. In either case: Thank you SO MUCH for doing all of this!!!!

Wow! This is way simpler than I thought it would be. There is indeed some idiomatic cleanup that could be done, but it certainly isnt a big deal. We can use preprocessor build commands to build out files with the correct constants and attribute naming that will only build when building for the given platform. So that would ease using this solution. I have not tried this on a mac yet, but I have one avialable to me. Once my daughter goes down for a nap I'll pull it and verify (I, of course, trust that it works... but it is always good to have another set of eyes). I'm going to take a look at the CGO implementation(s) you did and come back here and comment for comparrison. Or maybe I'll go tot he issue card and comment there. In either case: Thank you SO MUCH for doing all of this!!!!

sloum commented

2020-05-23 16:03:51 +00:00

I just noticed that the syscall lib also has constants for VTIME and VMIN. I dont think it should be combined into this, but if we go with the pure go route we could eventually set up a read timeout. That would allow us to capture control keys and other multibyte sequences, eventually allowing control by arrow keys (which some users would like over the vi keys, even if they wouldnt be something I would use).

I just noticed that the syscall lib also has constants for `VTIME` and `VMIN`. I dont think it should be combined into this, but if we go with the pure go route we could eventually set up a read timeout. That would allow us to capture control keys and other multibyte sequences, eventually allowing control by arrow keys (which some users would like over the vi keys, even if they wouldnt be something I would use).

sloum commented

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

Ok. I just pulled this branch and tried to build, but I get the following:

termios/termios.go:46:12: undefined: syscall.TIOCGETA
termios/termios.go:51:12: undefined: syscall.TIOCSETAF

Based on your comments I think this is expected behavior on Linux. That said, it is strange that the docs do not mention this.

Looking at the syscall docs here I do not see constants for any of the following:

TIOCGETA
TIOCSETAF
TCGETA
TCSETAF

Am I missing something or are the docs? Did you run into similar issues when working out the Darwin way of doing this (which upon further reading seems like may work for BSD)

Ok. I just pulled this branch and tried to build, but I get the following: ``` termios/termios.go:46:12: undefined: syscall.TIOCGETA termios/termios.go:51:12: undefined: syscall.TIOCSETAF ``` Based on your comments I think this is expected behavior on Linux. That said, it is strange that the docs do not mention this. Looking at the syscall docs [here](https://golang.org/pkg/syscall/) I do not see constants for any of the following: - `TIOCGETA` - `TIOCSETAF` - `TCGETA` - `TCSETAF` Am I missing something or are the docs? Did you run into similar issues when working out the Darwin way of doing this (which upon further reading seems like may work for BSD)

dancek commented

2020-05-23 18:41:02 +00:00

I pretty much looked at what the term package does in term/termios and wrote that without the dependencies. Now that I look at it, they also define constants on Linux. And they use TCGETS and TCSETSF instead of the A versions as I expected. Not sure about the differences, it's best to check the POSIX spec or wherever these constants are defined.

There seem to be TCGETS and TCSETS for linux in syscall, too. https://golang.org/src/syscall/ztypes_linux_amd64.go

And my best find: syscall includes ECHO, ICANON and such. So this might be possible without hardcoding any constants per platform, if there's a way to make linux work with the provided ones.

I pretty much looked at what the term package does in [`term/termios`](https://github.com/pkg/term/tree/master/termios) and wrote that without the dependencies. Now that I look at it, they also define constants on Linux. And they use TCGETS and TCSETSF instead of the A versions as I expected. Not sure about the differences, it's best to check the POSIX spec or wherever these constants are defined. There seem to be TCGETS and TCSETS for linux in syscall, too. https://golang.org/src/syscall/ztypes_linux_amd64.go And my best find: syscall includes ECHO, ICANON and such. So this might be possible without hardcoding any constants per platform, if there's a way to make linux work with the provided ones.

dancek commented

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

Also, I left some debug prints in this commit as you found out. Oops.

sloum commented

2020-05-23 18:53:05 +00:00

I found the syscall ECHO and ICANON. I have not gotten them to write, but I was hard coding 0x5405 and 0x5408 for linux amd64 for TCGETA and TCSETAF. I was still getting newlines and echoing, so I'm clearly not setting to the write location. I'll ty out TCSETSF and see if I have any better luck. Man syscall is a swamp of poor documentation. I have been referencing the unix package here for some constants and such.

I found the prints you left in right after I sent the message but figured I had already bombed you with a bunch of comments so I may as well not write just to let you know I figured it out.

EDIT: I just tried out TCGETS and TCSETS. Bingo! That works. This is an awesome fix. I think there will need to be some testing in different environments (once we work out the compiler directives for which CONSTs to use when) just to make sure this is stable accross platforms... but I am pretty psyched on this.

I found the syscall ECHO and ICANON. I have not gotten them to write, but I was hard coding `0x5405` and `0x5408` for linux amd64 for `TCGETA` and `TCSETAF`. I was still getting newlines and echoing, so I'm clearly not setting to the write location. I'll ty out `TCSETSF` and see if I have any better luck. Man syscall is a swamp of poor documentation. I have been referencing the unix package [here](https://godoc.org/golang.org/x/sys/unix?GOOS=linux&GOARCH=amd64) for some constants and such. I found the prints you left in right after I sent the message but figured I had already bombed you with a bunch of comments so I may as well not write just to let you know I figured it out. **EDIT**: I just tried out `TCGETS` and `TCSETS`. Bingo! That works. This is an awesome fix. I think there will need to be some testing in different environments (once we work out the compiler directives for which CONSTs to use when) just to make sure this is stable accross platforms... but I am pretty psyched on this.

sloum commented

2020-05-23 19:07:42 +00:00

I have a branch up called replace-stty-linux. It works for me really well. It does not use any hard coded values in the Bombadillo code (just constants provided by the go standard lib). I also noticed somehow that the code for hiding the cursor had been removed so I reintroduced it in a logical place.

If you have a linux box and want to try the branch feel free to pull and let me know. I will try it on a mac as well and see if it just works or if splitting the attribute that gets set between the platforms is required.

If anyone reading this has a non-darwin BSD setup available to them and wants to test this it would be very much appreciated.

I have a branch up called `replace-stty-linux`. It works for me really well. It does not use any hard coded values in the Bombadillo code (just constants provided by the go standard lib). I also noticed somehow that the code for hiding the cursor had been removed so I reintroduced it in a logical place. If you have a linux box and want to try the branch feel free to pull and let me know. I will try it on a mac as well and see if it just works or if splitting the attribute that gets set between the platforms is required. If anyone reading this has a non-darwin BSD setup available to them and wants to test this it would be very much appreciated.

sloum commented

2020-05-23 19:13:34 +00:00

Ok. Confirmed. OSX, and likely BSD (I can confirm that via the available consts for BSD in syscall), require TIOCSETAF and TIOCGETA instead. That should not be a big deal and I can create the files to handle the splitting of that.

Ok. Confirmed. OSX, and likely BSD (I can confirm that via the available consts for BSD in syscall), require `TIOCSETAF` and `TIOCGETA` instead. That should not be a big deal and I can create the files to handle the splitting of that.

dancek changed title from ~~WIP: replace calls to `stty` with syscalls~~ to replace calls to `stty` with syscalls

2020-05-23 22:12:16 +00:00

dancek commented

2020-05-23 22:16:27 +00:00

OK! I took your edits and added two sets of consts for Linux and Darwin/BSD/others.

This version looks good enough IMHO. but I don't have a Linux machine handy right now so I've only tested on MacOS.

Hope you don't mind me squashing your changes into the same commit. I'm used to working with Gerrit, always rewriting commits until the reviewers are happy. Now that I'm writing this I realize not all people like that.

OK! I took your edits and added two sets of consts for Linux and Darwin/BSD/others. This version looks good enough IMHO. but I don't have a Linux machine handy right now so I've only tested on MacOS. Hope you don't mind me squashing your changes into the same commit. I'm used to working with Gerrit, always rewriting commits until the reviewers are happy. Now that I'm writing this I realize not all people like that.

sloum commented

2020-05-23 22:45:30 +00:00

I'm usually against squash commits and prefer merge, but we hadn't talked about it. I'm not too worried about it in this instance.

I really appreciate all the help on this! I'll be able to test on my Linux machine in just a bit. I have an open release PR that, if this gets vetted enough, I would love to have this go into and be a part of this weekend's release (2.3.0). Removing the stty and tpu dependencies have been on my mind for awhile and I am really excited that this looks like it is going to work out!

I'm usually against squash commits and prefer merge, but we hadn't talked about it. I'm not too worried about it in this instance. I really appreciate all the help on this! I'll be able to test on my Linux machine in just a bit. I have an open release PR that, if this gets vetted enough, I would love to have this go into and be a part of this weekend's release (`2.3.0`). Removing the stty and tpu dependencies have been on my mind for awhile and I am really excited that this looks like it is going to work out!

sloum commented

2020-05-23 23:20:18 +00:00

Ok. This is confirmed as functional on two Linux systems (Ubuntu and Arch). I feel confident that the syscalls should work on any distro. I also built and ran it on OSX and had no issues there.

We use a branching model that uses develop as a proto-release kidna beta branch and anything that gets merged into it increments the version a minor version level. Since I have a release branch set up with a PR currently would you mind closing this PR and resubmitting a PR into release-2.3.0? Sorry for the pain in the butt on that one. And again, thank you so so much for your work on this. I'm really happy this was able to work out without directly using CGO or adding outside dependencies!

Ok. This is confirmed as functional on two Linux systems (Ubuntu and Arch). I feel confident that the syscalls should work on any distro. I also built and ran it on OSX and had no issues there. We use a branching model that uses `develop` as a proto-release kidna beta branch and anything that gets merged into it increments the version a minor version level. Since I have a release branch set up with a PR currently would you mind closing this PR and resubmitting a PR into `release-2.3.0`? Sorry for the pain in the butt on that one. And again, thank you so so much for your work on this. I'm really happy this was able to work out without directly using CGO or adding outside dependencies!

sloum referenced this pull request

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

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

sloum referenced this pull request

2020-05-23 23:27:21 +00:00

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

asdf commented

2020-05-24 03:38:30 +00:00

Tested this on my system (Ubuntu 20.04) and it seems to work pretty well! No issues I could see.

make test is fine, however vim-go's GoMetaLinter is picking up the exported functions that do not have comments (although it seems there are others like this too now so this might now be a big deal?).

Tested this on my system (Ubuntu 20.04) and it seems to work pretty well! No issues I could see. `make test` is fine, however vim-go's GoMetaLinter is picking up the exported functions that do not have comments (although it seems there are others like this too now so this might now be a big deal?).

sloum commented

2020-05-24 05:10:31 +00:00

I think for the moment in the interest of getting much needed bug patches to all the new gemini users I am ok with the GoMetaLinter comment violations for now, with the idea of coming back afterward and adding comments to the new package (and any other missing comments)

sloum commented

2020-05-24 06:12:52 +00:00

Also, I just put FreeBSD on my thinkpad and built this branch with few issues. The replacement of stty with syscalls works great.

Our makefile does not work at all on FreeBSD. So that is something to look into. I'll add an issue tomorrow. I'm crashing out for the night now.

Also, I just put FreeBSD on my thinkpad and built this branch with few issues. The replacement of stty with syscalls works great. Our makefile does not work at all on FreeBSD. So that is something to look into. I'll add an issue tomorrow. I'm crashing out for the night now.

dancek referenced this pull request

2020-05-24 06:29:07 +00:00

Replace calls to `stty` with syscalls #158

dancek commented

2020-05-24 06:30:15 +00:00

#158 is this same commit against release-2.3.0.

I'm happy that this was useful :)

#158 is this same commit against `release-2.3.0`. I'm happy that this was useful :)

👍 1 🎉 1

dancek closed this pull request

2020-05-24 06:30:18 +00:00