Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

installShellFiles: init #65211

Closed
wants to merge 1 commit into from

Conversation

@lilyball
Copy link
Member

commented Jul 21, 2019

This is a new package that provides a shell hook to make it easy to declare manpages and shell completions in a manner that doesn't require remembering where to actually install them. This is intended for packages that don't have a make install that does this already. Usage looks like

{ stdenv, installShellFiles, ... }:
stdenv.mkDerivation {
  # ...
  nativeBuildInputs = [ installShellFiles ];
  postInstall = ''
    installManPage doc/foobar.1
    installShellCompletion share/completions/foobar.{bash,fish}
    installShellCompletion --zsh share/completions/_foobar
  '';
  # ...
}

There's detailed documentation in the header comments of the shell functions.

Motivation for this change

I got tired of having to look up the shell completion paths for the various shells when writing derivations. It's also a centralized location to teach about other shells if anyone cares about adding completions (e.g. ffsend has .elv and .ps1 completions in its source tree but I have no idea where those get installed).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

I tested this by writing a dummy derivation whose source contained manpages and shell completions and then invoked this shell hook.

@lilyball lilyball requested a review from Ericson2314 as a code owner Jul 21, 2019
@nixos-discourse

This comment has been minimized.

Copy link

commented Jul 21, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/proposal-make-it-easier-to-define-manpages-shell-completions/3491/6

@lilyball

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2019

I haven't updated any packages to use this yet as that will require the package to be rebuilt. I wasn't sure if it was worth it when there's no functional difference. If this gets merged I'll try to start adopting it any time I update a package that has manpages or shell completions.

@lilyball lilyball force-pushed the lilyball:installShellFiles branch from 1e57f2a to 89ace0d Jul 21, 2019
@lilyball lilyball force-pushed the lilyball:installShellFiles branch from 89ace0d to 87e9d71 Jul 21, 2019
@lilyball

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

I just tested what happens with a multi-output derivation, and it appears to work just fine, because it seems that share/man gets moved into the man output (if it exists) in a default preFixup hook, meaning my hook doesn't need to care about the possibility of a man output.

That said, I'm thinking I should probably just go ahead and use ${!outputMan} and ${!outputDevman} after all instead of relying on that preFixup hook.

@lilyball lilyball force-pushed the lilyball:installShellFiles branch from 87e9d71 to bf5d616 Jul 25, 2019
@lilyball

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

I've updated it to use ${!outputMan} and ${!outputDevman} explicitly. I also went with ${!outputBin} for the shell completions, but I'm not sold on that one, maybe I should just keep that in $prefix.

@bjornfor

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

The usage of installShellFiles as a function is because attrsets can't serialize to the environment normally, so it converts the structured attrs into a format the shell hook can consume.

FYI, since Nix 2.0 you can set __structuredAttrs to get what you want. It's only used two places in nixpkgs, but since nixpkgs requires nix 2.0 now anyways, I don't see why it shouldn't be used more.

@lilyball

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

__structuredAttrs is set on the whole derivation, not on individual nested attrsets, and its use completely breaks the default builder. Even if the default builder worked with it, requiring it to be set on the derivation would be a significant hurdle to using this on arbitrary packages. And of course Bash doesn't have any built-in tools for dealing with JSON so I'd have to add a dependency on something like jq and write a complicated jq script to transform it into something usable by Bash.

@lilyball lilyball force-pushed the lilyball:installShellFiles branch from bf5d616 to ca87133 Jul 26, 2019
@lilyball

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

Updated again so the installShellFiles function adds a __toString attribute instead of immediately serializing it to a string. This way you can still query the structured shell completions in Nix code.

@nixos-discourse

This comment has been minimized.

Copy link

commented Jul 30, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/32

@lilyball lilyball force-pushed the lilyball:installShellFiles branch from ca87133 to 3db2370 Jul 30, 2019
@lilyball

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

Updated again because I screwed up the handling of escaped characters in shell completion paths.

@lilyball lilyball force-pushed the lilyball:installShellFiles branch 2 times, most recently from dd2e403 to c6c3321 Jul 31, 2019
@lilyball

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

Added support for renaming shell completions, including automatically renaming zsh completions because apparently zsh requires the convention _foo and packages might not necessarily have it that way in the source.

As an example I updated the nixpkgs.exa derivation to use this, including manual renaming support. The diff looks like

diff --git a/pkgs/tools/misc/exa/default.nix b/pkgs/tools/misc/exa/default.nix
index 7c43638ea56..f51fe9e2982 100644
--- a/pkgs/tools/misc/exa/default.nix
+++ b/pkgs/tools/misc/exa/default.nix
@@ -1,5 +1,5 @@
 { stdenv, fetchFromGitHub, rustPlatform, cmake, perl, pkgconfig, zlib
-, darwin, libiconv
+, darwin, libiconv, installShellFiles
 }:
 
 with rustPlatform;
@@ -17,25 +17,18 @@ buildRustPackage rec {
     sha256 = "14qlm9zb9v22hxbbi833xaq2b7qsxnmh15s317200vz5f1305hhw";
   };
 
-  nativeBuildInputs = [ cmake pkgconfig perl ];
+  nativeBuildInputs = [ cmake pkgconfig perl installShellFiles ];
   buildInputs = [ zlib ]
   ++ stdenv.lib.optionals stdenv.isDarwin [
     libiconv darwin.apple_sdk.frameworks.Security ]
   ;
 
-  postInstall = ''
-    mkdir -p $out/share/man/man1
-    cp contrib/man/exa.1 $out/share/man/man1/
-
-    mkdir -p $out/share/bash-completion/completions
-    cp contrib/completions.bash $out/share/bash-completion/completions/exa
-
-    mkdir -p $out/share/fish/vendor_completions.d
-    cp contrib/completions.fish $out/share/fish/vendor_completions.d/exa.fish
-
-    mkdir -p $out/share/zsh/site-functions
-    cp contrib/completions.zsh $out/share/zsh/site-functions/_exa
-  '';
+  manpages = [ "contrib/man/exa.1" ];
+  shellCompletions = installShellFiles {
+    bash = { name = "exa"; path = "contrib/completions.bash"; };
+    fish = { name = "exa.fish"; path = "contrib/completions.fish"; };
+    zsh  = { name = "_exa"; path = "contrib/completions.zsh"; };
+  };
 
   # Some tests fail, but Travis ensures a proper build
   doCheck = false;

I'm not committing this change because it requires rebuilding exa even though the resulting file tree doesn't change.

@rycee

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

An alternative solution would be to have the hook introduce shell functions such that the postInstall becomes something like:

installManPage contrib/man/exa.1
installShellCompletion --bash --name exa contrib/completions.bash
installShellCompletion --fish --name exa.fish contrib/completions.fish
installShellCompletion --zsh --name _exa contrib/completions.zsh

I.e., without needing to introduce new attributes in the derivation.

@lilyball

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

I rewrote this setup hook to vend installManPage and installShellCompletion functions instead, as suggested. I pushed it as a separate commit to the PR so we can see both versions, but if this PR is accepted I'll squash them together.

@lilyball

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

With this new version the patch to exa looks like

diff --git a/pkgs/tools/misc/exa/default.nix b/pkgs/tools/misc/exa/default.nix
index 7c43638ea56..d3f27b9814e 100644
--- a/pkgs/tools/misc/exa/default.nix
+++ b/pkgs/tools/misc/exa/default.nix
@@ -1,5 +1,5 @@
 { stdenv, fetchFromGitHub, rustPlatform, cmake, perl, pkgconfig, zlib
-, darwin, libiconv
+, darwin, libiconv, installShellFiles
 }:
 
 with rustPlatform;
@@ -17,24 +17,17 @@ buildRustPackage rec {
     sha256 = "14qlm9zb9v22hxbbi833xaq2b7qsxnmh15s317200vz5f1305hhw";
   };
 
-  nativeBuildInputs = [ cmake pkgconfig perl ];
+  nativeBuildInputs = [ cmake pkgconfig perl installShellFiles ];
   buildInputs = [ zlib ]
   ++ stdenv.lib.optionals stdenv.isDarwin [
     libiconv darwin.apple_sdk.frameworks.Security ]
   ;
 
   postInstall = ''
