llvmPackages: Clean up outputs#111487
Conversation
12c9e24 to
05f8433
Compare
05f8433 to
6024d59
Compare
8f34317 to
7068520
Compare
9b8a3ba to
516acdd
Compare
c09afdf to
162f786
Compare
ead6b9b to
659faca
Compare
sternenseemann
left a comment
There was a problem hiding this comment.
Overall this looks very sane to me, although I haven't reviewed the patches and haven't done any build testing yet (what sort of testing have you done?).
I wonder if we can merge then -manpages packages into the main one or at least install the manpages to the man output if man pages are enabled (I guess having them always enabled complicates the bootstrap because of python).
There was a problem hiding this comment.
Why did you remove the getLib here?
There was a problem hiding this comment.
since cc is defined as libllvm.out, the output is "predetermined" and stuff like lib.getLib won't work. it's a bummer but the only solution I can think of is to do "out" and "bin" instead of "out" and "lib" (i.e change the default output).
There was a problem hiding this comment.
That makes sense. I wonder if this behavior of lib.get* is actually necessary, but it is probably not a thing we should include here if it is at all possible since it would need additional testing.
ffd09c0 to
d60cff0
Compare
Also begin to start work on cross compilation, though that will have to be finished later. The patches are based on the first version of https://reviews.llvm.org/D99484. It's very annoying to do the back-porting but the review has uncovered nothing super major so I'm fine sticking with what I've got. Beyond making the outputs work, I also strove to re-sync the packages, as they have been drifting pointlessly apart for some time. ---- Other misc notes, highly incomplete - lvm-config-native and llvm-config are put in `dev` because they are tools just for build time. - Clang no longer has an lld dep. That was introduced in db29857, but if clang needs help finding lld when it is used we should just pass it flags / put in the resource dir. Providing it at build time increases critical path length for no good reason. ---- A note on `nativeCC`: `stdenv` takes tools from the previous stage, so: 1. `pkgsBuildBuild`: `(?1, x, x)` 2. `pkgsBuildBuild.stdenv.cc`: `(?0, ?1, x)` while: 1. `pkgsBuildBuild`: `(?1, x, x)` 2. `pkgsBuildBuild.targetPackages`: `(x, x, ?2)` 3. `pkgsBuildBuild.targetPackages.stdenv.cc`: `(?1, x, x)`
d60cff0 to
7869d16
Compare
|
@jonringer I'm curious what you think about this re stability and 21.05. I think there will be some breakage from the output splitting, but it should be fairly rote to fix. |
|
I would feel a lot better if this had a hydra job associated with it, and we could better capture the regressions. That being said, the "breaking changes to staging" period hasn't started, so you're still free to merge. If you still feel confident that the changes will have minimal regressions, then I'll take you at your word. I'm more afraid for macos which has llvm as part of their stdenv, and more likely to have regressions that linux with its gccStdenv default |
|
@jonringer thanks. So one important distinction is that I think the breakages will be for packages that use LLVM and Clang as libraries, or are scrapping our tool chains for parts to assemble their own toolchain. That's basically the same small subset of packages on both platforms. Anything just using Clang via the default I'll merge and then, but will be monitoring this change on hydra especially once it hits staging-next. |
|
These may not be related to these particular changes, but I did a full hmm, thought there was going to be more when writing the post, just looks like one :) |
| ]; | ||
|
|
||
| cmakeFlags = [ | ||
| "-DCOMPILER_RT_OS_DIR=" |
There was a problem hiding this comment.
IIUC, this causes libclang_rt.osx.a to be installed to $out/lib, instead $out/lib/darwin as it was previously, which means it can no longer be found by the linker driver, which always uses darwin (see MachO::AddLinkRuntimeLib). I noticed it trying to cross compile m4 for aarch64-darwin, but I don't yet know how it works on x86_64-darwin.
Previously
result
└── lib
├── darwin
│ ├── libclang_rt.cc_kext.a
│ └── libclang_rt.osx.a
├── libclang_rt.cc_kext.a -> darwin/libclang_rt.cc_kext.a
└── libclang_rt.osx.a -> darwin/libclang_rt.osx.a
Now
result
└── lib
├── libclang_rt.cc_kext.a
└── libclang_rt.osx.a
There was a problem hiding this comment.
@thefloweringash sorry about that. Does this effect the current 7-based bootstrapping or just the new one?
|
I think this is the right place to ask about this regression? (link) |
|
Should be fixed as of #122399 |
|
So I assume we should fast-forward that fix to |
|
Perhaps you'd like to recommend which of the multiple llvmPackages changes in |
|
#122399 seems to be the most critical. #120790 could help some things, but I didn't encounter an edge case that was fixed by those cleanups at the time, so may not be urgent to fast track (at least for native compilation). There were some eval issues surrounding it, so may not be possible to merge on its own (see comment at the end of the PR). For #122655, I'd first see how it does in staging. |
|
Thanks @sternenseemann. I agree with that summary, and would just add #122044 to that. |
|
I took the pair and it looks like a big improvement. On x86_64-darwin this apparently fixed also occurrences of |
Motivation for this change
Inspired by #100574 but we should worry about testing cross after because this is already enough. This is going to require some better upstream support for split outputs.
I left a comment in https://reviews.llvm.org/D28234. If that can go through, I'm happy to backport or just hack upI made https://reviews.llvm.org/D99484, see the fairly long commit message there for more info.llvm-configanother way.Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)