stdenv/darwin: remove workarounds and do cleanup#463900
stdenv/darwin: remove workarounds and do cleanup#463900emilazy merged 12 commits intoNixOS:stagingfrom
Conversation
f261bbc to
6a3489f
Compare
6a3489f to
29bdd60
Compare
| // { | ||
| inherit (super.llvmPackages) override; | ||
| inherit (super."llvmPackages_${llvmVersion}") override; | ||
| }; |
There was a problem hiding this comment.
I believe this produces incorrect results, as the .override will drop the .overrideScope changes. It was like this before for bug‐compatibility, but making sure .override and .overrideScope both work correctly seems like an important part of the clean‐up here – unless it’s intentional that these .overrideScopes get dropped completely when a user does llvmPackages.override? For early bootstrap stages I could see that potentially making sense, but is it what we want for the final .overrideScope?
Assuming we do want these .overrideScopes to be applied post‐.override, per #445668 (comment), I believe that what we’ll want to do here is define a function like overrideLlvmPackagesScope = llvmPackages: f: lib.makeOverridable (lib.mirrorFunctionArgs llvmPackages.override (args: (llvmPackages.override args).overrideScope f));. Then we can skip the // stuff here, and just use htat instead of .overrideScope. If this results in a measurable eval perf impact, then I think overrideLlvmPackagesScope = llvmPackages: f: llvmPackages.overrideScope f // { override = g: overrideLlvmPackagesScope (llvmPackages.override g) f; }; would avoid that at the expense of being less pleasant, but it probably isn’t necessary.
Of course having to do this kind of stuff is not very pleasant in general, but the various override interfaces interacting badly with each other is a known problem; we have special cases for .overrideAttrs but not .overrideScope. See #447012 and #454997 for the general case here; I’d like to work on unifying these interfaces rather than adding on more elaborate/generalized special cases every time it comes up, but I haven’t had the time lately…
There was a problem hiding this comment.
I thought it was like that because of the issue with overrideScope losing override not because of bug compatibility.
There was a problem hiding this comment.
Assuming we do want these .overrideScopes to be applied post‐.override, per #445668 (comment), I believe that what we’ll want to do here is define a function like
overrideLlvmPackagesScope = llvmPackages: f: lib.makeOverridable (lib.mirrorFunctionArgs llvmPackages.override (args: (llvmPackages.override args).overrideScope f));.
I tried this, but it didn’t work.
$ nix repl -f . --debugger --ignore-try
Lix 2.93.3
Type :? for help.
error: attribute 'compiler-rt' missing
at /Users/reckenrode/Developer/nixpkgs/pkgs/stdenv/darwin/default.nix:260:55:
259| llvmLibrariesPackages = prevStage: {
260| inherit (prevStage."llvmPackages_${llvmVersion}") compiler-rt libcxx;
| ^
261| };
Added 35 variables.
nix-repl> prevStage.llvmPackages_21
{
__functionArgs = { ... };
__functor = «lambda __functor @ /Users/reckenrode/Developer/nixpkgs/lib/trivial.nix:1018:17»;
}
nix-repl> prevStage.llvmPackages_21.__functionArgs
{
bootBintools = true;
bootBintoolsNoLibc = true;
buildPackages = false;
callPackage = false;
generateSplicesForMkScope = false;
lib = false;
llvmVersions = true;
patchesFn = true;
pkgs = false;
stdenv = false;
stdenvAdapters = false;
targetPackages = false;
}
If this results in a measurable eval perf impact, then I think
overrideLlvmPackagesScope = llvmPackages: f: llvmPackages.overrideScope f // { override = g: overrideLlvmPackagesScope (llvmPackages.override g) f; };would avoid that at the expense of being less pleasant, but it probably isn’t necessary.
This worked though.
There was a problem hiding this comment.
I pushed a commit that incorporates the second fix. llvmPackages.override { } should equal llvmPackages, and llvmPackages.overrideScope (_: _: { }) should equal it modulo the lost override issue.
| // lib.optionalAttrs (super.stdenv.targetPlatform == localSystem) { | ||
| # Make sure the following are all the same in the local == target case: | ||
| # - clang | ||
| # - llvmPackages.stdenv.cc | ||
| # - llvmPackages.systemLibcxxClang. | ||
| # - llvmPackages.clang | ||
| # - stdenv.cc | ||
| systemLibcxxClang = prevStage."llvmPackages_${llvmVersion}".systemLibcxxClang.override { | ||
| cc = finalLLVM.clang-unwrapped; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Do we definitely want this to be // and not .overrideScope? Again it seems like llvmPackages.override { } and llvmPackages.overrideScope { } should both be identical to llvmPackages.
There was a problem hiding this comment.
It’s in the overrideScope.
| { | ||
| inherit (prevStage."llvmPackages_${llvmVersion}") libllvm; | ||
| compiler-rt = prevStage."llvmPackages_${llvmVersion}".compiler-rt.override { | ||
| inherit (finalLLVM) libllvm; | ||
| }; | ||
| libclang = prevStage."llvmPackages_${llvmVersion}".libclang.override { | ||
| inherit (finalLLVM) libllvm; | ||
| }; | ||
| lld = prevStage."llvmPackages_${llvmVersion}".lld.override { | ||
| inherit (finalLLVM) libllvm; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Following on from my previous comment, I believe this means that (llvmPackages_21.override { }).compiler-rt is not the same as llvmPackages_21.compiler-rt, which seems very surprising.
I think this also means that (llvmPackages_21.override { libxml2 = …; }).libclang doesn’t work correctly, because the prevStage package set doesn’t get the overrides. llvmPackages_21.libclang.tests.withoutOptionalFeatures also seems like it would be incorrect in the presence of various kinds of overrides.
This is bootstrap build avoidance, right? I am wondering if the cure here might not be worse than the disease. It seems difficult to make dependency overrides propagate correctly while keeping things “incorrectly” one stage back in the default case, but maybe there’s a way we can do this closer to the root that would work reliably?
There was a problem hiding this comment.
I’ll reply more later when I’m at home, but this isn’t bootstrap avoidance. It’s so that overriding these does what you expect. Otherwise, overriding (for example) libllvm won’t rebuild dependent packages, which is surprising behavior.
There was a problem hiding this comment.
I see what you’re saying with llvmPackages.override { } not being equal to llvmPackages, but llvmPackages.overrideScope (_: _: { }) is identical except for the attributes that overrideScope drops. Even if we overlay nothing, scope.overrideScope (_: _: { }) will not equal scope. It should, but fixing that is outside the scope of this PR.
There was a problem hiding this comment.
Thinking about the “correct” way to do the bootstrap, it might be to drop the overlays completely and make buildPackages equal to prevStage. You’d probably need to do that a few times to go from the bootstrap tools to the final stage package set (assuming you want the final stage package set built with “its” compiler and not with the bootstrap one). That’s way outside the scope of this PR, and I’d prefer to work on other Darwin stuff before even messing with something like that.
There was a problem hiding this comment.
See above re: the llvmPackages vs. override vs. overrideScope issue.
300ac9a to
73f9135
Compare
0e0d2c4 to
0b825fd
Compare
It’s not ideal that the version has to be hardcoded, but right now it’s not recorded in any configuration accessible from the bootstrap outside of a package set. If that changes in the future, this should be changed to get `llvmVersion` from that source.
The LLVM derivation defines `llvm` as an alias for `libllvm`. The same is true for `clang-unwrapped` and `clang`. Doing the overrides this way allows overrides to work as expected (especially at the end).
This change allows `overrideScope` to work correctly outside of the bootstrap. Packages have to be overriden manually with the final `libllvm` to make sure any overrides to `libllvm` are picked up. The workaround is being dropped because rebuilds are expected. Note that the top-level `clang` is being dropped because it is an alias for `llvmPackages.clang`. Not overlaying it preserves the expected behavior if someone overlays `llvmPackages.clang` in their config (which is that the top-level one will be the same derivation as the overlay).
The Darwin bintools were moved to have all `darwin` packages in the same place. The LLVM ones are using the original, unaliased names.
Building `expand-response-params` early in the bootstrap will allow the custom wrappers (for `cc` and `bintools`) to be dropped.
This removes the need to keep these in sync, which should (hopefully) simplify maintenance.
This is the libc++ from the bootstrap. Once the past the xclang stage, the bootstrap uses the system libc++. Unfortunately, it doesn’t seem to work with the bootstrap tools.
0b825fd to
2441745
Compare
|
I dropped the backport label and rebased on current staging. @emilazy, is there anything else needed to move this forward? |
emilazy
left a comment
There was a problem hiding this comment.
Looks like this is working about as well as can be expected under the current system now, thanks!
| # Workaround for losing `override` after using `overrideScope` | ||
| # https://github.com/NixOS/nixpkgs/issues/447012 | ||
| overrideLlvmPackagesScope = | ||
| llvmPackages: f: | ||
| llvmPackages.overrideScope f | ||
| // { | ||
| override = g: overrideLlvmPackagesScope (llvmPackages.override g) f; | ||
| }; |
There was a problem hiding this comment.
Could you try:
| # Workaround for losing `override` after using `overrideScope` | |
| # https://github.com/NixOS/nixpkgs/issues/447012 | |
| overrideLlvmPackagesScope = | |
| llvmPackages: f: | |
| llvmPackages.overrideScope f | |
| // { | |
| override = g: overrideLlvmPackagesScope (llvmPackages.override g) f; | |
| }; | |
| # Workaround for losing `override` after using `overrideScope` | |
| # https://github.com/NixOS/nixpkgs/issues/447012 | |
| overrideLlvmPackagesScope = | |
| llvmPackages: f: | |
| let | |
| override = args: (llvmPackages.override args).overrideScope f; | |
| in | |
| lib.makeOverridable (lib.mirrorFunctionArgs llvmPackages.override override) { }; |
I think this will achieve the same thing but in a more override‐native way; I just messed up the arguments last time. If it still doesn’t work, feel free to just merge.
There was a problem hiding this comment.
It seemed to work. I pushed an update using this method instead.
| let | ||
| inherit (localSystem) system; | ||
|
|
||
| llvmVersion = "21"; # This needs to be updated when the default LLVM version is changed. |
There was a problem hiding this comment.
I assume we can’t get this out of the package set for infrec reasons?
There was a problem hiding this comment.
We don’t have any way of knowing what the default version actually is at this point because we don’t have a package set.
Co-authored-by: Emily <vcs@emily.moe>
2441745 to
ad2b074
Compare
This is a follow up to #445668. Many thanks to @LunNova for her work on that PR and to @RossComputerGuy for the initial work in #436350. Even though there are not as many deletions as I’d have liked, it has still allowed for a lot of cruft to be removed from the Darwin bootstrap.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.