replace calls to stty
with syscalls #155
No reviewers
Labels
No Label
blocked
bug
build
documentation
duplicate
enhancement
finger
gemini
gopher
help wanted
http
in progress
invalid
local
needs-info
non-code
non-functional
non-urgent
question
release
rendering
suggestion
telnet
terminal
urgent
wontfix
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sloum/bombadillo#155
Loading…
Reference in New Issue
No description provided.
Delete Branch "dancek/bombadillo:remove-stty"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Note that this isn't ready yet and should not be merged. But I wanted to show the code. Any comments are welcome.
Related to #154.
This is the first time I'm writing Go, btw, so I'll also appreciate any pointers on what would be more idiomatic.
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!!!!
I just noticed that the syscall lib also has constants for
VTIME
andVMIN
. 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).Ok. I just pulled this branch and tried to build, but I get the following:
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)
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.
Also, I left some debug prints in this commit as you found out. Oops.
I found the syscall ECHO and ICANON. I have not gotten them to write, but I was hard coding
0x5405
and0x5408
for linux amd64 forTCGETA
andTCSETAF
. I was still getting newlines and echoing, so I'm clearly not setting to the write location. I'll ty outTCSETSF
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
andTCSETS
. 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 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.
Ok. Confirmed. OSX, and likely BSD (I can confirm that via the available consts for BSD in syscall), require
TIOCSETAF
andTIOCGETA
instead. That should not be a big deal and I can create the files to handle the splitting of that.WIP: replace calls to `stty` with syscallsto replace calls to `stty` with syscallsOK! 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.
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!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 intorelease-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!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?).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)
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.
#158 is this same commit against
release-2.3.0
.I'm happy that this was useful :)
Pull request closed