-    mkdir -p $out/share/man/man1
-    cp contrib/man/exa.1 $out/share/man/man1/
-
-    mkdir -p $out/share/bash-completion/completions
-    cp contrib/completions.bash $out/share/bash-completion/completions/exa
-
-    mkdir -p $out/share/fish/vendor_completions.d
-    cp contrib/completions.fish $out/share/fish/vendor_completions.d/exa.fish
-
-    mkdir -p $out/share/zsh/site-functions
-    cp contrib/completions.zsh $out/share/zsh/site-functions/_exa
+    installManPage contrib/man/exa.1
+    installShellCompletion --name exa contrib/completions.bash
+    installShellCompletion --name exa.fish contrib/completions.fish
+    installShellCompletion --name _exa contrib/completions.zsh
   '';
 
   # Some tests fail, but Travis ensures a proper build
@rycee
rycee approved these changes Aug 2, 2019
Copy link
Member

left a comment

I like the functions better 🙂

Thanks! This hook seems very helpful.

This is a new package that provides a shell hook to make it easy to
declare manpages and shell completions in a manner that doesn't require
remembering where to actually install them. Basic usage looks like

  { stdenv, installShellFiles, ... }:
  stdenv.mkDerivation {
    # ...
    nativeBuildInputs = [ installShellFiles ];
    postInstall = ''
      installManPage doc/foobar.1
      installShellCompletion --bash share/completions/foobar.bash
      installShellCompletion --fish share/completions/foobar.fish
      installShellCompletion --zsh share/completions/_foobar
    '';
    # ...
  }

See source comments for more details on the functions.
@lilyball lilyball force-pushed the lilyball:installShellFiles branch from 57ee838 to b9369d1 Aug 3, 2019
@lilyball

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

I went ahead and squashed the commits together and force-pushed. No file changes, just git history.

@nixos-discourse

This comment has been minimized.

Copy link

commented Aug 8, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/38

@nixos-discourse

This comment has been minimized.

Copy link

commented Aug 17, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/45

@teto

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

the previous structured versions allowed to load completion while in nix-shell (which was great). The new one will work only after postInstall :(

@lilyball

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

@teto What's your actual use-case here? You're running buildPhase and then, what, manually instructing your shell to load the completions so you can use them when invoking the tool from the build folder? Is there a reason not to just run installPhase too at this point?

@teto

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

yes that's my usecase.

Running installPhase tends to fail as it won't be able to write to $out unless you change it (and let's not mention multioutput scenarios). Running installPhase can take some time too. And the installPhase is written so that it works after running a buildPhase but when in a nix-shell, most of the time you don't necessarily run buildPhase. For instance python enters develop mode.

@lilyball

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

@teto Is your goal to use this generically with arbitrary packages? Because if you're working on a specific package, surely you can just source the completion files from wherever you know they are.

Also if you're not running buildPhase then the structured approach wouldn't necessarily work for you anyway, because some packages may generate their completion files while building. Though admittedly that's pretty rare.

In any case, I liked the fact that the structured approach let you find the shell completions outside of the install process, but I have to admit the approach this PR currently takes is probably simpler to get people to use, more of a drop-in replacement for existing solutions, and also supports something the structured approach didn't, which is shell globbing.

@lilyball

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

If you have any ideas on how to regain the ability to source shell completions from within nix-shell, I'm all ears. Maybe we could do something like encourage people to define a phase like

installShellFilesPhase = ''
installManPage doc/foobar.1
'';

and we could configure this automatically to run after installPhase, and then you could do something special with nix-shell that redefines those shell functions such that you can just invoke installShellFilesPhase and it would source the completions.

One downside to this approach is anyone who doesn't use the separate installShellFilesPhase wouldn't be compatible with the automatic sourcing for nix-shell.

@teto

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

Something along these lines.
What matters is to make available the zsh/bash paths to the shell script. __structuredAttrs looked nice but apparenlty it's not good enough. I am thinking of bash arrays/parsable string. Sthg like makeFlags that could be named zshCompletionFiles for instances.

@lilyball

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2019

The problem is anything complex, people aren't going to use. With the current approach, it works pretty much the way people normally expect, except instead of having to remember where exactly the files get installed, they just use this shell function to install it (but it's otherwise done at the same time, in installPhase).

I liked the structured approach because it preserved structured data in a way you could take advantage of e.g. with what you're proposing, but it was definitely more complicated and harder for people to understand exactly what it's doing.

I'm not really sure how to get the best of both worlds. Like I said, we might possibly convince people to do something like write their shell script installations in a separate var installShellFilesPhase, but I'm not all that optimistic about it when the "obvious" solution is to just stick the shell functions into installPhase.

@teto

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

I just had a look at __structuredAttrs, what a shame it breaks builders (but the future is bright).

What's hard about defining :
zshCompletionFiles = [ "_exa=contrib/completions.zsh" ];
(btw do we really need the _exa ? this could be deduced from pname ?)
Then the installPhase could natively install the completion (rather than a setupHook).

It would allow myself or another to add some code later on for the nix-shell case.

@lilyball

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

(btw do we really need the _exa ? this could be deduced from pname ?)

Yes because the pname isn't necessarily the same thing as the executable name, and the package could install multiple executables.

What's hard about defining :
zshCompletionFiles = [ "_exa=contrib/completions.zsh" ];
[…]
Then the installPhase could natively install the completion (rather than a setupHook).

How do you get people to write this

zshCompletionFiles = [ "_exa=contrib/completions.zsh" ];
installPhase = ''
  installShellFiles $zshCompletionFiles
'';

when they could just write

installPhase = ''
  installShellFiles _exa=contrib/completions.zsh
'';

Especially since this approach doesn't even handle edge cases like paths that have spaces in them (Nix arrays get exposed to bash just as space-separated strings, and if you escape the elements of the array, you then need to pass the whole thing to eval to get it to handle the escapes/quotes properly, so that's actually eval installShellFiles $zshCompletionFiles).

@teto

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

making installShellFiles a default hook would make sense. Then you don't need to modify installPhase. modifying the installPhase can be tricky too (must remember to properly trigger the pre/post hooks)

As for the space default, I believe it's so rare that we could leave it as an edgecase until __structuredAttrs becomes a thing.

@lilyball

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

As for the space default, I believe it's so rare that we could leave it as an edgecase until __structuredAttrs becomes a thing.

Given that Bash doesn't have any support for JSON I'm not expecting to be using __structuredAttrs with the default builder any time soon.

@lilyball

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2019

At this point, what's stopping this PR from being merged?

@nixos-discourse

This comment has been minimized.

Copy link

commented Sep 4, 2019

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/53

rycee added a commit that referenced this pull request Sep 4, 2019
This is a new package that provides a shell hook to make it easy to
declare manpages and shell completions in a manner that doesn't require
remembering where to actually install them. Basic usage looks like

  { stdenv, installShellFiles, ... }:
  stdenv.mkDerivation {
    # ...
    nativeBuildInputs = [ installShellFiles ];
    postInstall = ''
      installManPage doc/foobar.1
      installShellCompletion --bash share/completions/foobar.bash
      installShellCompletion --fish share/completions/foobar.fish
      installShellCompletion --zsh share/completions/_foobar
    '';
    # ...
  }

See source comments for more details on the functions.
@rycee

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Thanks for your patience 🙂

Rebased to master in 43dade2.

@rycee rycee closed this Sep 4, 2019
@lilyball lilyball deleted the lilyball:installShellFiles branch Sep 4, 2019
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Sep 18, 2019
This is a new package that provides a shell hook to make it easy to
declare manpages and shell completions in a manner that doesn't require
remembering where to actually install them. Basic usage looks like

  { stdenv, installShellFiles, ... }:
  stdenv.mkDerivation {
    # ...
    nativeBuildInputs = [ installShellFiles ];
    postInstall = ''
      installManPage doc/foobar.1
      installShellCompletion --bash share/completions/foobar.bash
      installShellCompletion --fish share/completions/foobar.fish
      installShellCompletion --zsh share/completions/_foobar
    '';
    # ...
  }

See source comments for more details on the functions.

(cherry picked from commit 43dade2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.