-
-
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
Add llvm packages 15 #209536
Add llvm packages 15 #209536
Conversation
Oh, we've got another #194634... |
Oh I only saw the closed PR. Should I close it ? The other PR seems to have bitrotted. |
That's not for me to decide. To be honest, I also have PRs unclosed while better PRs are coming in. 😅 (Looks like this PR does not build on aarch64-linux, while the other one does.)
I feel the same way you do. I'd rather see that PR merged NOW, and additional patches added gradually, so that other important PRs (GHC, Zig 0.10.0, etc) can come in. One out of 197 committers takes the step, the problem is solved, while the reality is not. |
pkgs/top-level/all-packages.nix
Outdated
inherit (stdenvAdapters) overrideCC; | ||
buildLlvmTools = buildPackages.llvmPackages_15.tools; | ||
targetLlvmLibraries = targetPackages.llvmPackages_15.libraries or llvmPackages_15.libraries; | ||
stdenv = llvmPackages_13.stdenv; |
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.
Why does it need stdenv from llvm 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.
Related: #184408
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 dropped support for clang12 and I tested on macos so stdenv is llvm)
Let me find the line with this info
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.
It probably should be under isDarwin
condition since gcc is used to build llvm on Linux.
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.
https://github.com/llvm/llvm-project/blob/llvmorg-14.0.6/libcxx/include/__config#L423-L424
These line are removed on llvmorg-15.0.6, I didn't find requirement in README or in documentation so I don't know minimal version of gcc. You are right I will add a guard.
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 It is not the correct way to require a recent version of clang because it has reference to llvm11 (from darwin stdenv):
$ nix-store -qR $(nix-store -qd /nix/store/n58zxqjx4lz785swd6xxcxv03la5lhfv-libcxxabi-15.0.6) | grep llvm
/nix/store/yww9hd80ikq72zrvkdk36picpqgvsh8b-llvm-11.1.0.src.tar.xz.drv
/nix/store/6wv8acdd7bm7npixgr3rk86mf55fj47c-llvm-config-link-static.patch
/nix/store/2mkz2cmqwg45xv7xfsj9idsb39maipmr-llvm-11.1.0.drv
/nix/store/cpnly3bmw6blsryxfgp2hs8ig2jz2aaz-llvm-13.0.1.drv
EDIT: after using nix-store --tree -q
I see no llvm11 as direct dependencies.
1fa1af9
to
fb8668c
Compare
|
Just for those who want to see if it builds for your platform:
|
I tested this patch on Linux with Zig (a tool which uses libllvm and libclang), and it seemed to work well. (#210324) |
Is this PR ready to merge? |
Really ? Can I have a build log or a way to build if I don't have arm machine. |
Here is the build log on ofborg: https://logs.nix.ci/?key=nixos/nixpkgs.209536&attempt_id=a26d3c8f-3178-4417-917a-bea85254c8c9 |
then why does hydra have all the checks set to green? edit: oh its the ones that are gray that didnt report a status back to github |
i cherry-picked this into https://github.com/milahu/nixpkgs/tree/flutter-engine/init
|
e5e43ea
to
0ce1ad3
Compare
…opment/compilers/llvm/15 Signed-off-by: Et7f3 <[email protected]>
0ce1ad3
to
8438df0
Compare
Co-authored-by: misuzu <[email protected]>
Co-authored-by: misuzu <[email protected]>
8438df0
to
8c01b12
Compare
It eval correctly on my machine so ofborg should be happy |
No new regression since 15.0.6 it did pass the same (disabled) tests on x86_64-darwin. |
still failing on darwin |
If it is about ofborg, ofborg stops because LLVM always takes more than an hour to build on darwin. This is perfectly normal. |
lldb is broken on x86_64-darwin:
|
I tried building lldb on Darwin x86_64. I have patches which will likely fix the two issues mentioned in @misuzu's comment. They will take a while to build on my old laptop though. See attached:
EDIT: My Libsystem patch doesn't work yet. I'll continue working on it. EDIT: The lldb patch doesn't work either. 🙈 I'll fix it. |
I don't think lldb is blocking zig and I think I saw patch in the other PR to fix lldb on darwin (so I didn't investigated that build failure). Unless I am wrong what is blocking is test failure in main compiler. |
I thought this PR (#209536) is for LLVM, not Zig.
Which platform and package? I built LLVM on Linux x86_64 and got no test failures. |
We can just apply 1d3ca42 in the other PR. |
Yes this PR is only for LLVM. Zig need one PR that add llvm to be merged. Since you opened a PR for zig I supposed you main interest was to unlock the situation and have one PR merged. Ah my bad I saw #209536 (comment) if those are fixed upstream then pick what doesn't build sure. |
What's the difference between PRs #209536 and #194634? Do they both do the same thing? Are both being maintained? I originally built my Zig patch (#210324) on top of this PR (#209536) because this LLVM PR (#209536) was open and the other LLVM PR (#194634) was marked as a draft. But now the other one (#194634) is approved and seems to have some of the bug fixes already...? I'm confused.
Yes, I want my Zig patch in, and if that means getting your PR in (or another similar PR), I'd do whatever I can to help get that PR in too. |
Yes, they do the same thing. That PR has been marked as a draft, mainly because it has higher goals, like targeting more platforms, enabling LLVM tests on macOS, so it took months before the author finally succeeded. Since I'm now a committer (aha!), I think I'll merge it if Zig builds on my (slow) aarch64-linux machine. |
Closing in favor of #194634. |
Do you get it on master ? Even with one jobs at the time ? if yes open an issue please, so we don't loose it here. |
Description of changes
I have taken example of primos pr #162104 if it is a command I put it in commit message otherwise I do it in a separate commit. so commit 2 and 4 are by hand. I also updated patch to fix line number
fix #191132
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@nektro