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

Jump to bottom

kitty: support macOS #56740

Merged
merged 1 commit into from Mar 31, 2019
Merged

kitty: support macOS #56740

merged 1 commit into from Mar 31, 2019

Conversation

strager
Copy link
Contributor

@strager strager commented Mar 3, 2019 •
edited

Motivation for this change

Enable people to easily install and run kitty from source on macOS 10.12 and newer. This includes using kitty as a command-line utility (kitty) and as a macOS application (kitty.app`).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Additional information
  • Prefer libicns (png2icns) over Apple's non-free iconutil.
  • Work around warnings from Apple headers by disabling -pedantic-errors and -Werror.
  • Work around ld not support LLVM-LTO by disabling LTO.
  • Make Kitty and glfw compile for macOS 10.11 (and macOS 10.10).
  • Make an alias for libicns called png2icns to reduce confusion.

matthewbauer
matthewbauer reviewed Mar 3, 2019
View reviewed changes
pkgs/applications/misc/kitty/default.nix Show resolved Hide resolved
pkgs/applications/misc/kitty/default.nix
mkdir -p $terminfo/share
mv $out/share/terminfo $terminfo/share/terminfo
mv "$terminfo_src" $terminfo/share/terminfo
Copy link
Member

Choose a reason for hiding this comment

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

Does the app pick up the terminfo dirs?

Copy link
Contributor Author

@strager strager Mar 3, 2019 •
edited

Choose a reason for hiding this comment

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

Does the app pick up the terminfo dirs?

Are you asking if terminfo files are correctly installed? I'm not sure if the terminfo stuff is properly installed, but the files do exist:

$ cat /nix/store/nqyzdmi844za1vqqjh7n2s719ma15y9f-kitty-0.13.3/nix-support/propagated-user-env-packages
/nix/store/frhw04kcjzf0wfz3kby0ahkja10wh8rw-kitty-0.13.3-terminfo
$ find /nix/store/frhw04kcjzf0wfz3kby0ahkja10wh8rw-kitty-0.13.3-terminfo
/nix/store/frhw04kcjzf0wfz3kby0ahkja10wh8rw-kitty-0.13.3-terminfo
/nix/store/frhw04kcjzf0wfz3kby0ahkja10wh8rw-kitty-0.13.3-terminfo/share
/nix/store/frhw04kcjzf0wfz3kby0ahkja10wh8rw-kitty-0.13.3-terminfo/share/terminfo
/nix/store/frhw04kcjzf0wfz3kby0ahkja10wh8rw-kitty-0.13.3-terminfo/share/terminfo/78
/nix/store/frhw04kcjzf0wfz3kby0ahkja10wh8rw-kitty-0.13.3-terminfo/share/terminfo/78/xterm-kitty

Do you know how to verify that these terminfo files are working?

Note: kitty's build system is a bit weird. On macOS, it renames the UNIX share directory to kitty.app/Contents/Resources: https://github.com/strager/kitty/blob/v0.13.3/setup.py#L703

pkgs/applications/misc/kitty/default.nix Show resolved Hide resolved
pkgs/applications/misc/kitty/default.nix Outdated
pkgconfig which sphinx ncurses
] ++ stdenv.lib.optionals stdenv.isDarwin [
imagemagick
libicns
Copy link
Member

Choose a reason for hiding this comment

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

Libraries should be in buildInputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

libicns is used only for the png2icns program. png2icns is only used by kitty during compilation. kitty does not use the libicns library (at compile time or at run time).

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to add a comment about this.

matthewbauer
matthewbauer approved these changes Mar 3, 2019
View reviewed changes
Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

Ok looks to me then!

@strager
Copy link
Contributor Author

strager commented Mar 12, 2019

Do I need to do something in order to merge this?

@veprbl
Copy link
Member

veprbl commented Mar 12, 2019

@GrahamcOfBorg build kitty

@veprbl
Copy link
Member

veprbl commented Mar 12, 2019

For some reason this doesn't build on darwin. Complains about a missing header:

/tmp/nix-build-kitty-0.13.3.drv-0/source/kitty/logging.c:14:10: fatal error: 'os/log.h' file not found
#include <os/log.h>

@strager
Copy link
Contributor Author

strager commented Mar 13, 2019

For some reason this doesn't build on darwin. Complains about a missing header:

/tmp/nix-build-kitty-0.13.3.drv-0/source/kitty/logging.c:14:10: fatal error: 'os/log.h' file not found
#include <os/log.h>

kitty requires macOS 10.12 or newer.

I wrote a patch which backports kitty to macOS 10.11, but it wasn't merged upstream: kovidgoyal/kitty#1430

@veprbl
Copy link
Member

veprbl commented Mar 13, 2019

#56744 will provide os/log.h. Meanwhile we could use your patch.

@strager
Copy link
Contributor Author

strager commented Mar 30, 2019

@veprbl, I updated my branch to include the patch for macOS 10.11 (and 10.10).

@veprbl
Copy link
Member

veprbl commented Mar 30, 2019

@strager Let's not do the alias: it's not so uncommon for libfoo packages contain utilities, we don't want to later have to add alias for icns2png, there are already packages under those names out there. The comment in the kitty's code should suffice.

@strager
kitty: support macOS
d2688a4 
Install the `kitty` command-line utility and the `kitty.app` macOS
application.

* Prefer libicns (png2icns) over Apple's non-free iconutil.
* Work around warnings from Apple headers by disabling -pedantic-errors
  and -Werror.
* Work around ld not support LLVM-LTO by disabling LTO.
* Make Kitty and glfw compile for macOS 10.11 (and macOS 10.10).
@veprbl
Copy link
Member

veprbl commented Mar 31, 2019

@GrahamcOfBorg build kitty

veprbl
veprbl approved these changes Mar 31, 2019
View reviewed changes
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Looks great!

@veprbl veprbl merged commit 75aa846 into NixOS:master Mar 31, 2019
@strager strager deleted the kitty-macos branch April 18, 2019 16:34
veprbl pushed a commit that referenced this pull request May 13, 2019
@strager @veprbl
kitty: support macOS (#56740)
620c95c 
Install the `kitty` command-line utility and the `kitty.app` macOS
application.

* Prefer libicns (png2icns) over Apple's non-free iconutil.
* Work around warnings from Apple headers by disabling -pedantic-errors
  and -Werror.
* Work around ld not support LLVM-LTO by disabling LTO.
* Make Kitty and glfw compile for macOS 10.11 (and macOS 10.10).

(cherry picked from commit 75aa846)

cc #56740
@veprbl veprbl added the 8.has: port to stable A PR already has a backport to the stable release. label May 13, 2019
@Luflosi Luflosi mentioned this pull request Oct 3, 2020
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer approved these changes

@veprbl veprbl veprbl approved these changes

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 8.has: package (new) 8.has: port to stable A PR already has a backport to the stable release. 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@strager @veprbl @matthewbauer @GrahamcOfBorg