-
-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
kitty: Fix compilation on aarch64-darwin #137512
Conversation
Hmm, this is supposed to be detected automatically by kitty's build system. See kovidgoyal/kitty#2876 and kovidgoyal/kitty#2997. In one of those PRs I added some code to kitty to compile a simple program with the new UserNotifications framework and if it compiles fine, it uses that framework, else the old one. The reason for doing this is that the new API doesn't work on older versions of macOS. |
|
nix-info -m
|
It seems
But it does not work as expected:
|
Given that the command line option you found doesn't seem to work, I think the best workaround is to create a temporary directory for the useless files. diff --git a/setup.py b/setup.py
index b737c3c7..e7831d6e 100755
--- a/setup.py
+++ b/setup.py
@@ -14,6 +14,7 @@
import sys
import sysconfig
import platform
+import tempfile
import time
from contextlib import suppress
from functools import partial
@@ -230,18 +231,19 @@ def get_sanitize_args(cc: str, ccver: Tuple[int, int]) -> List[str]:
def test_compile(cc: str, *cflags: str, src: Optional[str] = None, lang: str = 'c') -> bool:
src = src or 'int main(void) { return 0; }'
- p = subprocess.Popen(
- [cc] + list(cflags) + ['-x', lang, '-o', os.devnull, '-'],
- stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, stdin=subprocess.PIPE,
- )
- stdin = p.stdin
- assert stdin is not None
- try:
- stdin.write(src.encode('utf-8'))
- stdin.close()
- except BrokenPipeError:
- return False
- return p.wait() == 0
+ with tempfile.TemporaryDirectory() as tdir:
+ p = subprocess.Popen(
+ [cc] + list(cflags) + ['-x', lang, '-o', os.devnull, '-'], cwd=tdir,
+ stdin=subprocess.PIPE,
+ )
+ stdin = p.stdin
+ assert stdin is not None
+ try:
+ stdin.write(src.encode('utf-8'))
+ stdin.close()
+ except BrokenPipeError:
+ return False
+ return p.wait() == 0
def first_successful_compile(cc: str, *cflags: str, src: Optional[str] = None, lang: str = 'c') -> str: Then compile as above. If that doesn't work, do diff --git a/setup.py b/setup.py
index b737c3c7..0c945ed2 100755
--- a/setup.py
+++ b/setup.py
@@ -14,6 +14,7 @@
import sys
import sysconfig
import platform
+import tempfile
import time
from contextlib import suppress
from functools import partial
@@ -230,18 +231,19 @@ def get_sanitize_args(cc: str, ccver: Tuple[int, int]) -> List[str]:
def test_compile(cc: str, *cflags: str, src: Optional[str] = None, lang: str = 'c') -> bool:
src = src or 'int main(void) { return 0; }'
- p = subprocess.Popen(
- [cc] + list(cflags) + ['-x', lang, '-o', os.devnull, '-'],
- stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, stdin=subprocess.PIPE,
- )
- stdin = p.stdin
- assert stdin is not None
- try:
- stdin.write(src.encode('utf-8'))
- stdin.close()
- except BrokenPipeError:
- return False
- return p.wait() == 0
+ with tempfile.TemporaryDirectory() as tdir:
+ p = subprocess.Popen(
+ [cc] + list(cflags) + ['-x', lang, '-o', tdir + '/result', '-'],
+ stdin=subprocess.PIPE,
+ )
+ stdin = p.stdin
+ assert stdin is not None
+ try:
+ stdin.write(src.encode('utf-8'))
+ stdin.close()
+ except BrokenPipeError:
+ return False
+ return p.wait() == 0
def first_successful_compile(cc: str, *cflags: str, src: Optional[str] = None, lang: str = 'c') -> str: and try compiling again. |
First patch:
Second patch:
|
Try this patch: diff --git a/kitty/cocoa_window.m b/kitty/cocoa_window.m
index d3281ade..05f8acfd 100644
--- a/kitty/cocoa_window.m
+++ b/kitty/cocoa_window.m
@@ -11,7 +11,7 @@
#include "monotonic.h"
#include <Cocoa/Cocoa.h>
#ifndef KITTY_USE_DEPRECATED_MACOS_NOTIFICATION_API
-#include <UserNotifications/UserNotifications.h>
+#import <UserNotifications/UserNotifications.h>
#endif
#include <AvailabilityMacros.h>
diff --git a/setup.py b/setup.py
index b737c3c7..365bec2e 100755
--- a/setup.py
+++ b/setup.py
@@ -14,6 +14,7 @@
import sys
import sysconfig
import platform
+import tempfile
import time
from contextlib import suppress
from functools import partial
@@ -230,18 +231,19 @@ def get_sanitize_args(cc: str, ccver: Tuple[int, int]) -> List[str]:
def test_compile(cc: str, *cflags: str, src: Optional[str] = None, lang: str = 'c') -> bool:
src = src or 'int main(void) { return 0; }'
- p = subprocess.Popen(
- [cc] + list(cflags) + ['-x', lang, '-o', os.devnull, '-'],
- stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, stdin=subprocess.PIPE,
- )
- stdin = p.stdin
- assert stdin is not None
- try:
- stdin.write(src.encode('utf-8'))
- stdin.close()
- except BrokenPipeError:
- return False
- return p.wait() == 0
+ with tempfile.TemporaryDirectory() as tdir:
+ p = subprocess.Popen(
+ [cc] + list(cflags) + ['-x', lang, '-o', tdir + '/result', '-'],
+ stdin=subprocess.PIPE,
+ )
+ stdin = p.stdin
+ assert stdin is not None
+ try:
+ stdin.write(src.encode('utf-8'))
+ stdin.close()
+ except BrokenPipeError:
+ return False
+ return p.wait() == 0
def first_successful_compile(cc: str, *cflags: str, src: Optional[str] = None, lang: str = 'c') -> str:
@@ -383,7 +385,7 @@ def kitty_env() -> Env:
platform_libs = [
'-framework', 'CoreText', '-framework', 'CoreGraphics',
]
- test_program_src = '''#include <UserNotifications/UserNotifications.h>
+ test_program_src = '''#import <UserNotifications/UserNotifications.h>
int main(void) { return 0; }\n'''
user_notifications_framework = first_successful_compile(
ans.cc, '-framework UserNotifications', src=test_program_src, lang='objective-c')
diff --git a/shell.nix b/shell.nix
index c735f133..4995e97f 100644
--- a/shell.nix
+++ b/shell.nix
@@ -4,7 +4,7 @@ with pkgs;
let
inherit (lib) optional optionals;
inherit (xorg) libX11 libXrandr libXinerama libXcursor libXi libXext;
- inherit (darwin.apple_sdk.frameworks) Cocoa CoreGraphics Foundation IOKit Kernel OpenGL;
+ inherit (darwin.apple_sdk.frameworks) Cocoa CoreGraphics Foundation IOKit Kernel OpenGL UserNotifications;
harfbuzzWithCoreText = harfbuzz.override { withCoreText = stdenv.isDarwin; };
in
with python3Packages;
@@ -20,6 +20,7 @@ mkShell rec {
IOKit
Kernel
OpenGL
+ UserNotifications
libpng
python3
zlib |
And then enter the |
|
So this PR needs to download a patch for kitty after I made a PR upstream and add |
I think something like this should work (not tested): diff --git a/pkgs/applications/terminal-emulators/kitty/default.nix b/pkgs/applications/terminal-emulators/kitty/default.nix
index cfd46a613f7..c2645364d1a 100644
--- a/pkgs/applications/terminal-emulators/kitty/default.nix
+++ b/pkgs/applications/terminal-emulators/kitty/default.nix
@@ -11,6 +11,8 @@
IOKit,
Kernel,
OpenGL,
+ UserNotifications,
+ apple_sdk_version,
libcanberra,
libicns,
libpng,
@@ -45,6 +47,8 @@ buildPythonApplication rec {
libpng
python3
zlib
+ ] ++ lib.optionals (stdenv.isDarwin && lib.versionAtLeast apple_sdk_version "11.0") [
+ UserNotifications
] ++ lib.optionals stdenv.isLinux [
fontconfig libunistring libcanberra libX11
libXrandr libXinerama libXcursor libxkbcommon libXi libXext
diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index 89756d2669c..fb08998291a 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -978,7 +978,8 @@ with pkgs;
kitty = callPackage ../applications/terminal-emulators/kitty {
harfbuzz = harfbuzz.override { withCoreText = stdenv.isDarwin; };
- inherit (darwin.apple_sdk.frameworks) Cocoa CoreGraphics Foundation IOKit Kernel OpenGL;
+ inherit (darwin.apple_sdk.frameworks) Cocoa CoreGraphics Foundation IOKit Kernel OpenGL UserNotifications;
+ apple_sdk_version = darwin.apple_sdk.sdk.version;
};
lifecycled = callPackage ../tools/misc/lifecycled { }; The patch for kitty is still missing of course. |
Do you know why the evaluation fails and how to fix that? |
a7477f8
to
6343e31
Compare
Yeah, that looks much nicer than my version. |
Unfortunately it looks like it still doesn't evaluate properly yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
pkgs/top-level/all-packages.nix
Outdated
UserNotifications = if (builtins.hasAttr "UserNotifications" darwin.apple_sdk.frameworks) | ||
then darwin.apple_sdk.frameworks.UserNotifications | ||
else null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There needs to be another way than this hackery. Could we gate the usage of the framework behind a lib.optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I tried this first:
} // lib.optionalAttrs (builtins.hasAttr "UserNotifications" darwin.apple_sdk.frameworks) {
UserNotifications = darwin.apple_sdk.frameworks.UserNotifications;
};
But it does not work, see #137512 (comment) for log.
pinentry
in pkgs/top-level/all-packages.nix has similar logic:
pinentry = libsForQt5.callPackage ../tools/security/pinentry {
libcap = if stdenv.isDarwin then null else libcap;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pinentry in pkgs/top-level/all-packages.nix has similar logic:
I am going to change that right now because that is a really error prone coding style.
This is what it used to be at some earlier time:
Is that better? I don't see how else it could be done. |
I've tried this, It does not work, same error as the original issue. |
|
I think I like that more. |
Unfortunately that doesn't work as mentioned above. |
Please review the new changes. |
Cocoa | ||
CoreGraphics | ||
Foundation | ||
IOKit | ||
Kernel | ||
OpenGL | ||
darwin.apple_sdk.frameworks.Cocoa | ||
darwin.apple_sdk.frameworks.CoreGraphics | ||
darwin.apple_sdk.frameworks.Foundation | ||
darwin.apple_sdk.frameworks.IOKit | ||
darwin.apple_sdk.frameworks.Kernel | ||
darwin.apple_sdk.frameworks.OpenGL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this. I am not sure how to properly handle the UserNotifications framework but this is also not the solution. Maybe we should just wait until the SDK is bumped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@@ -5,6 +5,7 @@ | |||
lcms2, | |||
installShellFiles, | |||
dbus, | |||
darwin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most packages have UserNotifications as input, which defaults to null, and in the all-packages.nix the test if available is done. Any reason to not do that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
darwin.apple_sdk.frameworks.UserNotifications
does not exit in sdk 10, and x86_64-darwin use sdk 10.
Bumping this. Are we still waiting for something else? |
shell.nix is missing a depencency on the UserNotifications framework on macOS with Apple silicon. Unfortunately we can't just unconditionally include this dependency because Nixpkgs uses a different macOS SDK version for macOS on an Intel processor compared to macOS on Apple silicon. The older SDK version 10.12 on Intel does not have the UserNotifications framework, which would lead to an "attribute missing" error when trying to execute `nix-shell`. The condition can be dropped when the macOS SDK version for Intel processors is finally updated. See NixOS/nixpkgs#101229 for progress on that. See NixOS/nixpkgs#137512 for more context. Closes kovidgoyal#4352.
Could this have broken Kitty on x86_64-darwin? |
No, see kovidgoyal/kitty#4175 and #143987 |
Motivation for this change
See compilation error log in https://hydra.nixos.org/build/151584660/nixlog/1/tail
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)