.png)
4 months ago
10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
libxev needs to be replaced after the upstream merge
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also in general I think that instead of Linux/BSD we should just unify on "the GTK app", even though GTK can technically also be built for macOS and Windows. It's a bit of a mess, since FreeBSD behaves like Linux in userland and more like macOS in kernel land, so I really am not sure how best we should explain this.
build.zig Outdated Show resolved Hide resolved
| @@ -1823,7 +1823,7 @@ keybind: Keybinds = .{}, | |||
| /// Automatically hide the quick terminal when focus shifts to another window. | |||
| /// Set it to false for the quick terminal to remain open even when it loses focus. | |||
| /// | |||
| /// Defaults to true on macOS and on false on Linux. This is because global | |||
| /// Defaults to true on macOS and on false on Linux/BSD. This is because global | |||
| /// shortcuts on Linux require system configuration and are considerably less | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to edit this one, too. I wonder if we should just replace all instances of "Linux" to "the GTK app (runtime)"... pending discussion
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to replace Linux with Unix/Unix-like.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that macOS is also technically a Unix system (even more so than Linux), but on the user-facing level the BSDs are far more similar to Linux. It's a tricky nomenclature problem for sure
Comment on lines +2384 to +2410
| @"linux-cgroup": LinuxCgroup = if (builtin.os.tag == .linux) | ||
| .@"single-instance" | ||
| else | ||
| .never, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a specific switch for Linux even though the option already indicates that it's Linux-specific? Maybe we should rename the option then?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be just cgroups since one can enable it on BSD as well, so I just gated it for now. Config change such as this should probably be done in its own PR, but I can push it if it is okay.
I'm going to convert this PR to draft for now since some parts of it are still unfinished (i.e. localization support) and pending on the libxev PR. It's just not productive right now IMO to assess this PR as it currently stands. Once the libxev PR is merged and this PR has reached (near-)feature parity with Linux, then we can again mark it as ready for review.
@pluiedev fixed local issues, builds are passing now with i18n. Oh, theres error on the release that I need to look into.
Comment on lines 289 to 297
| run: | | ||
| # switch to pkg | ||
| wget https://ziglang.org/builds/zig-x86_64-freebsd-0.15.0-dev.777+6810ffa42.tar.xz | ||
| tar -xf zig-x86_64-freebsd-0.15.0-dev.777+6810ffa42.tar.xz | ||
| zig-x86_64-freebsd-0.15.0-dev.777+6810ffa42/zig build -Dapp-runtime=none test |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is it not possible to use 0.14.0 or 0.14.1? Seems like this could cause headaches later.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fresh zig is already in the latest branch so it will be in quarterly soon. This is just a temp workaround.
Also in general I think that instead of Linux/BSD we should just unify on "the GTK app"
Aside: the repo currently has os/macos and os/linux labels; would FreeBSD get its own label or should there be a label for “GTK app” instead?
@00-kat I'd say it should be both BSD and GTK (gui/gtk). There is probably no need to have individual labels for BSDs (at least not yet).
Translations are working as expected now.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good. Here is the TODO on my end:
- Review the xev changes and fix as necessary
- Migrate CI to Namespace and verify it works there
- Consider adding prefer to the non-dynamic xev to avoid the comptime checks here
Note: vmactions/freebsd-vm doesn't work on Namespace yet. I've opened a support ticket with them to get this looked at (cc @hugosantos). I didn't block libxev on it because its low traffic, but I want to wait for that to merge this because its so much more expensive to run Ghostty's CI.
Namespace fixed the issue, we can run the FreeBSD VM action on Namespace now, see libxev: mitchellh/libxev#168
Moving this to ready for review and will make some tweaks to get this merged. It may not be perfect yet but it at least gets Ghostty working to some extent (similar to Windows).
@charlesrocket We only test unit tests on FreeBSD right now in CI. It would be better to test both the GLFW and GTK builds as well. Can you work on a future PR to address that?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work @charlesrocket. Thank you very much! 😄
FreeBSD will remain a lower tier platform for now since none of the maintainers have access to actually test on it, but we at least have CI ensuring we don't break it and an active community member to help support it.
We can talk about more maintenance going forward in Discord. Thanks 😄
Hm, looks like the FreeBSD CI job failed... kind of a weird failure. I'll rerun it and see if that works.
@mitchellh definitely, finishing the freebsd port and will switch to GLFW build error.
@charlesrocket The firecracker VM approach didn't work for probably unrelated CI reasons. I'm sorry I haven't had time to look into this yet. I'd be fine merging this WITHOUT CI for now but we have to both be okay with the reality that FreeBSD support will probably bitrot until we have CI support because we don't have anyone staying on top of it.
If you're okay with that in the interim we can remove the freebsd job and try a subsequent PR to add it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have my reservations about some of these changes (a lot of them look a bit too tacked-on with the builtin.target.os.tag check), but I don't feel like blocking this further.
In the future I think there needs to be a cleanup PR to unify the terminology a little bit: we should use "GTK app" instead of "Linux" in a lot of cases within the codebase, unless something is truly specific to Linux (nothing immediately comes to mind yet). This IMO is the least intrusive option since although the GTK app can technically be built for macOS, we never distribute it in that form and we generally assume it to be run in a FreeDesktop-based environment (Linux, FreeBSD, etc.). We also should probably update the website ahead of Ghostty 1.2 to reflect this nomenclature change.
| @@ -21,14 +22,22 @@ pub fn init(b: *std.Build, cfg: *const Config) !GhosttyI18n { | |||
| defer steps.deinit(); | |||
|
|
|||
| inline for (internal_os.i18n.locales) |locale| { | |||
| // There is no encoding suffix in the LC_MESSAGES path on FreeBSD, | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but I'm curious how programs like vim gets away with it on FBSD. Are there truly no programs out there that contain the encoding suffix?
| @@ -1823,7 +1823,7 @@ keybind: Keybinds = .{}, | |||
| /// Automatically hide the quick terminal when focus shifts to another window. | |||
| /// Set it to false for the quick terminal to remain open even when it loses focus. | |||
| /// | |||
| /// Defaults to true on macOS and on false on Linux. This is because global | |||
| /// Defaults to true on macOS and on false on Linux/BSD. This is because global | |||
| /// shortcuts on Linux require system configuration and are considerably less | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: go through all the configs mentioning "Linux" and think about whether it applies to BSDs as well
| @@ -2812,7 +2815,7 @@ pub fn loadCliArgs(self: *Config, alloc_gpa: Allocator) !void { | |||
| // styling, etc. based on the command. | |||
| // | |||
| // See: https://github.com/Vladimir-csp/xdg-terminal-exec | |||
| if (comptime builtin.os.tag == .linux) { | |||
| if ((comptime builtin.os.tag == .linux) or (comptime builtin.os.tag == .freebsd)) { | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add a helper function inside std/os that determines whether the current system is based on FreeDesktop (XDG) standards, since I can totally see us needing to expand this check a lot later on.
i mean if the want for more portability beyond freebsd is interesting i have quiet a few friends who use openbsd which i could ask to help with knowledge about the inner workings of it
I still have my reservations about some of these changes (a lot of them look a bit too tacked-on with the builtin.target.os.tag check), but I don't feel like blocking this further.
I don't think there was much. There was zero cases added to the GTK app (just an xev check unrelated to BSD), and almost the entirety of OS tag checks are in the build system and src/os package, both of which are inherently extremely OS/target dependent. I think it's pretty clean, considering.
In the future I think there needs to be a cleanup PR to unify the terminology a little bit: we should use "GTK app" instead of "Linux" in a lot of cases within the codebase, unless something is truly specific to Linux (nothing immediately comes to mind yet).
This is probably right. I've avoided "GTK" in case we ever actually dropped GTK as the default for Linux (and now BSD). But that's probably a stretch. Using the apprt as the description is probably more appropriate. I think macOS is in line with that because while macOS uses the libghostty apprt technically, its really a dedicated macOS app there consuming that.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, given the caveats noted earlier, which I'll repeat here.
We never got CI working. As such, we've all agreed that the FreeBSD support may bitrot. We don't have any active maintainers running FreeBSD (or any BSD for that matter). We can't guarantee that PRs the community or ourselves make will continue to work on FreeBSD. As such, we should strive to get FreeBSD in CI.
Until then, I don't want to block improvements on our portability, so I'm willing to approve and merge with that disclaimer out of the way.
And I want to thank @charlesrocket again for the excellent effort and work to get us here!
