[pkg/term] refactor and switch syscall to x/sys#33180
Conversation
|
@tophj-ibm There seem to be two issues with the code: |
tophj-ibm
force-pushed
the
switch-pkg-term-syscalls-to-x/sys
branch
from
b05148b to
60741c7
Compare
|
@unclejack thanks, fixed. 😓 (I swear I tested this). Windows failure is real. |
pkg/term/term_windows.go
Outdated
There was a problem hiding this comment.
windows failure is because this and the import above it both resolve to package windows.
@jhowardmsft, because I want to make the change from syscall to -> golang.org/x/sys/(windows|unix etc) across the entire project, you have any objections to me changing the /pkg/term/windows package to something else (package windowsconsole or something)?
AkihiroSuda
added
the
status/failing-ci
tophj-ibm
force-pushed
the
switch-pkg-term-syscalls-to-x/sys
branch
from
60741c7 to
a3ebc52
Compare
tophj-ibm
force-pushed
the
switch-pkg-term-syscalls-to-x/sys
branch
from
a3ebc52 to
7241291
Compare
dnephin
removed
the
status/failing-ci
|
@unclejack PTAL ❤️ |
|
I was hoping that with this change we could get rid of all the cgo in pkg/term. |
Switches calls to syscall to x/sys, which is more up to date. This is fixes a number of possible bugs on other architectures where ioctl tcget and tcset aren't implemented correctly. There are a few remaining syscall references, because x/sys doesn't have an Errno implementation yet. Also removes a ppc64le and cgo build tag that fixes building on ppc64le without cgo Signed-off-by: Christopher Jones <[email protected]>
tophj-ibm
force-pushed
the
switch-pkg-term-syscalls-to-x/sys
branch
from
7241291 to
f30b072
Compare
| @@ -1,21 +1,21 @@ | |||
| // +build !windows | |||
| // +build !linux !cgo | |||
| // +build !linux !ppc64le | |||
There was a problem hiding this comment.
@tiborvass @clnperez I think 🤞 these are right but would appreciate the double check. We still need this file because tc_solaris defines tcget/sets, but it should include the linux, cgo case.
There was a problem hiding this comment.
I think your "because" is throwing me. I think we still need this to catch "other" (FreeBSD & OpenBSD?).
There was a problem hiding this comment.
Or is this also defining tcgets/tcsets for linux as well now? I think it is -- so maybe we should rename the file to tc.go ? headache
There was a problem hiding this comment.
1.) This should still catch those, because they are !(windows) and (!solaris or !cgo).
2.) Sort of, it's defining a func named tcget / tcsets as the unix syscall tcget/tcset, where before it was defining them as the C ones. I think it should stay as other because it's building either the solaris file or this (other) one. If it was named tc.go and tc_solaris.go I don't think that would be as obvious, and the build constraints in these files are already confusing enough.
Signed-off-by: Tibor Vass <[email protected]>
tophj-ibm
changed the title
Signed-off-by: Christopher Jones <[email protected]>
|
So this PR turned into more of a refactoring of /pkg/term, so here are the total changes at this point: 1.) switch all syscall references (except for Errno because it hasn't been brought over yet) to golang.org/x/sys/(unix|windows) 2.) refactor all termios_(freebsd|darwin|openbsd) to termios_bsd (thanks @tiborvass!) 3.) remove tc_linux_cgo because it was only used as a workaround for a bug fixed by step 1.) 4.) rename tc_other.go to tc.go and change buildtags so it builds on linux/ppc64le and linux/cgo (although it doesn't use cgo) 5.) rename term_(solaris|unix) to winsize_(solaris|unix) because they were confusing with term_windows.go and term.go which are completely different files. 6.) add a cgo buildtag to now named winsize_solaris_cgo because it should have had one in the first place. 7.) rename the term/windows package to windowsconsole because it conflicts with /x/sys/windows It should be ready for review again, |
dnephin
left a comment
dnephin
left a comment
There was a problem hiding this comment.
LGTM
Tested with docker/cli and it fixes ppc64le
|
ping @jhowardmsft @tiborvass PTAL |
|
LGTM |
9 participants
Part of a bigger plan to switch all syscall references to x/sys, but I thought
I would start small.
Switches calls to syscall to x/sys, which is more up to date.
This is fixes a number of possible bugs on other architectures
where ioctl tcget and tcset aren't implemented correctly.
There are a few remaining syscall references, because x/sys doesn't
have an Errno, Signal, or SysProcAttr implementation yet.
Also removes a ppc64le and cgo build tag that fixes building on
ppc64le without cgo which was a problem because we weren't using
/x/sys
Signed-off-by: Christopher Jones [email protected]