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: Enhance installShellCompletion #83630

Merged
merged 4 commits into from Oct 9, 2020

Conversation

@lilyball
Copy link
Member

@lilyball lilyball commented Mar 28, 2020

Motivation for this change

Some packages don't bundle shell completions as files in their source tree but instead synthesize them by invoking the built tool with specific flags. This enhances the installShellCompletion command such that it can install these completions without having to pipe them to a file first. To do this, it now supports a named pipe as a path, such as produced by <(cmd). It also now has a convenience flag to synthesize the name for all files based on the shell.

Examples:

installShellCompletion --cmd foobar \
  --bash <($out/bin/foobar --bash-completion) \
  --fish <($out/bin/foobar --fish-completion) \
  --zsh <($out/bin/foobar --zsh-completion)

Fixes #83284

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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 one package that uses installShellCompletion (ffsend) and it produced the same output. I also manually tested all of the new functionality, including error cases.

@lilyball lilyball requested a review from Ericson2314 as a code owner Mar 28, 2020
@lilyball
Copy link
Member Author

@lilyball lilyball commented Mar 28, 2020

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

@lilyball
Copy link
Member Author

@lilyball lilyball commented Mar 28, 2020

#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"

@rycee
Copy link
Member

@rycee rycee commented Mar 29, 2020

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

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

work?

@lilyball
Copy link
Member Author

@lilyball lilyball commented Mar 29, 2020

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.

@rycee
Copy link
Member

@rycee rycee commented Mar 29, 2020

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.

@lilyball lilyball force-pushed the install-shell-files branch from 648d6d1 to 13bab4b Mar 29, 2020
@lilyball
Copy link
Member Author

@lilyball lilyball commented Mar 29, 2020

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.

@lilyball lilyball force-pushed the install-shell-files branch from 13bab4b to 2c18eab Apr 2, 2020
@lilyball
Copy link
Member Author

@lilyball lilyball commented Apr 2, 2020

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

@ofborg ofborg bot requested a review from Profpatsch Apr 2, 2020
@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Apr 2, 2020

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 :) )

@lilyball
Copy link
Member Author

@lilyball lilyball commented Apr 2, 2020

@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).

@lilyball lilyball force-pushed the install-shell-files branch from 2c18eab to 40e20f7 Apr 3, 2020
@lilyball
Copy link
Member Author

@lilyball lilyball commented Apr 3, 2020

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

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Apr 17, 2020

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

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Apr 30, 2020

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

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Apr 30, 2020

@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.

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented May 1, 2020

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

@lilyball
Copy link
Member Author

@lilyball lilyball commented May 2, 2020

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.

@lilyball
Copy link
Member Author

@lilyball lilyball commented May 8, 2020

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

@lilyball
Copy link
Member Author

@lilyball lilyball commented May 8, 2020

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

@lilyball
Copy link
Member Author

@lilyball lilyball commented May 15, 2020

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?

@lilyball
Copy link
Member Author

@lilyball lilyball commented May 20, 2020

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.

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented May 24, 2020

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented May 24, 2020

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

@Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Aug 31, 2020

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

@lilyball
Copy link
Member Author

@lilyball lilyball commented Aug 31, 2020

@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.

@blaggacao
Copy link
Contributor

@blaggacao blaggacao commented Oct 8, 2020

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.

@lilyball
Copy link
Member Author

@lilyball lilyball commented Oct 8, 2020

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

Lily Ballard and others added 4 commits Oct 8, 2020
Teach installShellCompletion how to install completions from a named
pipe. Also add a convenience flag `--cmd NAME` that synthesizes the name
for each completion instead of requiring repeated `--name` flags.

Usage looks something like

    installShellCompletion --cmd foobar \
      --bash <($out/bin/foobar --bash-completion) \
      --fish <($out/bin/foobar --fish-completion) \
      --zsh <($out/bin/foobar --zsh-completion)

Fixes NixOS#83284
@lilyball lilyball changed the base branch from master to staging Oct 8, 2020
@lilyball lilyball force-pushed the install-shell-files branch from 5e7d73c to 03c9f6a Oct 8, 2020
@lilyball
Copy link
Member Author

@lilyball lilyball commented Oct 8, 2020

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.

@ofborg ofborg bot requested a review from Profpatsch Oct 8, 2020
@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Oct 9, 2020

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

@Profpatsch Profpatsch merged commit 9639d56 into NixOS:staging Oct 9, 2020
19 of 20 checks passed
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 9, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment