sile: Add passthru, and change some pre/post hooks #175749
Conversation
ofborg
bot
added
11.by: package-maintainer
| @@ -9,13 +9,15 @@ | |||
| , icu | |||
| , fontconfig | |||
| , lua | |||
| # Use this to override the lua environment used by sile | |||
| , luaEnv ? null | |||
There was a problem hiding this comment.
can't you set the correct luaEnv as default instead ?
There was a problem hiding this comment.
I thought about that too, but occasionally I see reviews by @SuperSandro2000 that instruct not to do this (if that's what you meant):
| , luaEnv ? null | |
| , luaEnv ? lua.withPackages(ps: with ps; [...]) |
Because it may cause undefined behavior if lua is overridden as well. Perhaps @SuperSandro2000 may give a 2nd explanation as to why this is discouraged. I think it's also more readable the way it is now.
There was a problem hiding this comment.
For the sake of the default packages yes I think you can (as we were doing up until this PR). I think the issue is that this method makes it very difficult to modify the lua downstream in the Nix Flake for the project ... we can't add new Lua dependencies in the Flake as only the modifications since the last stable release. Background here: sile-typesetter/sile#1388 (comment)
There was a problem hiding this comment.
Yeah, doing that will mask eval errors when luaEnv would be removed/renamed.
What do you want to do? Adding new packages could be done via extraLuaPackages or similar.
There was a problem hiding this comment.
What we need downstream is some way to add dependencies (and occasionally remove dependencies) in the downstream flake.nix while still importing most of the dependency and build configuration from this package.
There was a problem hiding this comment.
Adding things is easy. Removing is more complicated. It's probably the easiest to just copy luaEnv and then change how you'd like.
There was a problem hiding this comment.
Adding things is easy.
I wasn't able to figure out a syntax that worked at all. Actually I think I did get the dependency added, but the dependency itself also failed to build when injected the way I was doing it.
My attempt is here: sile-typesetter/sile#1388 (comment)
There was a problem hiding this comment.
If there are not strong objections to this PR, and considering @alerque is a bit stressed to see this change in the next nix flake update, I'd like to merge this.
There was a problem hiding this comment.
th proposed change makes the expression harder to read for what seems to be not a great reason. You should be able to override the luaEnv by adding a new one to the buildInputs, just be careful in what order you specify it (new one should come first). Worst case you can override the current expression and just override fully the bits (buildInputs for instance) you are interested in
There was a problem hiding this comment.
new one should come first
Indeed adding the overridden luaEnv as the first buildInput works, thanks for that. I still think though that this proposal makes things much more elegant, (in general, we'd like to avoid writing things twice in the flake expression and in the expression here). Having eventually two luaEnv inputs seems not ideal, but I guess not harmful. Perhaps when __structuredAttrs will be supported in nixpkgs, we'd be able to override such inputs easily.
doronbehar
changed the title
|
I still would like to get some small changes in though. |
| @@ -59,19 +59,23 @@ stdenv.mkDerivation rec { | |||
| makeWrapper | |||
| ]; | |||
| buildInputs = [ | |||
| luaEnv | |||
There was a problem hiding this comment.
I am not sure changing its position here changes anyhting ? I just wanted to say: when you override buildinputs, do it like:
buildInputs = [ newLuaEnv ] ++ oldBuildInputs rather than buildInputs = oldBuildInputs ++ [ newLuaEnv ]
There was a problem hiding this comment.
This is what we will do, see this comment.
Motivation for this change
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/)nixos/doc/manual/md-to-db.shto update generated release notes