Skip to content

Comments

kitty: support macOS#56740

Merged
veprbl merged 1 commit intoNixOS:masterfrom
strager:kitty-macos
Mar 31, 2019
Merged

kitty: support macOS#56740
veprbl merged 1 commit intoNixOS:masterfrom
strager:kitty-macos

Conversation

@strager
Copy link
Contributor

@strager strager commented Mar 3, 2019 •
edited
Loading

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.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: darwin Running or building packages on Darwin 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 3, 2019
matthewbauer
matthewbauer reviewed Mar 3, 2019
View reviewed changes
pkgs/applications/misc/kitty/default.nix Outdated
Copy link
Member

@matthewbauer matthewbauer Mar 3, 2019

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
Loading

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 Outdated
Copy link
Member

@matthewbauer matthewbauer Mar 3, 2019

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

@strager strager Mar 3, 2019

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

@veprbl veprbl Mar 12, 2019

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 This 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

@veprbl veprbl veprbl approved these changes

+1 more reviewer

@matthewbauer matthewbauer matthewbauer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: darwin Running or building packages on Darwin 8.has: package (new) This PR adds a new package 8.has: port to stable This PR already has a backport to the stable release. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@strager @veprbl @matthewbauer @GrahamcOfBorg