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 x/term on Unix systems, deduplicate tscreen_*.go #2

Open
wants to merge 5 commits into
base: legacy
Choose a base branch
from

Conversation

niten94
Copy link

@niten94 niten94 commented Jan 18, 2025

Reasons why syscall.Syscall6(syscall.SYS_IOCTL, ...) has to be replaced are written below:

  • Syscall(...) and Go 1.23 or newer has to be used with SYS_IOCTL since syscall(2) is removed in OpenBSD 7.5
  • unsafe.Pointer has to be converted to uintptr in arguments like written in 4th pattern in documentation

There may be changes in any platform where using syscall.Syscall* will not work, so calls are replaced with functions in x/sys and x/term instead.

Input is drained as an attempt to remove code that is duplicated to address bug(s) on MacOS, but I wasn't able to understand how they exactly occur. There only seemed to be one occuring when I tried searching about the explanations in source code comments.

The addressed bug probably occurs when closing /dev/tty since polling done internally with os.File doesn't work on MacOS.12 I am also not sure if this workaround in upstream addresses the same bug or should be done in the fork. Code could be reduplidcated if not practical to come up with a better fix.

Micro started 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. The changes may not be tested and thought well enough.

Fixes zyedidia/micro#3557

Footnotes

  1. https://github.com/gokcehan/lf/issues/480#issuecomment-782896586

  2. https://nathancraddock.com/blog/macos-dev-tty-polling/

var tios uintptr
var ioc uintptr
t.tiosp = &termiosPrivate{}
var state *term.State

if t.in, e = poller.Open("/dev/tty", poller.O_RO); e != nil {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another file descriptor is opened upstream to fix the bug and there is no duplicate code just on MacOS, but I have not tried checking how much does the code structure need to be changed when using the same method yet.

@JoeKar
Copy link
Member

JoeKar commented Mar 22, 2025

Unfortunately I've no MacOS for tests too.

@niten94 niten94 force-pushed the legacy/syscall-ptrconv branch from e0961ee to e701458 Compare March 24, 2025 15:09
@niten94
Copy link
Author

niten94 commented Mar 24, 2025

I have split the changes into different commits since not all are similar.

Unfortunately I've no MacOS for tests too.

Would it be possible if Zachary could test if bugs don't occur when running shell commands in Micro with this pull request, and if they do on the commit before e701458 (1482264)?

I remember seeing old comments in issues where he seemed to have MacOS, but I'll re-add tscreen_darwin.go or use poller in tscreen_unix.go if no one could test or bugs are introduced.

niten94 added 2 commits March 26, 2025 21:35
Set VMIN and VTIME to read all input before closing tty, using
tcSetBufParams() added from upstream source code.
@niten94 niten94 force-pushed the legacy/syscall-ptrconv branch from e701458 to 60065f3 Compare March 26, 2025 13:38
@niten94
Copy link
Author

niten94 commented Mar 26, 2025

Sorry, I realized that there were files I did not add so I added them now.

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