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.

Sign up for GitHub

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

ghostty: add nixos tests, add build options, fix x11 backend #368726

Merged
merged 7 commits into from
Dec 29, 2024
Merged

ghostty: add nixos tests, add build options, fix x11 backend #368726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cf22cac
ghostty: add nixos test
getchoo Dec 28, 2024
cddaf44
ghostty: add nixosTests.allTerminfo to passthru.tests
getchoo Dec 28, 2024
aeae376
ghostty: factor out dependencies
getchoo Dec 28, 2024
7f6a54b
ghostty: add optimizationLevel option
getchoo Dec 28, 2024
720556a
ghostty: cleanup outputs
getchoo Dec 28, 2024
b36e029
ghostty: fix x11 backend
getchoo Dec 28, 2024
f4d4b22
ghostty: add darwin to meta.platforms
getchoo Dec 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions nixos/tests/terminal-emulators.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ let

germinal.pkg = p: p.germinal;

ghostty.pkg = p: p.ghostty;

gnome-terminal.pkg = p: p.gnome-terminal;

guake.pkg = p: p.guake;
Expand Down
190 changes: 122 additions & 68 deletions pkgs/by-name/gh/ghostty/package.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,153 +1,207 @@
{
lib,
stdenv,
bzip2,
callPackage,
expat,
fetchFromGitHub,
fontconfig,
freetype,
glib,
glslang,
harfbuzz,
lib,
libadwaita,
libGL,
libpng,
libX11,
libXcursor,
libXi,
libXrandr,
libadwaita,
ncurses,
nixosTests,
oniguruma,
pandoc,
pkg-config,
removeReferencesTo,
stdenv,
versionCheckHook,
wrapGAppsHook4,
zig_0_13,
zlib,
# Usually you would override `zig.hook` with this, but we do that internally
# since upstream recommends a non-default level
# https://github.com/ghostty-org/ghostty/blob/4b4d4062dfed7b37424c7210d1230242c709e990/PACKAGING.md#build-options
optimizeLevel ? "ReleaseFast",
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this? It doesn't sound like any user could possibly want to use anything but ReleaseFast.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

# https://github.com/ghostty-org/ghostty/blob/4b4d4062dfed7b37424c7210d1230242c709e990/build.zig#L106
withAdwaita ? true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}:

let
# Ghostty needs to be built with --release=fast, --release=debug and
# --release=safe enable too many runtime safety checks.
zig_hook = zig_0_13.hook.overrideAttrs {
zig_default_flags = "-Dcpu=baseline -Doptimize=ReleaseFast --color off";
zig_default_flags = "-Dcpu=baseline -Doptimize=${optimizeLevel} --color off";
};

# https://github.com/ghostty-org/ghostty/blob/4b4d4062dfed7b37424c7210d1230242c709e990/src/apprt.zig#L72-L76
appRuntime = if stdenv.hostPlatform.isLinux then "gtk" else "none";
# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

If the goal is only platform support, you should simply make the deps optional depending on platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

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


stdenv.mkDerivation (finalAttrs: {
pname = "ghostty";
version = "1.0.0";

outputs = [
"out"
"man"
"shell_integration"
"terminfo"
"vim"
];

src = fetchFromGitHub {
owner = "ghostty-org";
repo = "ghostty";
tag = "v${finalAttrs.version}";
hash = "sha256-AHI1Z4mfgXkNwQA8xYq4tS0/BARbHL7gQUT41vCxQTM=";
};

strictDeps = true;

nativeBuildInputs = [
glib # Required for `glib-compile-schemas`
ncurses
pandoc
pkg-config
removeReferencesTo
wrapGAppsHook4
zig_hook
];

buildInputs = [
bzip2
expat
fontconfig
freetype
glslang
harfbuzz
libadwaita
libGL
libpng
libX11
libXcursor
libXi
libXrandr
oniguruma
zlib
];

dontConfigure = true;
# doCheck is set to false because unit tests currently fail inside the Nix sandbox.
doCheck = false;
doInstallCheck = true;
# 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");'
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity: any idea why this zig lookup doesn't succeed without forcing the full path?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ;)

'';

getchoo marked this conversation as resolved.
Show resolved Hide resolved
deps = callPackage ./deps.nix {
name = "${finalAttrs.pname}-cache-${finalAttrs.version}";
};

strictDeps = true;

nativeBuildInputs =
[
ncurses
pandoc
pkg-config
removeReferencesTo
zig_hook
]
++ lib.optionals (appRuntime == "gtk") [
glib # Required for `glib-compile-schemas`
wrapGAppsHook4
];

buildInputs =
[
glslang
oniguruma
]
++ lib.optional (appRuntime == "gtk" && withAdwaita) libadwaita
++ lib.optional (appRuntime == "gtk") libX11
++ lib.optional (renderer == "opengl") libGL
++ lib.optionals (fontBackend == "fontconfig_freetype") [
bzip2
fontconfig
freetype
harfbuzz
];

zigBuildFlags =
[
"--system"
"${finalAttrs.deps}"
"-Dversion-string=${finalAttrs.version}"

"-Dapp-runtime=${appRuntime}"
"-Dfont-backend=${fontBackend}"
"-Dgtk-adwaita=${lib.boolToString withAdwaita}"
"-Drenderer=${renderer}"
]
++ lib.mapAttrsToList (name: package: "-fsys=${name} --search-prefix ${lib.getLib package}") {
inherit glslang;
};

zigCheckFlags = finalAttrs.zigBuildFlags;

outputs = [
"out"
"terminfo"
"shell_integration"
"vim"
getchoo marked this conversation as resolved.
Show resolved Hide resolved
];
# Unit tests currently fail inside the sandbox
doCheck = false;

postInstall = ''
mkdir -p "$terminfo/share"
mv "$out/share/terminfo" "$terminfo/share/terminfo"
ln -sf "$terminfo/share/terminfo" "$out/share/terminfo"
/**
Ghostty really likes all of it's resources to be in the same directory, so link them back after we split them

mkdir -p "$shell_integration"
mv "$out/share/ghostty/shell-integration" "$shell_integration/shell-integration"
ln -sf "$shell_integration/shell-integration" "$out/share/ghostty/shell-integration"
- https://github.com/ghostty-org/ghostty/blob/4b4d4062dfed7b37424c7210d1230242c709e990/src/os/resourcesdir.zig#L11-L52
- https://github.com/ghostty-org/ghostty/blob/4b4d4062dfed7b37424c7210d1230242c709e990/src/termio/Exec.zig#L745-L750
- https://github.com/ghostty-org/ghostty/blob/4b4d4062dfed7b37424c7210d1230242c709e990/src/termio/Exec.zig#L818-L834

terminfo and shell integration should also be installable on remote machines

```nix
{ pkgs, ... }: {
environment.systemPackages = [ pkgs.ghostty.terminfo ];

programs.bash = {
interactiveShellInit = ''
if [[ "$TERM" == "xterm-ghostty" ]]; then
builtin source ${pkgs.ghostty.shell_integration}/bash/ghostty.bash
fi
'';
};
}
```
*/
postFixup = ''
ln -s $man/share/man $out/share/man

moveToOutput share/terminfo $terminfo
getchoo marked this conversation as resolved.
Show resolved Hide resolved
ln -s $terminfo/share/terminfo $out/share/terminfo

mv $out/share/ghostty/shell-integration $shell_integration
ln -s $shell_integration $out/share/ghostty/shell-integration

mv $out/share/vim/vimfiles $vim
rmdir $out/share/vim
ln -s $vim $out/share/vim-plugins
Copy link
Contributor

Choose a reason for hiding this comment

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

$vim needs to be added to $out/nix-support/propagated-user-env-packages as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It needs to be added to $out/nix-support/propagated-user-env-packages so that it is installed when Ghostty is installed.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just vim that needs the extra config and adding that config 'installs' the output. Neovim appears to just work.


mv "$out/share/vim/vimfiles" "$vim"
ln -sf "$vim" "$out/share/vim/vimfiles"
'';

preFixup = ''
remove-references-to -t ${finalAttrs.deps} $out/bin/ghostty
'';

NIX_LDFLAGS = [ "-lX11" ];

nativeInstallCheckInputs = [
versionCheckHook
];

doInstallCheck = true;

versionCheckProgramArg = [ "--version" ];

passthru = {
tests = lib.optionalAttrs stdenv.hostPlatform.isLinux {
getchoo marked this conversation as resolved.
Show resolved Hide resolved
inherit (nixosTests) allTerminfo;
nixos = nixosTests.terminal-emulators.ghostty;
};
};

meta = {
homepage = "https://ghostty.org/";
description = "Fast, native, feature-rich terminal emulator pushing modern features";
longDescription = ''
Ghostty is a terminal emulator that differentiates itself by being
fast, feature-rich, and native. While there are many excellent terminal
emulators available, they all force you to choose between speed,
features, or native UIs. Ghostty provides all three.
'';
homepage = "https://ghostty.org/";
downloadPage = "https://ghostty.org/download";

license = lib.licenses.mit;
platforms = lib.platforms.linux;
mainProgram = "ghostty";
outputsToInstall = finalAttrs.outputs;
maintainers = with lib.maintainers; [
jcollie
pluiedev
getchoo
];
outputsToInstall = [
"out"
"man"
"shell_integration"
"terminfo"
];
platforms = lib.platforms.linux ++ lib.platforms.darwin;
# Issues finding the SDK in the sandbox
broken = stdenv.hostPlatform.isDarwin;
};
})
Loading