nixos/shells-pkgs: added environment.shellPkgs option #110762
Conversation
|
After a quick read through I'm left wanting an example of when you might use this option. Just out of curiosity. |
|
@aanderse the reason being that most shell plugins contain the script to be |
|
Oh get it now. Thanks for explaining. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
pasqui23
requested review from
sternenseemann,
mweinelt and
Ma27
and removed request for
mweinelt
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
There was a problem hiding this comment.
Cool idea! My thoughts are the following:
- The code is a bit hard to read. More comments explaining and less aliases for stuff we know would be nice :)
- I feel like this module is too magic. I'd prefer it if
environment.shellPkgswas namespaced per Shell (i. e.environment.shellPkgs.fish, …), alternativelyprograms."${shell}".shellPkgs` could also be interesting. This would have the following benefits in my eyes:- Users generally want to install stuff for a specific shell, not specific stuff for all shell which then only supports one shell anyways.
- If someone wants to install a zsh package, but uses say fish, with
environment.shellPkgsit may be confusing why it didn't work. If it was namespaced we could add an assertion to make sure that doesn't happen or make evaluation fail.
- The extra output is a neat idea, but also not nice that we duplicate everything effectively. Maybe there is an alternative solution like (ab)using
passthruand adding asupportedShellslist or something. - Needs documentation in the
nixpkgsmanual, so packagers know how to package a shell package to support this.
Just my 2 cents, let me know what you think on those points!
|
@sternenseemann Added documentation , |
pkgs/tools/misc/direnv/default.nix
Outdated
| ${lib.concatMapStrings | ||
| ( | ||
| sh: '' | ||
| $out/bin/direnv hook ${sh} > $${sh}_interactiveShellInit |
There was a problem hiding this comment.
$${foo} escapes the ${foo}.
| $out/bin/direnv hook ${sh} > $${sh}_interactiveShellInit | |
| $out/bin/direnv hook ${sh} > ''$${sh}_interactiveShellInit |
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
|
Still developing it |
stale
bot
removed
the
2.status: stale
|
Still waiting on @sternenseemann ,@Ma27 or @mweinelt to review this |
| Those need to have extra outputs called `${shell}_${phase}` where: | ||
| * shell refers to the specific shell it needs to be included (either bash, fish or zsh). | ||
| * phase refers to when the file will be sourced, this can be one of: | ||
| * * `interactiveShellInit`,if it needs to be included in the interactive shell initialization |
There was a problem hiding this comment.
| * * `interactiveShellInit`,if it needs to be included in the interactive shell initialization | |
| * * `interactiveShellInit` if it needs to be included in the interactive shell initialization |
| outputs = [ "out" "interactiveShellInit_zsh"]; | ||
|
|
||
| installPhase = '' | ||
| install -Dm0644 fzf-zsh.plugin.zsh $out/share/zsh/plugins/fzf-zsh/fzf-zsh.plugin.zsh | ||
| install -Dm0644 fzf-zsh.plugin.zsh $interactiveShellInit_zsh |
There was a problem hiding this comment.
I think it would be a better design to only have the interactiveShellInit output and subdirectories for bash, fish and zsh. This would also be easier to extend.
There was a problem hiding this comment.
I've chosen this design for a couple of reason:
- It allow users of, say, zsh to not pay for fish plugins
- It allows
programs,zsh,shellPkgsto contain only packages with zsh plugins - In the immediate future I intend to create (either here or on home-manager) a module that allows me to manage zsh plugins to be loaded and managed by zplugin to improve parsing even further.
- Especially point 3 require IFD with your design
In short I need to filter plugins by shell without IFD and I can't do that with per shell subdirs
There was a problem hiding this comment.
As for extensibility, you just have to add to the lists in the imports in https://github.com/NixOS/nixpkgs/blob/a4e97999decd694aba59773ae384d030a97e08e0/nixos/modules/config/shells-pkgs.nix#L71
pkgs/tools/misc/direnv/default.nix
Outdated
|
|
||
| # replace the build phase to use the GNUMakefile instead | ||
| buildPhase = '' | ||
| make BASH_PATH=$BASH_PATH | ||
| ''; | ||
|
|
||
| outputs = [ "out" ] ++ map (sh: "${sh}_interactiveShellInit"); |
There was a problem hiding this comment.
Why do we have once the shells at the end and here at the front? Not very consistent.
There was a problem hiding this comment.
I don't understand
There was a problem hiding this comment.
ok now I get it,it was an error
pasqui23
force-pushed
the
shplugins
branch
5 times, most recently
from
0d0fae3
to
85fc860
Compare
Co-authored-by: sterni <sternenseemann@systemli.org>
Co-authored-by: Manuel Mendez <708570+mmlb@users.noreply.github.com>
stale
bot
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
Motivation for this change
Nowadays nixpkgs contains several packages containing snippets to be added to shells initializations, however those snippets are often in disparate places and need to read the package before knowing the specific path to be sourced.
This option allow for a programmatic addition of shell snippets, as simply as
environment.sessionPackagesThings done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)