ghostty: add nixos tests, add build options, fix x11 backend #368726
Conversation
|
@ofborg build ghostty ghostty.passthru.tests |
bcc5ad2 to
1b713d3
Compare
|
👋 for ghostty package, I'm getting a build error both on master and this PR, by any chance @getchoo have you run into it while making changes here? If not - that's alright, I'll probably open a seperate issue about it. I already raised this issue on Discord too but no success. In theory, it should be working without any problem. build log (x86_64-linux) |
Can't say I have :( An error about libappstream is actually very confusing as I'm not sure what could be using it. As this is already happening on master though, I would encourage a separate issue to investigate further |
| # https://github.com/ghostty-org/ghostty/blob/4b4d4062dfed7b37424c7210d1230242c709e990/src/font/main.zig#L94 | ||
| fontBackend ? if stdenv.hostPlatform.isDarwin then "coretext" else "fontconfig_freetype", | ||
| # https://github.com/ghostty-org/ghostty/blob/4b4d4062dfed7b37424c7210d1230242c709e990/PACKAGING.md#build-options | ||
| optimizeLevel ? "ReleaseFast", |
There was a problem hiding this comment.
Do we really need this? It doesn't sound like any user could possibly want to use anything but ReleaseFast.
There was a problem hiding this comment.
It was asked for previously. I think it would also make sense to expose this directly, as the way we override zig.hook makes it difficult to change this flag directly
I've also found it useful in iterating on this expression at least :p
1b713d3 to
d0fd31a
Compare
This is meant to make cross platform support a bit easier. The options are kept private as they aren't meant to be touched by end users
d0fd31a to
a1f5e53
Compare
| ln -sf "$vim" "$out/share/vim/vimfiles" | ||
| mv $out/share/vim/vimfiles $vim | ||
| rmdir $out/share/vim | ||
| ln -s $vim $out/share/vim-plugins |
There was a problem hiding this comment.
$vim needs to be added to $out/nix-support/propagated-user-env-packages as well.
There was a problem hiding this comment.
Why? programs.vim in NixOS already add share/vim-plugins to environment.pathsToLink and using the vim output with home-manager's programs.vim.plugins will work as expected. Installing the plugin to the top-level will do nothing AFAIK
There was a problem hiding this comment.
It needs to be added to $out/nix-support/propagated-user-env-packages so that it is installed when Ghostty is installed.
There was a problem hiding this comment.
It's already installed at $out/share/vim-plugins whenever the ghostty binary is. There's no point installing the plugin itself to the a profile; that only belongs in plugin wrappers, etc.
There was a problem hiding this comment.
It's just vim that needs the extra config and adding that config 'installs' the output. Neovim appears to just work.
| # https://github.com/ghostty-org/ghostty/blob/4b4d4062dfed7b37424c7210d1230242c709e990/src/font/main.zig#L94 | ||
| fontBackend = if stdenv.hostPlatform.isDarwin then "coretext" else "fontconfig_freetype"; | ||
| # https://github.com/ghostty-org/ghostty/blob/4b4d4062dfed7b37424c7210d1230242c709e990/src/renderer.zig#L51-L52 | ||
| renderer = if stdenv.hostPlatform.isDarwin then "metal" else "opengl"; | ||
| in |
There was a problem hiding this comment.
I don't think that letting the user modify these options really adds anything but more work for us (or the Ghostty developers) when people inevitably choose incompatible or non-functional combinations and then report bugs. Ghostty will automatically choose the right runtimes, backends, and renderers depending on the platform. These should be removed.
There was a problem hiding this comment.
I don't think that letting the user modify these options really adds anything but more work for us (or the Ghostty developers) when people inevitably choose incompatible or non-functional combinations and then report bugs.
While I very much disagree with this (people can already set these flags with zigBuildFlags or when compiling manually and do the same thing...plus I don't think making it easier to play around with these options will actually influence people to go enable them and then complain here or upstream when they're clearly not supported defaults) these options aren't able to be modified by the user already. Previously they were, but after conversation with @pluiedev they're now in this let ...; in block
The main goal here is to avoid dumping all of our dependencies into broad isDarwin and isLinux checks, especially when in the future more backends/platforms may be supported, which features/dependencies overlapping between
There was a problem hiding this comment.
If the goal is only platform support, you should simply make the deps optional depending on platform.
There was a problem hiding this comment.
As I said
The main goal here is to avoid dumping all of our dependencies into broad isDarwin and isLinux checks
I believe this makes future platform support much easier, as chances are BSD support for example would probably use similar, but not the exact same features and dependencies; separating these features into their own distinct booleans makes mixing and matching these relatively simple and a lot more comprehensible. This is also modeled almost exactly after how the build scripts determine whether or not the dependencies are needed, which I think is a good thing to follow
| versionCheckHook, | ||
| wrapGAppsHook4, | ||
| zig_0_13, | ||
| zlib, | ||
| # https://github.com/ghostty-org/ghostty/blob/4b4d4062dfed7b37424c7210d1230242c709e990/build.zig#L106 | ||
| withAdwaita ? true, |
There was a problem hiding this comment.
I also don't think that there's any value in letting the user build the package without Adwaita. It can be disabled at runtime if the user really needs to.
There was a problem hiding this comment.
It should also be noted that there are a number of developers that work on the GTK frontend that want to eliminate the possibility of building without Adwaita. There are many features that don't work at all without Adwaita (tab overviews is the big one) and other features have to be implemented twice, once for plain GTK, and once for GTK+Adwaita.
There was a problem hiding this comment.
This can be useful to do if you want to minimise your dependency tree. Though you'd be hard-pressed to get rid of Adwaita on any desktop system.
Still, use-cases where one doesn't need any fancy UI elements at all exist. The first thing I did when trying out ghostty was to disable the bar.
There was a problem hiding this comment.
I also don't think that there's any value in letting the user build the package without Adwaita.
I think upstream may disagree with you there, at least for now. Why else would this be an option?
It should also be noted that there are a number of developers that work on the GTK frontend that want to eliminate the possibility of building without Adwaita.
If they do it'll be easy to remove. I don't see much reason in hiding this option in the meantime
a1f5e53 to
2e5fdf1
Compare
Forcing linkage isn't enough for Zig's `dlopen()` call. Let's just point it towards the exact path instead
f6c194a to
f4d4b22
Compare
|
Hoping to get this merged soon so the x11 issues doesn't hit nixos-unstable 🤞 |
|
Self merging to get this fix in ASAP. Feel free to make followups for other changes |
| # Avoid using runtime hacks to help find X11 | ||
| postPatch = lib.optionalString (appRuntime == "gtk") '' | ||
| substituteInPlace src/apprt/gtk/x11.zig \ | ||
| --replace-warn 'std.DynLib.open("libX11.so");' 'std.DynLib.open("${lib.getLib libX11}/lib/libX11.so");' |
There was a problem hiding this comment.
Out of curiosity: any idea why this zig lookup doesn't succeed without forcing the full path?
There was a problem hiding this comment.
Not really sure tbh. It could be the dlopen implementation internally; usually forcing linkage at runtime via -l would be enough. I also tried using -rpath before this, but no dice
Won't be a problem once ghostty-org/ghostty#3857 gets into a stable tag though ;)
…68726) * ghostty: add nixos test * ghostty: add nixosTests.allTerminfo to passthru.tests * ghostty: factor out dependencies This is meant to make cross platform support a bit easier. The options are kept private as they aren't meant to be touched by end users * ghostty: add optimizationLevel option * ghostty: cleanup outputs * ghostty: fix x11 backend Forcing linkage isn't enough for Zig's `dlopen()` call. Let's just point it towards the exact path instead * ghostty: add darwin to meta.platforms (cherry picked from commit 716c941)
…68726) * ghostty: add nixos test * ghostty: add nixosTests.allTerminfo to passthru.tests * ghostty: factor out dependencies This is meant to make cross platform support a bit easier. The options are kept private as they aren't meant to be touched by end users * ghostty: add optimizationLevel option * ghostty: cleanup outputs * ghostty: fix x11 backend Forcing linkage isn't enough for Zig's `dlopen()` call. Let's just point it towards the exact path instead * ghostty: add darwin to meta.platforms
Follow up on #368404
Big changes here include:
Exposing all major build configuration optionsSeems this isn't something wantedThings done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.