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

Use unix.Ioctl* on BSD systems, deduplicate tscreen_*.go #29

Closed
wants to merge 2 commits into from

Conversation

niten94
Copy link

@niten94 niten94 commented Dec 7, 2024

The behavior that occurs when calling syscall.Syscall and passing syscall.SYS_IOCTL in OpenBSD has been changed in Go 1.23 because syscall(2) is removed in OpenBSD 7.5. There may be changes in any platform where using syscall.Syscall* will not work, so Syscall6 is replaced with golang.org/x/sys/unix.Ioctl* in this pull request to partly address zyedidia/micro#3557.

There may be fields in Termios added that have to be copied, so the whole struct is also copied before modifying it in this pull request. The serial TTY session in my OpenBSD VM always crashed when I compiled with the original tscreen_linux.go code.

I do not know how micro was able to start up properly on old OpenBSD versions even though unsafe.Pointer was not converted to uintptr in arguments like written in the 4th pattern written in documentation, but the screen would be blank in newer versions.

There is a different module that had to be updated but micro seemed to start up properly when I tested using an OpenBSD 7.6 VM. I do not have a MacOS machine so I cannot test if there are no bugs on MacOS.

@niten94 niten94 closed this Dec 24, 2024
@niten94 niten94 deleted the syscall-ptrconv branch December 24, 2024 10:11
@niten94 niten94 restored the syscall-ptrconv branch December 24, 2024 10:15
@niten94 niten94 reopened this Dec 24, 2024
@niten94 niten94 changed the title Call ioctl using Syscall, convert pointers in call Use unix.Ioctl* on BSD systems, deduplicate tscreen_.*go Dec 24, 2024
@niten94 niten94 changed the title Use unix.Ioctl* on BSD systems, deduplicate tscreen_.*go Use unix.Ioctl* on BSD systems, deduplicate tscreen_*.go Dec 24, 2024
@JoeKar
Copy link

JoeKar commented Dec 31, 2024

Upstream SYS_IOCTL isn't used, but still syscall is invoked at some places.
The long term target is to keep track with the upstream version of tcell and only merge what is necessary for micro which isn't upstream so far.

Edit1:
Unfortunately I can't do any merges here, since it is a other private repository from @zyedidia.

As a short term plan it is maybe better to prepare https://github.com/micro-editor/tcell 1 with the same state as https://github.com/zyedidia/tcell has to have ready2go drop-in replacement for micro, with better/extended community access.

Edit2:
See zyedidia/micro#3593 resp. https://github.com/micro-editor/tcell/tree/legacy branch.

Footnotes

  1. https://github.com/micro-editor/tcell/tree/upstream-rebase already created on a much newer upstream version with some of micro's patches, but by far not all.

Replace Syscall with unix.Ioctl* in tscreen_{bsd,darwin}.go. Merge
tscreen_{bsd,solaris}.go with tscreen_linux.go and rename to
tscreen_unix.go to deduplicate code.
@niten94
Copy link
Author

niten94 commented Jan 1, 2025

Upstream SYS_IOCTL isn't used, but still syscall is invoked at some places.

SetNonblock seems to be the only function in syscall invoked upstream on Unix platforms, but libc functions seem to be used internally so upstream code may currently work in platforms where syscalls cannot be used. I may still create a pull request upstream to switch to golang.org/x/sys/unix.

The long term target is to keep track with the upstream version of tcell and only merge what is necessary for micro which isn't upstream so far.

I think it is good to avoid adding code different with upstream, but the code in tscreen_*.go is currently moved to tty_unix*.go and there will be more changes needed if added. This pull request is made to fix bugs only occuring in the fork without changing APIs and features, so I do not see problems with changing tscreen_*.go. However, I realized there is this old version of tscreen_unix.go similar with this pull request so I could adjust to it if it is better: https://github.com/gdamore/tcell/blob/e9095fe4f117459e3d961ea4eb3f55bf8f45e91c/tscreen_unix.go

Edit2: See zyedidia/micro#3593 resp. https://github.com/micro-editor/tcell/tree/legacy branch.

I have no experience with maintaining forks but I will leave a comment when there is anything I could find or help with deciding.

@JoeKar
Copy link

JoeKar commented Jan 1, 2025

I have no experience with maintaining forks but I will leave a comment when there is anything I could find or help with deciding.

We now have two forks12. For us it is easier to get the things into 2 and 2 used as dependency for micro itself, since we've no rights to merge in 1.
What I was trying to tell is, that this PR is maybe better to directly target the legacy of 2 instead of 1.

@dmaluka:
What do you think?

Footnotes

  1. https://github.com/zyedidia/tcell 2 3 4

  2. https://github.com/micro-editor/tcell, where the legacy branch contains more or less the same as 1 2 3 4

@niten94
Copy link
Author

niten94 commented Jan 3, 2025

Sorry, I was trying to say that I will leave a comment in zyedidia/micro#3593 about the questions brought up in the description. Rebasing with a new version may take a while and it seems like permission has been given to maintain forks and move them to the organization, so I do not see any problem maintaining the current version used there. I have not submitted the pull request to micro-editor/tcell yet, so I will create it there.

@niten94 niten94 closed this Jan 3, 2025
@niten94 niten94 deleted the syscall-ptrconv branch January 3, 2025 11:26
@niten94
Copy link
Author

niten94 commented Jan 7, 2025

The pull request was closed because I tried renaming the branch. I have not compared the file mentioned at this part in #29 (comment) a lot yet, but I have classes so I may not be able to open the pull request in micro-editor early:

However, I realized there is this old version of tscreen_unix.go similar with this pull request so I could adjust to it if it is better: https://github.com/gdamore/tcell/blob/e9095fe4f117459e3d961ea4eb3f55bf8f45e91c/tscreen_unix.go

@JoeKar
Copy link

JoeKar commented Jan 7, 2025

Summarizing it (as far as possible) inside a tscreen_unix.go doesn't seem to be the wrong approach, especially since it was also done upstream.
The only important thing is, that the change/PR must be created against the legacy-branch instead of the master-branch.

@niten94
Copy link
Author

niten94 commented Jan 18, 2025

There doesn't seem to be differences with the original code and functions in golang.org/x/term, so I have created a pull request using x/term like the old version of tscreen_unix.go I mentioned: micro-editor#2

There will be more changes in code structure if the commit is cherry picked so I replaced the calls instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants