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.

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: init at 1.0.0 #368404

Merged
merged 2 commits into from
Dec 28, 2024
Merged

ghostty: init at 1.0.0 #368404

merged 2 commits into from
Dec 28, 2024

Conversation

jcollie
Copy link
Contributor

@jcollie jcollie commented Dec 26, 2024

Adding Ghostty:

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.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Dec 26, 2024
pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
@pluiedev
Copy link
Contributor

pluiedev commented Dec 26, 2024

Also needs to be reformatted w/ nixfmt-rfc-style (the nixpkgs-vet CI failure is a zig2nix bug that we can't really control)

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 26, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Dec 26, 2024
downloadPage = "https://ghostty.org/download";

license = lib.licenses.mit;
platforms = lib.platforms.linux;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
platforms = lib.platforms.linux;
platforms = lib.platforms.linux ++ lib.platforms.darwin;

If it doesn’t build on macOS, it would be better to set broken

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 less that it doesn't build on Darwin and more it can't build on Darwin since xcodebuild has to be invoked outside of the Nix build environment. I think we should just repackage the .dmg for now until we somehow figure out a way to compile it with Nix

Copy link
Member

Choose a reason for hiding this comment

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

I’m curious why that’s the case

And whether that’s been tested since the darwin SDK refactor

Copy link
Contributor

Choose a reason for hiding this comment

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

Ultimately neither I nor @jcollie have Darwin setups to test with, so either someone comes forward with a better idea, or we have to stick with a repackaging for now

Copy link
Contributor 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 anyone has tried since the darwin refactor. I won't be doing that work because I don't have a macOS machine to test on.

Copy link
Contributor

@Eveeifyeve Eveeifyeve Dec 27, 2024

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 anyone has tried since the darwin refactor. I won't be doing that work because I don't have a macOS machine to test on.

I could test, I willing be happy to help to support darwin. I have a m2 macbook which means I can cross compile.

Copy link
Contributor

@anund anund Dec 27, 2024

Choose a reason for hiding this comment

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

I believe discussions on this topic are happening on matrix https://matrix.to/#/#macos:nixos.org (though more focused on getting the build tooling chain working for source builds). I'm not sure where work to package the DMG is happening/being discussed.

Copy link
Member

Choose a reason for hiding this comment

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

Long story short is that building the Mac app from source appears to be blocked on getting our Swift situation into a better place (if anyone has ideas on how to do a source build with our current Swift setup, please chime in in that matrix room).

I haven't seen anything about packaging the DMG.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bit of a drive by comment but: for my setup and https://github.com/kovidgoyal/kitty I eventually gave up and used the dmg directly; some features (like OS notifications) require signed executables

https://github.com/nmattia/homies/blob/1c27051828b7c10ba2cda8cc790cc50bd796b423/kitty/default.nix#L11

Copy link
Member

Choose a reason for hiding this comment

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

Please keep the macOS discussion for follow-up PRs or issues. @jcollie has already mentioned that they won't be the one doing any sort of macOS support.

Please add the platforms though and set badPlatforms or broken.

pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
@jcollie jcollie force-pushed the ghostty-init-1.0.0 branch 3 times, most recently from 9da662e to ee671bc Compare December 26, 2024 21:59
Copy link
Contributor

@pluiedev pluiedev left a comment

Choose a reason for hiding this comment

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

Just needs mister @ofborg to be happy

@lilyball
Copy link
Member

Any chance we can also get Ghostty's terminfo file added to the NixOS terminfo database, or does that belong in a separate PR?

@jcollie
Copy link
Contributor Author

jcollie commented Dec 26, 2024

Any chance we can also get Ghostty's terminfo file added to the NixOS terminfo database, or does that belong in a separate PR?

That should be handled already, there's a terminfo output in the package and environment.enableAllTerminfo should install it.

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 368404


x86_64-linux

⏩ 2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
✅ 4 packages built:
  • ghostty
  • ghostty.shell_integration
  • ghostty.terminfo
  • ghostty.vim

aarch64-linux

⏩ 2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
✅ 4 packages built:
  • ghostty
  • ghostty.shell_integration
  • ghostty.terminfo
  • ghostty.vim

Copy link
Member

@pbsds pbsds left a comment

Choose a reason for hiding this comment

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

Could you add this to nixos/tests/terminal-emulators.nix and add the resulting test to passthru.tests?

@surfaceflinger
Copy link
Member

Would it be possible to include a patch so that ghostty runs without io_uring (which is disabled on linux-hardened)?

sed -i 's/^const xev = @import("xev");$/const xev = @import("xev").Epoll;/' **/*.zig

https://discord.com/channels/1005603569187160125/1312996817264181350 (sorry)
mitchellh/libxev#131

No idea if this causes any visible performance issues

@philiptaron
Copy link
Contributor

Would it be possible to include a patch so that ghostty runs without io_uring (which is disabled on linux-hardened)?

That's the sort of thing that you can patch with overrideAttrs in your own Nix config. Let's follow upstream's lead here.

@fpletz
Copy link
Member

fpletz commented Dec 27, 2024

The runtime closure includes all the zig sources:

$ nix path-info --recursive ./result
[...]
/nix/store/8pgw0b3ymk014c1v7l5pc9ymj4dfd6nx-freetype
/nix/store/9bgzradqy1346lcj56d32mfcdjyinjk6-oniguruma
/nix/store/az6rd4gxwskz4y6l94cm0axgn5bks3wa-glfw
/nix/store/b2wn0akxsjr3g5gx3yzahmgj588q44mp-vaxis
/nix/store/c1qpqqm0m13mblz3f33saa27sxxd3m1x-wuffs
/nix/store/d6h0qkxnq0abkg7483c7x66b8sappdyd-z2d
/nix/store/dpqgcdf9nc4jn0dwbxbvy1vqvfyl77sa-vulkan_headers
/nix/store/dr9nnmiawcanp8wr7p99jdyb41yxfw3a-highway
/nix/store/f5zs2lbbx451slw0h91dlgldfv3cr63v-utfcpp
/nix/store/fl90p12n0yxvkhhn2kfhirhc320lgxpk-wayland_headers
/nix/store/gj6v5dbyacy8z9ph5llmagmf128vdl1x-iterm2_themes
/nix/store/h4n0xbm6dg132v7g0v3zq9g8qvrmclmr-ghostty-1.0.0-terminfo
/nix/store/h991k3x95mmig7i993dfyibbmiqn094p-fontconfig
/nix/store/hs1vqpyyw2xg04jpy7y21127iw1sjyyz-zigimg
/nix/store/ilx7scnpgxycfmj6cq5qf31spyykma84-libpng
/nix/store/q6jsp14xjmk08zqf8gg9hfc95448sghc-harfbuzz
/nix/store/qb3f9gd22c2sbp00ng1fpk7hmfk23wvm-zig_objc
/nix/store/qp1m1zci2n60idvv79sblq82lml18l6g-imgui
/nix/store/wa8zy9z7ngwn9mzhjpqxy1260dzngczj-glslang
/nix/store/xbwlxzyyzha763mz231bia72ybnzmbbc-mach_glfw
/nix/store/xk13b5q3c3x7rh1rqm355zf0y1zdgm9p-libxev
/nix/store/xwnpf0q80wijjnn4s7sk82syxdfzqhzh-zlib
/nix/store/p9p2056gs8cqqiwsnrpdvhk12rbd40si-ghostty-cache-1.0.0
/nix/store/w6skm2pz5dc6dk8w2g92hl2jd2q2yc49-oniguruma-6.9.9-lib
/nix/store/wqnzn50w9kflabjlnm8m3sm8k5kbwgf5-ghostty-1.0.0-shell_integration
/nix/store/xgfyyyxxpq5x6rd15w2xigwzdiv10gni-ghostty-1.0.0

Would be nice if we could get rid of those references. There seems to be a reference in the binary to /nix/store/p9p2056gs8cqqiwsnrpdvhk12rbd40si-ghostty-cache-1.0.0/1220bc6b9daceaf7c8c60f3c3998058045ba0c5c5f48ae255ff97776d9cd8bfc6402/imgui_demo.cpp. I think we should be able remove this without causing issues using removeReferencesTo.

Here is the fix, feel free to squash into your init commit: jcollie/nixpkgs@ghostty-init-1.0.0...fpletz:nixpkgs:pr/368404/remove-zig-src-refs

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

It may also be nice to pull glslang, spriv-cross, and simdutf as well since we have them in Nixpkgs: https://github.com/ghostty-org/ghostty/blob/35b9ceee2116331b83c0c86269394e2545070b0f/build.zig#L271-L280

pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/gh/ghostty/package.nix Show resolved Hide resolved
maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
pkgs/by-name/gh/ghostty/package.nix Show resolved Hide resolved
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Dec 27, 2024
@ofborg ofborg bot requested a review from pluiedev December 27, 2024 01:54
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Dec 27, 2024
@jcollie
Copy link
Contributor Author

jcollie commented Dec 27, 2024

@jcollie can you fix the formatting issue please ?

Formatting fixed.

Copy link
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

Thanks !
Let's merge this PR now. If any issue shows up, we can surely fix it in a follow up PR.

pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
@rhodes-b
Copy link

Can provide an option to build without gtk? kde users may want

No. The GLFW frontend is missing many features and is intended only for a rapid edit/compile/test cycle for developers. The macOS frontend only works on macOS, obviously. A native Qt frontend is possible, but that wildly out of the scope of a packaging request.

To make this more explicit this is a quote from mitchell below, and it will continue to be a developer only runtime

Note the GLFW build has bugs, memory corruption, crashes, and is missing dozens of features. It’s only used for Dev and isn’t suitable for daily use.

@jcollie
Copy link
Contributor Author

jcollie commented Dec 28, 2024

OK, I believe that I have addressed all of the requested changes at this point.

Copy link
Contributor

@pluiedev pluiedev left a comment

Choose a reason for hiding this comment

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

Let's do this

pkgs/by-name/gh/ghostty/package.nix Outdated Show resolved Hide resolved
@getchoo
Copy link
Member

getchoo commented Dec 28, 2024

@ofborg build ghostty

@getchoo
Copy link
Member

getchoo commented Dec 28, 2024

@ofborg build ghostty ghostty.passthru.tests
@ofborg test allTerminfo

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

$ nix-build -A ghostty --eval-system x86_64-linux
/nix/store/581anvgyq96jsams23xz85h5brr04jaq-ghostty-1.0.0
$ nix-build -A ghostty --eval-system aarch64-linux
/nix/store/2xdmahyczwsaabivpqzz30smfjvimc4n-ghostty-1.0.0

@getchoo getchoo dismissed GaetanLepage’s stale review December 28, 2024 03:04

Changes were addressed

@getchoo getchoo merged commit 3c6c692 into NixOS:master Dec 28, 2024
31 of 34 checks passed
@getchoo
Copy link
Member

getchoo commented Dec 28, 2024

Thanks for your work here @jcollie, as well as the many reviewers! 🥳

I will be making a follow up PR to address some of the other suggestions here, hopefully followed by a backport to 24.05 and Darwin support

And obligatory mention: we ended up becoming the second most upvoted PR in Nixpkgs
image

@Eveeifyeve
Copy link
Contributor

Eveeifyeve commented Dec 28, 2024

Thanks for your work here @jcollie, as well as the many reviewers! 🥳

I will be making a follow up PR to address some of the other suggestions here, hopefully followed by a backport to 24.05 and Darwin support

I will be making a pr that hopefully adds darwin support, but one of the things that need to be done first is swift 6 and 5.10 in nixpkgs then it will be possible.

getchoo pushed a commit to getchoo-contrib/nixpkgs that referenced this pull request Dec 29, 2024
* maintainers: add jcollie

* ghostty: init at 1.0.0

(cherry picked from commit 3c6c692)
Mic92 pushed a commit to Mic92/nixpkgs that referenced this pull request Dec 29, 2024
* maintainers: add jcollie

* ghostty: init at 1.0.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.