stdenv: Fix overriding + overrideAttrs
#134463
Conversation
| dontDisableStatic = true; | ||
| } // pkgs.lib.optionalAttrs (!(args.dontAddStaticConfigureFlags or false)) { | ||
| } // lib.optionalAttrs (!(args.dontAddStaticConfigureFlags or false)) { | ||
| configureFlags = (args.configureFlags or []) ++ [ |
There was a problem hiding this comment.
Just for reference, following a quick chat @Ericson2314 and I had:
For packages that unconditionally add --enable-shared in their configureFlags (for example: musl), this will effectively add both --enable-shared --disable-shared to the compilation command...
There was a problem hiding this comment.
yes this regression has been there since the introduction of dontAddStaticConfigureFlags
There was a problem hiding this comment.
I'm fixing a number of them in #133008
|
This break musl somehow: |
|
@symphorien I think cc @dtzWill @thoughtpolice (maintainers of |
|
well I rebased #133008 on this and got new failures due to that, so there must be a regression somewhere... |
|
@symphorien When trying to locate the "regression", I checked out 20.03, and |
|
On the parent commit of this pr, pkgsStatic.hello build, but not on this commit. The failure involves musl but maybe it's a red herring, since as you pointed out pkgsStatic.musl has not been building for a long time. |
|
@symphorien My guess is that this PR has just made the failure of |
Ericson2314
force-pushed
the
stdenv-adapter-latebind
branch
from
24da206
to
66df13b
Compare
The old stdenv adapters were subtly wrong in two ways:
- `overrideAttrs` leaked the original, unoverridden `mkDerivation`.
- `stdenv.override` would throw away any manually-set `mkDerivation`
from a stdenv reverting to the original.
Now, `mkDerivation` is controlled (nearly directly) via an argument, and
always correctly closes over the final ("self") stdenv. This means the
adapters can work entirely via `.override` without any manual `stdenv //
...`, and both those issues are fixed.
Note hashes are changed, because stdenvs no previously overridden like
`stdenvNoCC` and `crossLibcStdenv` now are. I had to add some
`dontDisableStatic = true` accordingly. The flip side however is that
since the overrides compose, we no longer need to override anything but
the default `stdenv` from which all the others are created.
We previously make it just be the function, not a single-item attrset, without deindenting to make a readable diff. No we deindent.
Ericson2314
force-pushed
the
stdenv-adapter-latebind
branch
from
66df13b
to
b4cc2a2
Compare
|
OK fixed the issue. The derivation to check is No, I am not sure why we want the dynamic build of Musl, but I didn't want to make a big behavioral change like that, so I just added |
|
I think this somehow removed |
|
Oh, I think this is happening because nixpkgs/pkgs/stdenv/booter.nix Lines 92 to 96 in abeef13 So if stdenv is overridden, these extra attributes are discarded. |
|
Yup that's why. I feel like the cure might be worse than the disease. In any event, I hope we might finally get away from that and the way bootstrapping is done today once #132343 is done. |
|
This broke the Also seeing a case where the relevant nixos config: |
Maybe broken by NixOS/nixpkgs#134463
|
I can confirm this ^ . Breaks |
|
I talked to @andir a bit in chat. |
NixOS/nixpkgs#134463 made keepDebugInfo obsolete for generic packages. This copies what keepDebugInfo used to do.
NixOS/nixpkgs#134463 made keepDebugInfo obsolete for generic packages. This copies what keepDebugInfo used to do.
NixOS/nixpkgs#134463 made keepDebugInfo obsolete for generic packages. This copies what keepDebugInfo used to do.
|
Should Filed #136756 |
NixOS/nixpkgs#134463 made keepDebugInfo obsolete for generic packages. This copies what keepDebugInfo used to do.
|
We should seriously consider simplifying the problem away. #136903 (comment) |
NixOS/nixpkgs#134463 made keepDebugInfo obsolete for generic packages. This copies what keepDebugInfo used to do.
| }); | ||
| }; | ||
| })); | ||
| } // lib.optionalAttrs (stdenv0.hostPlatform.libc == "libc") { |
There was a problem hiding this comment.
@Ericson2314 did you mean "glibc" here instead of "libc"?
@picostove reports that changing it fixes #244232.
There was a problem hiding this comment.
He did mean glibc #134923 (comment)
since that one had the typo, neither is doing anything.
Motivation for this change
The old stdenv adapters were subtly wrong in two ways:
overrideAttrsleaked the original, unoverriddenmkDerivation.stdenv.overridewould throw away any manually-setmkDerivationfrom a stdenv reverting to the original.
Now,
mkDerivationis controlled (nearly directly) via an argument, andalways correctly closes over the final ("self") stdenv. This means the
adapters can work entirely via
.overridewithout any manualstdenv // ..., and both those issues are fixed.Note hashes are changed, because stdenvs no previously overridden like
stdenvNoCCandcrossLibcStdenvnow are. I had to add somedontDisableStatic = trueaccordingly. The flip side however is thatsince the overrides compose, we no longer need to override anything but
the default
stdenvfrom which all the others are created.Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)