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

Haskell bash completion #42657

Closed
wants to merge 9 commits into from
Closed

Conversation

roberth
Copy link
Member

@roberth roberth commented Jun 27, 2018

Motivation for this change

The optparse-applicative library provides a uniform method of adding completions to haskell programs. This introduces two helper functions in haskell.lib that capture that.

This PR also adds completion to a small number of Haskell commands that are built with this library.

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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff!

What do you think of making this a standard procedure for all packages? So, adding optparse-applicative to the dependencies should automatically set up a postInstall hook that tries to run --bash-completion-script on all binaries, install the result on success, or fail silently (in case it's not supported).

Pinging some Haskell people @peti @shlevy @domenkozar @Profpatsch

@roberth
Copy link
Member Author

roberth commented Jul 8, 2018

@infinisil, others, I have thought about trying all executables but was a bit wary of the side effects. A package might include both a command line tool and an executable that fails in the sandbox, or takes a very long time to complete.

I have already spent too much time debugging silent failures. These kinds of failures are probably rare, so I'd rather require an override to selectively disable this feature, rather than give a one-line cue in an already verbose build log.

To make an opt-out design work without too nasty surprise, it should involve at least

  • Conditioning on isExecutable and, depending on what works best, one of
    • Detecting optparse-applicative in the transitive closure of dependencies
    • Checking direct dependencies against a list of at least optparse-applicative and optparse-generic
  • Running the executables with --bash-completion-script and logging that we're doing so
  • Set up a five minute timeout to catch interactive commands without completion script, fail hard on timeout with explanatory message
  • Fail hard with explanatory error message on failure, including methods to resolve the issue
    • Disable completion script extraction entirely
    • Blacklist an executable
  • Log that we're done with completion script extraction

An intermediate possibility is to require opt-in but not require to list the command line executables. That should give an opportunity to test the logic above that may become the opt-out solution.

@infinisil
Copy link
Member

Set up a five minute timeout to catch interactive commands without completion script, fail hard on timeout with explanatory message

This is just for getting the output of --bash-completion-script though, wouldn't it make more sense to fail after like 1 second? Any program that takes longer than that to finish with such an argument probably doesn't support it. Also why fail hard? Wouldn't this make lots of packages fail because of that? I'd rather have all packages at least try to get bash completion, but ignore it if it's not possible, since bash completion is really secondary. And such a failure really doesn't have a lot of other remedies than just blacklisting that package, which will result in the same as just ignoring such failures.

@roberth
Copy link
Member Author

roberth commented Jul 8, 2018

@infinisil I agree that completion is secondary, but my concern is about bitrot. If we're going to fail silently we should at least test some high profile haskell packages. Five minute timeout is for a builder with high load to recover. It's only 2.5 orders of magnitude more ;)

Do we have a place for (small) integration tests in nixpkgs nowadays? I think I can live with soft failure in this case if we have warnings in the log and some test cases to prevent bitrot.

@shlevy
Copy link
Member

shlevy commented Jul 8, 2018

I'd love to see some thought put into a more general notion of how packages interact with installation environments, but for now some kind of post install hook seems great to me

@infinisil
Copy link
Member

@roberth By bitrot you mean the completion scripts potentially failing for more packages over time without anybody noticing? Does that really happen? I can't think of anything that could go wrong with such a simple post build hook.

@roberth
Copy link
Member Author

roberth commented Jul 8, 2018

@infinisil That's what I mean. I don't expect anything in particular to go wrong either. I just want to set things up so that when they do go wrong, they get noticed.

Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the PR, thank you very much! I have some bike-shedding suggestions with regard to the name of that combinator, but I'm hugely in favor of having this function available in Nixpkgs.

* command: name of an executable
* pkg: Haskell package that build the executable
*/
appendCompletion = command: pkg:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that generateCompletion is a more descriptive name than appendCompletion. Also, there might be more than one way to generate those files depending on which option parsing library is used, therefore it might be a good idea to put the optparse-applicative in there somehow, i.e. generateOptparseApplicativeCompletion.

