I'd love to write a test suite for installShellCompletion but I'm not aware of any existing means to do such a thing.

#82964 is an example of a PR that could make use of this functionality. That PR has

    $PSC_PACKAGE --bash-completion-script $PSC_PACKAGE > psc-package.bash
     $PSC_PACKAGE --fish-completion-script $PSC_PACKAGE > psc-package.fish
     $PSC_PACKAGE --zsh-completion-script $PSC_PACKAGE > _psc-package
     installShellCompletion \
       psc-package.{bash,fish} \
       --zsh _psc-package

With this enhancement it could be rewritten as

    installShellCompletion --cmd psc-package \
      --bash --exec "$PSC_PACKAGE --bash-completion-script $PSC_PACKAGE" \
      --fish --exec "$PSC_PACKAGE --fish-completion-script $PSC_PACKAGE" \
      --zsh --exec "$PSC_PACKAGE --zsh-completion-script $PSC_PACKAGE"

I haven't tried it but wouldn't something like

installShellCompletion --bash --name foobar.bash <($out/bin/foobar --bash-completion)

work?

I forgot that was a feature of bash. However, trying it right now on macOS gets me

install: skipping file '/dev/fd/63', as it was replaced while being copied

So at the very least installShellCompletion would have to be changed to use something other than install. Curiously, install -T <(echo hi) x works just fine on NixOS, it's just macOS where it's failing.

Ah, didn't know that macos would have problems with that. When I wrote the comment I tried using install on NixOS using the <(…) and since that worked I figured the installShellCompletion would work.

Perhaps <(…) would work also on macOS if the install command was replaced by

mkdir -p "$outDir" \
  && cat "$arg" > "$outPath"
  && chmod 644 "$outPath"

?

In any case, I'm not against the - implementation but figured this solution might be worth considering since it's quite simple. If it can't easily be made to work on macOS then it's a non-starter, though.

I've updated this PR to remove the --exec flag in favor of <(cmd). It now detects named pipes and used cat to read them, still using install for regular paths. I didn't bother adding the chmod 644 since -rw-r--r-- seems to be the default permissions anyway, it's just there on the install because it's easy to be specific there.

I just added a second commit that updates psc-package to use this new syntax.

I’d love to have this new functionality, but I’m really uncomfortable with the amount of complexity it adds to the bash code (and all that command line parsing logic and interacting command line arguments, halp).

I’d feel a lot better if there was a test suite and if possible a list of packages that use this feature and whose output could be checked to contain the completions in the right location.

Maybe we can reduce the scope and just add the simplest subset of this PR first, instead of doing it all in one go?

(Also, this is the first time I’ve seen you here @lilyball, welcome. Awesome avatar :) )

@Profpatsch I'd really love a test suite too, but I couldn't find any precedent for writing test suites on package setup hooks so I'm really not sure how to go about doing it. I did manually verify that ffsend still installs its completions correctly (which exercises the pre-existing functionality) and the second commit updates psc-package to use most of the new functionality.

Maybe we can reduce the scope and just add the simplest subset of this PR first, instead of doing it all in one go?

I could remove stdin support, but that doesn't really save much. A more compelling argument for removing it is "why have 2 ways to do the same thing?", but I'm conflicted here because I feel like using stdin is perhaps more obvious to people than using <(cmd). Still, perhaps the conceptual simplification is worth it.

FWIW I wrote this package setup hook in the first place (see #65211).

I went ahead and stripped out the stdin support. Not a huge difference to the patch, but I did it for the conceptual simplification.

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/139

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/152

@lilyball this is looking good to me, though I am no too familiar with this stuff. There are tests in pkgs/tests for other pkgs/build-support items, so it could go there.

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/153

pkgs/test/patch-shebangs/default.nix looks like a good template. I'll try to find the time soon to write some tests like that.

I just added a suite of tests at tests.install-shell-files.

I didn't add the tests anywhere to be invoked automatically though.

I wasn't sure if I should put it in the release file or not. Are you recommending that I do? Or should I just put it in installShellFiles.passthru.tests?

Given that darwin.cctools now depends on this, which means all of stdenv does, do I need to point this at staging? FWIW darwin.cctools just uses installManPage which this PR doesn't touch.

I wasn't sure if I should put it in the release file or not. Are you recommending that I do?

I guess it makes sense? I don’t have a good overview on when these are run, probably before advancing channels.

Or should I just put it in installShellFiles.passthru.tests?

Ah, I wasn’t aware installShellFiles (or more generally anything created by makeSetupHook was an actual derivation.

In that case, let’s go for the passthru.tests

@lilyball Could you please resolve the conflict?
Do you have an idea, when this might be merged?

@Luflosi Sorry, I haven't had time to work on this in ages. I wanted to put the tests into passthru.tests so they'd actually be run. I'll try to find the time to come back to this soon.

I would have had an interest in using this better sooner than later. Most Go tools behave according to what this PR tries to solve.

I can't promise it but I will do my best to revisit this today.

I rebased onto staging, fixed the merge conflict, and added passthru.tests. For the tests I had to use overrideAttrs as makeSetupHook has no direct support for this; I can't test locally because of the mass rebuild but I'm assuming it will work.

It has tests, it adds some features, doesn’t seem to break the existing stuff, LGTM!

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

https://discourse.nixos.org/t/how-many-people-are-paid-to-work-on-nix-nixpkgs/8307/79