-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
crystal: 1.2 -> 1.7 #195606
crystal: 1.2 -> 1.7 #195606
Conversation
579fff9
to
41b908c
Compare
41b908c
to
e0d0d1c
Compare
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.
thank you for taking this on. There are a few things related to the version we keep around as well as comments.
feat: bump 1.6.x to 1.6.1 fix: fetch 12601 patch for 1.3.x-1.6.0 fix: darwin url for versions < 1.2.0
or > 1.5.x and aarch64-linux
I'm not really sure how to deal with the current error.
|
Also I'm still not super-convinced about compiling it in release mode, as straight-shoota mentioned:
It does take awfully long to compile the compiler AND the compiler spec AND run them when release mode is on. |
so 12601 gets applied successfully
]; | ||
|
||
LLVM_CONFIG = "${llvmPackages.llvm.dev}/bin/llvm-config"; | ||
|
||
FLAGS = [ | ||
"--release" |
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 guess we no longer need to do this as you're patching the versions that do not support it.
Otherwise good to. Once this is reinstated I'll squash and merge.
Thanks a ton @GeopJr !!!
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.
The release flag moved to makeFlags
(which has the same effect as far as I am aware) from the #173928 PR. Is there another reason to include it in FLAGS
?
ofborg still has an issue with the platforms that I am too unfamiliar with Nix to solve |
Thanks for pushing this. Looks like some failed depending pkgs need updates or pinning to 1.2.
Update: Invidious Tested.
Result: Invidious works great with this as-is. 🎉 Sign-up, login, subscriptions, trending, video viewing all work without error, and upgrading to this version from 1.2 finally cleans up some nasty log noise about unhandled SSL ctrl codes that's been a problem for ages. Ship it! ...at least when the other stuff is fixed! 😃 Result of 3 packages failed to build:
13 packages built:
|
Suggested fixes for downstream build errors: scry and mint: Quickfix by using crystal 1.2 for now. diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index 49d1abbf9d05f..ed61424497ce7 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -13455,7 +13455,7 @@ with pkgs;
icr = callPackage ../development/tools/icr { };
- scry = callPackage ../development/tools/scry { };
+ scry = callPackage ../development/tools/scry { crystal = crystal_1_2; };
dasm = callPackage ../development/compilers/dasm { };
@@ -14500,7 +14500,7 @@ with pkgs;
millet = callPackage ../development/tools/millet {};
- mint = callPackage ../development/compilers/mint { };
+ mint = callPackage ../development/compilers/mint { crystal = crystal_1_2; };
mitscheme = callPackage ../development/compilers/mit-scheme
{ stdenv = gcc10StdenvCompat; texLive = texlive.combine { inherit (texlive) scheme-small epsf texinfo; }; }; ameba: 1.0.1 -> 1.3.1. Couldn't immediately get 1.0.1 to build on old crystal. 1.3.1 builds fine on new crystal. diff --git a/pkgs/development/tools/ameba/default.nix b/pkgs/development/tools/ameba/default.nix
index 4239f5c0056a9..27068cbc4c49f 100644
--- a/pkgs/development/tools/ameba/default.nix
+++ b/pkgs/development/tools/ameba/default.nix
@@ -2,13 +2,13 @@
crystal.buildCrystalPackage rec {
pname = "ameba";
- version = "1.0.1";
+ version = "1.3.1";
src = fetchFromGitHub {
owner = "crystal-ameba";
repo = "ameba";
rev = "v${version}";
- hash = "sha256-dvhGk6IbSV3pxtoIV7+0+qf47hz2TooPhsSwFd2+xkw=";
+ hash = "sha256-SZ2sBQeZgtPOYioH9eK5MveFtWVGPvgKMrqsCfjoRGM=";
};
format = "make";
0 packages fail to build with the above. |
I believe you can use |
Apologies if you know the below already. Throwing in an x86 darwin build of the latest here. Looks like a similar ld error.
Result of 13 packages failed to build:
|
pkgs/top-level/all-packages.nix
Outdated
@@ -13889,18 +13889,17 @@ with pkgs; | |||
}; | |||
|
|||
inherit (callPackages ../development/compilers/crystal { | |||
llvmPackages = if stdenv.system == "aarch64-darwin" then llvmPackages_11 else llvmPackages_10; | |||
llvmPackages = llvmPackages_13; |
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.
Something I don't get from the x86-darwing error is that it refers to clang-11, but shouldn't it refer to clang-13?
Maybe we could try sticking with llvmPackages_11 here? I'm not sure for the underlying reason, but the llvm package in nixpkgs points to 11.
Or we are missing some path to use llvmPackages_13.clang
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.
:/ Closest discussion I could find: https://discourse.nixos.org/t/nixpkgs-aarch64-darwin-build-m1-uses-wrong-old-clang-how-to-fix/ (in our case x86_64 instead of aarch64)
the suggested change adds clang-unwrapped to PATH
nixpkgs/pkgs/build-support/libredirect/default.nix
Lines 35 to 49 in 6de1617
${if stdenv.isDarwin && stdenv.isAarch64 then '' | |
# We need the unwrapped binutils and clang: | |
# We also want to build a fat library with x86_64, arm64, arm64e in there. | |
# Because we use the unwrapped tools, we need to provide -isystem for headers | |
# and the library search directory for libdl. | |
# We can't build this on x86_64, because the libSystem we point to doesn't | |
# like arm64(e). | |
PATH=${bintools-unwrapped}/bin:${llvmPackages_13.clang-unwrapped}/bin:$PATH \ | |
clang -arch x86_64 -arch arm64 -arch arm64e \ | |
-isystem ${llvmPackages_13.clang.libc}/include \ | |
-isystem ${llvmPackages_13.libclang.lib}/lib/clang/*/include \ | |
-L${llvmPackages_13.clang.libc}/lib \ | |
-Wl,-install_name,$libName \ | |
-Wall -std=c99 -O3 -fPIC libredirect.c \ | |
-ldl -shared -o "$libName" |
Someone on darwin needs to give it a try otherwise I'll just revert to llvmPackages_11 for x86_64
The nix build definitely downloaded clang-13:
...
/nix/store/5b4s502i0ifbivjfb0pwxjlf7vrwgr2v-clang-13.0.1-lib
...
/nix/store/dx1q2d5zh3f45y8amkisy5lbq9b48zsx-clang-13.0.1
...
/nix/store/myqkicxg2zg8wmc84k048hn6fznilqm9-clang-wrapper-13.0.1
...
https://logs.nix.ci/?attempt_id=65e680ba-2ce4-4aae-8375-2b4107302113&key=nixos%2Fnixpkgs.195606
On macOS 12.6.1 (Monterrey)
Building
We can see that the missing On top of this branch I attempted to use llvm 13 clang by setting the
We can see that If I downgrade
I still get the
I think the problem might be on libevent package actually. The lib is already built and references Given that curl/curl#5210 is similar and it was resolved eventually with an xcode upgrade maybe the issue here is how libevent was built on x86-64_darwin initially. I attempted to change something in the libevent formula to trigger a rebuilt locally
Unfortunately, the I am not sure how to maybe tweak which xcode version should be used. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/unable-to-build-crystal-on-x86-64-darwin/24409/1 |
(At the time of writing this) While I made the Crystal package use sdk 11 in an attempt to get rid of Unfortunately the |
It finished! So our only issue now is |
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.
Awesome! @GeopJr it's great you found about stdenv override. How did you manage?
I think is not critical and it would be great if we have a non-broken crystal in nixpkgs 😊
Quite the rabbit hole - I was trying to find more info on
Awesome! I'll run a review to see if anything is broken on 1.7.1 and then try to move this forward! |
x86 darwin: Crystal itself looks like it's out of the woods! Nice! Result of 5 packages failed to build:
8 packages built:
|
Thanks for the review @miangraham! Unless someone on darwin wants to debug the failing packages, I'm ok with leaving them as is (if allowed), considering they haven't been built in darwin for ages and might need some workarounds from their maintainers. This doesn't seem to be a Crystal version issue: ameba, oq, thicket, crystal2nix should work on 1.7 and mint is locked to 1.2. |
Description of changes
Changelog
This is a copy of #173928 but for
1.2 -> 1.6
with some additional changes.Changes:
libffi
is needed nowTMPDIR=$TMP
(used in runtime forDir.tempdir
)aarch64-linux
as 1.5 is not yet availableNot sure if this should have been a review instead to the original draft but it targets different versions.
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/
)nixos/doc/manual/md-to-db.sh
to update generated release notes