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

Fixed bash completion command in setup docs #2696

Merged
merged 1 commit into from
Nov 18, 2023

Conversation

fetwar
Copy link
Contributor

@fetwar fetwar commented Nov 17, 2023

Updated bash completion command in setup.md that would not persist past end of shell.

Installing bash completions in the system wide location should be better suggesting by default user-level install in ~/.bashrc as gopass is installed system wide as well.

This should be more inline with best practices for allowing the user to manually set their bash completion commands per https://github.com/scop/bash-completion

Adding completions by default on install would be better in the long term though. Potentially as part of package install by default, as pass does (see pass makefile) or a builtin command to install completions post download.

Copy link
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

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

LGTM

@dominikschulz dominikschulz merged commit 55ea5cf into gopasspw:master Nov 18, 2023
8 checks passed

```bash
source <(gopass completion bash)
gopass completion bash | sudo tee $(pkg-config --variable=completionsdir bash-completion)
Copy link
Member

Choose a reason for hiding this comment

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

Is pkg-config really usually bundled with bash or with most distros nowadays?

@fetwar
Copy link
Contributor Author

fetwar commented Nov 20, 2023

I think @AnomalRoil may have a good point.

Availability of pkg-config

Looking into it pkg-config does seem to be quite universal and installed with bash-completion at least on debian, but admittedly my understanding of how widespread it is is lacking.

bash-completion is a separate package for me on apt, whilst it doesn't seem to always be installed with bash, it is a recommended package. So I believe that following the instructions here: github.com/scop/bash-completions should get us what we want, since this appears to be the official bash completions package.

Part of the bash package description reads:

 The Programmable Completion Code, by Ian Macdonald, is now found in
 the bash-completion package.

From my understanding, installing bash-completion installs the pkg-config variable I use in this PR.

How does gopass find distro default install locations etc normally?

Overall, I agree that finding a more robust way of installing completions is a good idea, but that it shouldn't be up to the user to do it.


Improving Further

There are more options for robust bash completion install I discovered while researching briefly for this PR, but I avoided including them for brevity of the docs. I think this would suit an automated install well though, where error handling can be much more in depth.

To summarise my understanding of bash completions following brief research:

System wide completions methods:

  • /etc/bash_completion.d - the old recommended install location for completions.
    • This initiates all completions on shell start for all users (eager loads), usually sourced within system wide profile script.
    • Legacy install location because of the eager loading, but makes a good fallback compared to just not having completions at all.
  • /usr/share/bash-completion/completions - the new install location for completions.
    • Initiates dynamically when required, allowing for faster shell startup times afaik
    • Not a fixed location, can change distro to distro, which is why I believe the info I followed from scoop/bash-completions mentions using pkg-config to find it's location. However, I don't trust pkg-config to always be installed.

User specific completion methods:

  • ~/.bashrc
    • Initiates on shell start (eager loads) and is user specific
    • User specific is a good method to install, however for system wide apps I think system wide makes more sense
  • ~/.bash_completions
    • Mentioned as good abstracted location compared to having completions directly within ~/.bashrc, but also eagerly loads.

There's a few others mentioned in the scop/bash-completions github, but I will be the first to admit this isn't my area of expertise.

I think that adding proper install of bash completions automatically when installing the software is the best method, where we can check that packages like pkg-config and bash-completion (to provide the variable I mention in the docs) are installed, using the locations provided if so, falling back to defined fallback paths if not.

Is there a make Makefile that is run for install that we can edit to include this? I can't see why bash completions shouldn't be installed by default as part of the package. Notably installing pass via make or apt (presumably most other methods, although personally untested) does install completions.

@fetwar
Copy link
Contributor Author

fetwar commented Nov 20, 2023

Found an issue in this PR, silly typo on my end, will amend and re-open or create another if needed

Instead of

gopass completion bash | sudo tee $(pkg-config --variable=completionsdir bash-completion)

It should be

gopass completion bash | sudo tee $(pkg-config --variable=completionsdir bash-completion)/gopass

dominikschulz pushed a commit that referenced this pull request Nov 20, 2023
* Updated bash autocompletion setup commands

Signed-off-by: Marc <[email protected]>

* Fix typo with bash completions command in setup.md

Signed-off-by: Marc <[email protected]>

---------

Signed-off-by: Marc <[email protected]>
dominikschulz added a commit that referenced this pull request Nov 20, 2023
* 'master' of github.com:gopasspw/gopass:
  Fix typo introduced in PR #2696 (#2707)
@dominikschulz
Copy link
Member

We can require or recommend the bash-completion package in our Deb and RPM packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants