Skip to content

termios: Support FreeBSD #2544

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

Merged
merged 2 commits into from
Nov 4, 2022
Merged

Conversation

patmaddox
Copy link
Contributor

@patmaddox patmaddox commented Oct 31, 2022

This sets some variables necessary to support Tailscale's built-in sshd server on FreeBSD (tailscale/tailscale#6155).

I could use some help finishing this off. Current issues that I think are worth addressing:

  • pkg/termios/termios_freebsd.go is a cp of pkg/termios/termios_darwin.go, with the addition of the go:build freebsd / +build freebsd lines.
    • Addressed by renaming to termios_bsd.go and conditionally building.
  • pkg/termios/var_freebsd.go has a lot of duplication with pkg/termios/var_unix.go as well.
    • Addressed by moving some syscalls out of var_unix.go and into var_linux.go and var_darwin.go.

From what I can tell, CI doesn't currently verify FreeBSD.

#2421 says:

Remove cirrus ci: It was only used for verifying freebsd buildability. We are going to use github actions for that instead (#2419)

GitHub actions doesn't officially support FreeBSD as far as I know. The method I'm aware of requires you to run a FreeBSD VM inside of a MacOS instance. That's pretty janky.

Cirrus CI runs directly on FreeBSD machines, so it should probably be re-instated for FreeBSD verification.

I am willing to assist (re-)configuring FreeBSD CI on Cirrus.

@patmaddox patmaddox force-pushed the freebsd-tty-vars branch 3 times, most recently from 8381d55 to da6a6f6 Compare November 1, 2022 02:13
@codecov
Copy link

codecov bot commented Nov 1, 2022

Codecov Report

Base: 73.66% // Head: 73.66% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (b057145) compared to base (7e76d6f).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head b057145 differs from pull request most recent head 027ebb5. Consider uploading reports for the commit 027ebb5 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2544   +/-   ##
=======================================
  Coverage   73.66%   73.66%           
=======================================
  Files         405      405           
  Lines       41231    41234    +3     
=======================================
+ Hits        30374    30377    +3     
  Misses      10857    10857           
Impacted Files Coverage Δ
pkg/termios/sgtty_unix.go 0.00% <ø> (ø)
pkg/termios/var_linux.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@patmaddox patmaddox changed the title Set TTY vars for FreeBSD termios: Support FreeBSD Nov 1, 2022
Copy link
Member

@rminnich rminnich left a comment

Choose a reason for hiding this comment

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

This sure looks good to me. If you can make cirrus CI
work it would be nice to have freebsd CI back. Help wanted :-)

@rminnich rminnich added the Awaiting author Waiting for new changes or feedback for author. label Nov 3, 2022
Copy link
Member

@rminnich rminnich left a comment

Choose a reason for hiding this comment

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

from what I've seen, ttyio is always going to be a maze, I think this is about as good a job as can be done. Thanks again.

rminnich
rminnich previously approved these changes Nov 3, 2022
Signed-off-by: Pat Maddox <pat@ratiopbc.com>
@patmaddox
Copy link
Contributor Author

Removed unnecessary build line from var_freebsd.go, and rebased off of main.

@patmaddox
Copy link
Contributor Author

patmaddox commented Nov 3, 2022

This sure looks good to me. If you can make cirrus CI work it would be nice to have freebsd CI back. Help wanted :-)

Cool! Happy to do it. Would you like to see it as part of this PR, or a separate one?

Edit: Based on the number of failures, it should probably be a separate PR. I suspect there will be a lot of stub implementations on FreeBSD that serve as a TODO list for full support.

@patmaddox patmaddox requested a review from rminnich November 3, 2022 18:59
@patmaddox patmaddox added Awaiting reviewer Waiting for a reviewer. and removed Awaiting author Waiting for new changes or feedback for author. labels Nov 3, 2022
@rminnich rminnich added automerge Applying this label auto-merges the PR when ready Awaiting author Waiting for new changes or feedback for author. and removed Awaiting reviewer Waiting for a reviewer. labels Nov 4, 2022
@rminnich
Copy link
Member

rminnich commented Nov 4, 2022

yeah, let's do a separate PR, and thanks in advance for the issues we are certain to see :-)

I will help whereever I can.

@probot-auto-merge probot-auto-merge bot merged commit 18fd0ce into u-root:main Nov 4, 2022
@patmaddox patmaddox deleted the freebsd-tty-vars branch November 4, 2022 06:37
ghuntley pushed a commit to coder/coder that referenced this pull request Dec 7, 2022
`./scripts/build_go.sh --os freebsd` fails to build with the following
(unique) errors:

  termios/sgtty_unix.go:13:20: undefined: unix.TCGETS
  termios/termios.go:33:10: undefined: TTYIO
  termios/termios.go:39:9: undefined: MakeRaw
  termios/var_unix.go:42:38: undefined: syscall.IUTF8
  termios/var_unix.go:50:37: undefined: syscall.OFILL
  termios/var_unix.go:51:37: undefined: syscall.OFDEL

This was resolved upstream in u-root/u-root/pull/2544 [1]
via tailscale/tailscale/pull/6155 [2] in u-root/u-root/commit/18fd0ce36 [3]

Update termios (u-root) dependency to a commit just past that
change.

[1] u-root/u-root#2544
[2] tailscale/tailscale#6155
[3] u-root/u-root@18fd0ce36

Fixes: #4516
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Applying this label auto-merges the PR when ready Awaiting author Waiting for new changes or feedback for author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants