-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
emacs-macport: build on LLVM 14 #252244
emacs-macport: build on LLVM 14 #252244
Conversation
Result of 13 packages marked as broken and skipped:
9 packages failed to build:
5939 packages built:
|
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.
@AndersonTorres let me know if this (abed035) is what you had in mind!
@@ -84,6 +84,7 @@ | |||
, withSystemd ? lib.meta.availableOn stdenv.hostPlatform systemd | |||
, withToolkitScrollBars ? true | |||
, withTreeSitter ? lib.versionAtLeast version "29" | |||
, withUniformTypeIdentifiers ? variant == "macport" && lib.versionAtLeast stdenv.hostPlatform.darwinMinVersion "11" |
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.
Would it be necessary to check stdenv.isDarwin
here if we check variant == "macport"
?
Also, does the line need to be wrapped, and if so, how would you wrap it?
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.
One more question: does this need some kind of assert
guard to ensure that people don't accidentally turn it off when it's needed?
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.
Would it be necessary to check
stdenv.isDarwin
here if we checkvariant == "macport"
?
variant == "macport" -> stdenv.isDarwin
Also, does the line need to be wrapped, and if so, how would you wrap it?
It's fine.
One more question: does this need some kind of
assert
guard to ensure that people don't accidentally turn it off when it's needed?
Nah, that's on them.
I don't really see a reason why this needs to be configurable though? If you're modifying the drv on this level, you're using overrideAttrs anyways. I'd just inline this.
] ++ lib.optionals withUniformTypeIdentifiers [ | ||
UniformTypeIdentifiers |
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.
Is lib.optionals
good here or would lib.optional
be better?
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.
This is mostly a question of personal preference. This is good.
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.
optional
signifies to me that there's some deeper meaning behind there being exactly one element. If it's currently only one element but there's no specific reason why there might not be more further elements in the future, I prefer optionals
.
QuartzCore WebKit; | ||
Accelerate AppKit Carbon Cocoa GSS ImageCaptureCore ImageIO IOKit OSAKit | ||
Quartz QuartzCore WebKit; | ||
inherit (pkgs.darwin.apple_sdk_11_0.frameworks) UniformTypeIdentifiers; |
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.
UniformTypeIdentifiers
isn't present on 10.12, so I have to explicitly inherit from apple_sdk_11_0
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.
Is this compatible with the rest of Darwin-related attributes? No ABI clashing will occur?
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.
If we need the newer SDK here, please use the newer SDK for everything (pkgs.darwin.apple_sdk_11_0.callPackage too).
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.
Is this compatible with the rest of Darwin-related attributes? No ABI clashing will occur?
I think UniformTypeIdentifiers is a relatively isolated header-only library, but it will definitely be safer to just bump everything in tandem.
@@ -3,14 +3,14 @@ | |||
lib.makeScope pkgs.newScope (self: | |||
let | |||
gconf = pkgs.gnome2.GConf; | |||
inherit (self) callPackage; | |||
inherit (pkgs.darwin.apple_sdk_11_0) callPackage; |
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.
I think this should be fine for Linux too, viz. https://ryantm.github.io/nixpkgs/stdenv/platform-notes/
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.
Better not.
A long time ago inherit (pkgs) callPackage
caused a strange incompatibility bug - something about nativeBuildInputs
vs buildInputs
.
Use darwin.apple_sdk_11_0.callPackage
on all-packages.nix
instead.
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.
Use
darwin.apple_sdk_11_0.callPackage
onall-packages.nix
instead.
Sorry, can you clarify what this means? Something like this?
inherit (pkgs.darwin.apple_sdk_11_0) callPackage; | |
callPackage = pkgs.darwin.apple_sdk_11_0.callPackage; |
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.
No, they meant to callPackage emacs' default.nix from all-packages using the newer SDK.
What you wrote is syntactically the same thing as before.
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.
oh!! that was silly of me <_<
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.
Approving the wording, not the end product - because I have no Darwin machine to test.
Remember squashing all commits into only one. |
- Build on `apple_sdk_11_0` to support `aarch64-darwin` - Add frameworks required for building on newer SDKs - `Accelerate`, `UniformTypeIdentifiers` - Include header to work around `CF_NOESCAPE` issue circa LLVM 7.0
Thanks for the speedy review! |
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.
Diff LGTM and, more importantly, it actually works!
Huge thanks!
Backport failed for Please cherry-pick the changes locally. git fetch origin release-23.05
git worktree add -d .worktree/backport-252244-to-release-23.05 origin/release-23.05
cd .worktree/backport-252244-to-release-23.05
git checkout -b backport-252244-to-release-23.05
ancref=$(git merge-base e35f1fd9763f2b11b3f723b88287949cc2afd37c 685ad962ca44236b111be55fae057fbc435cc97e)
git cherry-pick -x $ancref..685ad962ca44236b111be55fae057fbc435cc97e |
@@ -0,0 +1,31 @@ | |||
#ifndef NOESCAPE_NOOP_H_ |
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.
Can this file be upstreamed?
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.
I am doing so — figuring out what is happening with the Homebrew build first so I get a concrete diagnosis for the maintainer.
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.
They use Apple Clang I believe which presumably behaves differently.
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.
Yes, but their issue tracker has a few reports of similar issues when self-compiled :/
This should not be backported. 23.05 will continue to provide LLVM6 until 23.05 EOL. |
This has broken
|
Ugh, it's because the |
Fixes regression caused by #252244. `env` was first defined in its own attrset, which was merged with a second attrset: ```nix ... { env = { NATIVE_FULL_AOT = "1"; LIBRARY_PATH = lib.concatStringsSep ":" libGccJitLibraryPaths; }; } // { ... env.NIX_CFLAGS_COMPILE = ... ... } ``` In this situation, the `env` from the first attrset is not preserved, since `//` does a shallow merge. Signed-off-by: Andrew Pan <[email protected]>
FYI this merge broke build of EDIT: perhaps I don't need to say it, but that build takes down thousands of packages with it. I just noticed when triaging mass regressions on staging-next; I'm interested neither in emacs nor in darwin stuff, really. |
Doing a quick Googling I found this: Homebrew/legacy-homebrew#43330 d12frosted/homebrew-emacs-plus#351 https://trac.macports.org/ticket/64162#no1 Maybe this should be the case of using LLVM/Clang to build in Darwin too? |
Sorry for all the breakage, everyone. Reproduced:
Initial diagnosis: seems to be SDK-related? But we migrated to only using |
Definitely due to an old libSystem lurking around:
I don't know how it gets there. |
I think it may be Proposed patch ( I haven't tested it yet, I'll do it tomorrow ) --- a/pkgs/applications/editors/emacs/default.nix
+++ b/pkgs/applications/editors/emacs/default.nix
@@ -4,13 +4,19 @@ lib.makeScope pkgs.newScope (self:
let
gconf = pkgs.gnome2.GConf;
inherit (self) callPackage;
+ stdenv = if pkgs.stdenv.isDarwin then pkgs.darwin.apple_sdk_11_0.stdenv else pkgs.stdenv;
inheritedArgs = {
inherit gconf;
+ inherit stdenv;
inherit (pkgs.darwin) sigtool;
inherit (pkgs.darwin.apple_sdk_11_0.frameworks)
Accelerate AppKit Carbon Cocoa GSS ImageCaptureCore ImageIO IOKit OSAKit
Quartz QuartzCore UniformTypeIdentifiers WebKit;
+ gnutls =
+ if pkgs.stdenv.isDarwin
+ then pkgs.gnutls.override { inherit stdenv; inherit (pkgs.darwin.apple_sdk_11_0.frameworks) Security; }
+ else pkgs.gnutls;
};
in { |
Built on my machine and it works, thanks! Dumb question: why is it necessary to pass the correct
... but how? Shouldn't apple_sdk_11_0.callPackage from all-packages.nix pass in a newer stdenv ?
|
I believe this is a bit because of the dark magic of splicing. |
@vcunat note that while the absolute number of packages is great, these packages are emacsPackages which are usually just a handful of elisp files that get AOT compiled in a generic way. They're like vim plugins. It's not great to have them fail obviously but it's also not like this has caused a wide fall-out like the pure number of packages would suggest. It's all contained in the Emacs ecosystem but, you know, it's the entire Emacs ecosystem that failed and it's huge. Thanks a bunch for keeping an eye on Hydra and notifying us! I had assumed at least someone tested this on x86_64-darwin aswell and IIRC I did build it successfully at some point but I'll be more diligent about that next time. @AndersonTorres I think this is more a case of the issues discussed in #242666. |
Description of changes
Compile emacs-macport with
llvmPackages_14
:Accelerate
andUniformTypeIdentifiers
headers__attribute__((noescape))
codegen issue with headerThis should fix the derivation on
aarch64-darwin
.Resolves #127902.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)