llvmPackages: Reinstate overriding across whole package set#341855
llvmPackages: Reinstate overriding across whole package set#341855RossComputerGuy merged 1 commit intoNixOS:masterfrom
Conversation
github-actions
bot
added
the
6.topic: llvm/clang
|
I'm going to give this a bit of testing and report back if it works for me. When I'm happy I'll mark it ready for review. |
ofborg
bot
added
10.rebuild-darwin: 0
Pull NixOS#320261 introduced the possibility to consistently override dependencies within an llvm package set. I think NixOS#325175 accidentally dropped this, so reinstate it. Signed-off-by: Peter Waller <[email protected]>
pwaller
force-pushed
the
reinstate-llvmPackages-overrides
branch
from
e152016 to
ddafd4b
Compare
|
I ran /cc other reviewers of #325175 who may me interested: @paparodeo @reckenrode @alyssais |
cmake flags have a 'last flag wins' logic, so by appending to the end of the flags it is possible to override any cmake flag. It also ignores (and warns) if a flag is unused, so passing flags across all packages should be safe if you want to target one package. In combination with NixOS#320261, this PR allows consistently overriding all packages within LLVM with additional cmake arguments. Consistency here means for example 'if you override LLVM, then all dependencies on it are also see the overridden LLVM in their input'. Consistency is hard to achieve with the other obvious way of implementing this as a user: if you use overrideAttrs then you have to write a big mess of override code in order to override all dependents, and this can be very difficult in a cross-compilation scenario using crossSystem and useLLVM, for example. With this PR it is possible to write an overlay which overlays `llvmPackages` with `llvmPackage.override { devExtraCmakeFlags = [ ... ]; }`, and then the toolchain used with useLLVM in effect should respect these flags. This is useful in development for experimenting with the effect of various flags, hence the chosen name `devCmakeFlags`. This won't work out of the box without NixOS#341855 applied, which fixes override passthrough. See-Also: NixOS#320261, NixOS#341855 Signed-off-by: Peter Waller <[email protected]>
There was a problem hiding this comment.
verified same tests from #320261 (review) ran as expected.
looks good. minor comment about overriding stdenv inline.
| version | ||
| ; | ||
| } | ||
| // packageSetArgs # Allow overrides. |
There was a problem hiding this comment.
should this go after the stdenv so it (stdenv) can get overridden too?
There was a problem hiding this comment.
My thinking was that this involves conditional logic to pull in a different stdenv; that conditional logic still needs to apply. If the user wants to override gcc12Stdenv or stdenv they still can, through the original argument mechanism. What we don't want is to override the stdenv which has been conditionally set to gcc12Stdenv with the original stdenv.
There was a problem hiding this comment.
What we don't want is to override the stdenv which has been conditionally set to gcc12Stdenv with the original stdenv.
i think that is exactly what we want -- an override that does an override. anyway, i don't think this needs needs to hold up merging given it is pretty minor and the llvm which pins gcc12 will eventually get dropped.
There was a problem hiding this comment.
FWIW i don't think this comment should've been marked as resolved but i am unable to change to unresolved.
There was a problem hiding this comment.
Marked unresolved for you. I think we need someone to break the tie. My point is that the recipe has conditional logic which pulls in the correct stdenv required to build a given LLVM version. If I splat the args in after the conditional logic, the original stdenv would override the conditional logic. This is clearly not what we want. So, I reason that if we put the conditional logic after the args, it's still possible for someone to apply overrides; they just need to override gcc12Stdenv on the input if the conditional logic is in effect.
There was a problem hiding this comment.
ok. i am just being dense. moving packageSetArgs below the conditional breaks llvm13 as clobbers stdenv . so that won't work.
tho -- i am getting build failures just trying to build llvmPackages_13 from this PR with no modifications.
There was a problem hiding this comment.
Thanks for testing - can you confirm that the commit on which this is based will build llvmPackages_13? (If not just say and I'll do it when I get a chance)
There was a problem hiding this comment.
but maybe it was marked broken on master.
error: Package ‘lldb-13.0.1’ in is marked as broken, refusing to evaluate.
There was a problem hiding this comment.
Thanks for testing - can you confirm that the commit on which this is based will build llvmPackages_13? (If not just say and I'll do it when I get a chance)
4f807e8 appears to have the same issue
error: build of '/nix/store/4813qnnz6ymba266v0yjas8b8j6faggl-llef-1.2.0.drv', '/nix/store/s3n5dm0s2x57yr18mqrzy8r0j1nvhzi3-lldb-13.0.1.drv' failed
wegank
added
the
12.approvals: 1
RossComputerGuy
added
12.approvals: 2
|
Shall we merge? |
4 participants
Description of changes
Pull #320261 introduced the possibility to consistently override
dependencies within an llvm package set. I think #325175 accidentally
dropped this, so reinstate it.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.