appendCompletion = command: pkg:
overrideCabal pkg (old: {
postInstall = old.postInstall or "" + ''
echo "installing bash completion script for ${command}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we'd also want completion files for zsh and fish? optparse-applicative can generate those as well.

@peti
Copy link
Member

peti commented Jul 9, 2018

It's interesting to note that the generated completion files are totally generic, i.e. they contain no code that's specific to the executable in questions other than its name and full path:

$ cabal2nix --bash-completion-script=/full/path/to/cabal2nix
_cabal2nix()
{
    local CMDLINE
    local IFS=$'\n'
    CMDLINE=(--bash-completion-index $COMP_CWORD)

    for arg in ${COMP_WORDS[@]}; do
        CMDLINE=(${CMDLINE[@]} --bash-completion-word $arg)
    done

    COMPREPLY=( $(/full/path/to/cabal2nix "${CMDLINE[@]}") )
}

complete -o filenames -F _cabal2nix cabal2nix

So arguably we could generate those files without actually running the executable, which is a trait the cross-compilation people would surely appreciate. The downside is, of course, that we expose ourselves to potential incompatibilities that might occur across different versions of optparse-applicative.

@@ -658,7 +658,7 @@ self: super: {
}));

# Need newer versions of their dependencies than the ones we have in LTS-11.x.
cabal2nix = super.cabal2nix.overrideScope (self: super: { hpack = addTestToolDepend (self.hpack_0_28_2) self.hspec-discover; hackage-db = self.hackage-db_2_0_1; });
cabal2nix = appendCompletions ["cabal2nix"] (super.cabal2nix.overrideScope (self: super: { hpack = addTestToolDepend (self.hpack_0_28_2) self.hspec-discover; hackage-db = self.hackage-db_2_0_1; }));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that cabal2nix (and some other tools) have similar overrides already in pkgs/top-level/all-packages.nix and elsewhere, so we have to make sure those are removed as part of this PR:

$ grep -R -- --bash-completion-script pkgs
pkgs/development/haskell-modules/hackage-packages.nix:         $exe --bash-completion-script $exe >$out/share/bash-completion/completions/stack
pkgs/top-level/all-packages.nix:      $exe --bash-completion-script $exe >$out/share/bash-completion/completions/${drv.pname}

@roberth
Copy link
Member Author

roberth commented Jul 9, 2018

@peti, interesting observation for cross compilation. I should add though that running $executable --bash-completion-script also serves as a check whether it is really an optparse program. For example in the dhall package, dhall-repl does not support completions. If you'd generate a generic completion script for each executable, bash would start the dhall repl as soon as you hit tab.

This is not a concern for the current state of the PR, but it is for the opt-out approach. I think opt-out is feasible and I want to give it a try soon.

@peti
Copy link
Member

peti commented Jul 9, 2018 via email

@infinisil
Copy link
Member

@peti It's all sandboxed anyways though, what bad could happen? If we can't find some serious problem/risk with the opt-out approach, this would be a huge usability improvement, especially since so many packages are using optparse-applicative.

@Profpatsch
Copy link
Member

I understand peti’s concerns, it will make debugging harder. It also has a taste of an inverse autotools, running checks that might generate output, or maybe not.

@roberth
Copy link
Member Author

roberth commented Jul 9, 2018

Although I think a well-implemented opt-out approach is valuable, it will take more time than I can realistically expect to spend on it short/mid-term. I will limit the scope of this PR to opt-in.

The optparse-applicative library provides a uniform method of adding
completions to haskell programs. These functions capture that.

Other shells than bash are supported by the library, so if anyone is
interested, they could add it to the appendCompletion function.
@roberth
Copy link
Member Author

roberth commented Jul 10, 2018

to do

  • function rename
  • install for zsh
  • successful test with zsh
  • install for fish
  • successful test with fish
  • make cross-compilation not fail

fish

For fish I could use some help. It doesn't pick up the completion from ~/.nix-profile. I've tried to configure fish but didn't get it to work.

cross compilation

The fish and zsh completions are 'adapters' that use optparse's --bash-* flags. They may be hacky and therefore subject to change.

@infinisil
Copy link
Member

Ping?

@roberth
Copy link
Member Author

roberth commented Aug 13, 2018

  1. We need a fish user to test this.
  2. Cross compilation can be 'solved' by disabling it during cross compilation.

Don't wait for me if you want to have this in 18.09 :(

@roberth
Copy link
Member Author

roberth commented Nov 3, 2018

Am I too diligent? #49477

@roberth
Copy link
Member Author

roberth commented Nov 4, 2018

Closing in favor of pr #49714 and cross compilation issue #49748

@roberth roberth closed this Nov 4, 2018
Ericson2314 pushed a commit to roberth/nixpkgs that referenced this pull request Nov 8, 2018
This adds the remaining parts of NixOS#42657 on top of NixOS#49477, renames the
function to a better name, and improves the docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants