buildLuaPackage: enable __structuredAttrs rocks #224553
Conversation
github-actions
bot
added
6.topic: cinnamon
teto
force-pushed
the
lua-structured-attrs
branch
from
bba4205 to
2a8ff4c
Compare
it makes overriding easier, instead of having to know internals to
decide which of `sqlite = prev.luaLib.overrideLuarocks prev.sqlite (drv: {` or
`sqlite = prev.sqlite.overrideAttrs (drv: {` just use the latter
teto
force-pushed
the
lua-structured-attrs
branch
from
2a8ff4c to
3f29874
Compare
|
This seems to have broken a lot in
I started trying to fix/revert some of this but I kept discovering more problems and gave up. I think this needs more attention before merging? |
|
do you have specific examples in mind ? maybe you hit this because you are running out of tree lua modules ? I ran nixpkgs-review on it and was able to build everything. There should be no need to pass a passthru anymore. The meta might be broken indeed. |
Well yes, this applies to anyone who might call or use these functions, now they rigidly only work specifically for the packages tested with. I'm under the impression that there's precedence (and expectation) that builders won't just ignore and blindly override input attrs passed by the package using them - all the code that was in place to prevent that from happening has been removed by this PR.
|
|
do you have some public reference of outside usage . Since so few people contribute to the lua ecosystem I am under the (apparently wrong) impression that no one uses it.
This PR changed a lot of things with the goal to make it easier to use: this removes the need to know when to call overrideLuarocks and overrideAttrs (tip: just use the latter). It took a lot of work and time to get there, if only because building those packages need CPU and disk storage. I am sorry if this broke your workflow but the fixes are trivial in comparison, especially now that the packages are cached. Do not hesitate to add tests to avoid this kind of regression. We have the distinction between unstable and stable precisely for this reason. And nixpkgs makes it easy to rollback. |
|
@teto passthru is very important even with structuredAttrs to avoid unnecessary attrs becoming a part of the derivation (causing unnecessary rebuilds) |
|
yes but it can still be used, just differently until I fix the call (and it is not used within nixpkgs) |
| broken = disabled; | ||
| } // meta; | ||
| } // attrs.meta; |
| inherit externalDeps; | ||
| inherit luarocks_content; | ||
| } // passthru; | ||
| }; |
There was a problem hiding this comment.
also env is important to preserve too
| @@ -167,7 +167,7 @@ let | |||
| wrapLuaPrograms | |||
| '' + attrs.postFixup or ""; | |||
|
|
|||
| installPhase = attrs.installPhase or '' | |||
There was a problem hiding this comment.
I am sorry if this broke your workflow but the fixes are trivial in comparison
I understand it's an impactful/breaking change to the builder in general, the only part I really take issue with here is that no care or attention was given to preserving input attrs.
The lines with or/++/// like this one (and all the others: meta, passthru, buildInputs, installPhase, checkPhase, etc) were removed while they could've just been left as-is - this leaves the function incorrect and less flexible than it was before, for no benefit or change. It's just silly to remove lines unrelated to your changes that serve a purpose 😞
follow up of NixOS#224553 where some arguments got ignored whil they were before taken into account.
follow up of #224553 where some arguments got ignored whil they were before taken into account.
it makes overriding easier, instead of having to know internals to decide which of
sqlite = prev.luaLib.overrideLuarocks prev.sqlite (drv: {orsqlite = prev.sqlite.overrideAttrs (drv: {just use the latterDescription of changes
Things done
sandbox = trueset 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/)