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

Add basic tab completion support for Bash and Zsh (II) #432

Closed
wants to merge 5 commits into from

Conversation

FranklinYu
Copy link
Contributor

This is a continuation of @callahad's work. Homebrew formula and Fedora package should put those two files in correct location such that user don’t need to explicitly source them (especially Zsh).

Closes #297.

Fixes #27.

@FranklinYu FranklinYu changed the title Add basic tab completion support for Bash and Zsh Add basic tab completion support for Bash and Zsh (II) Feb 24, 2020
@FranklinYu
Copy link
Contributor Author

Any comment? I see that some PRs are merged so I assume that this PR was overlooked? This PR seems trivial enough to review.

@FranklinYu
Copy link
Contributor Author

This Pull Request should be straightforward to review. Please give it another shot.

@postmodern postmodern self-assigned this Feb 3, 2021
@ms-ati
Copy link

ms-ati commented Dec 3, 2021

Hello! Is this going to be reviewed? I came here searching for "chruby bash completion"

@postmodern
Copy link
Owner

Thanks for reminding me. I think we can rebase this and merge it into the 0.4.0 branch.

@postmodern postmodern added this to the 0.4.0 milestone Dec 3, 2021
@postmodern
Copy link
Owner

I manually rebased the commit against the 0.4.0 branch and merged it locally. I am just curious why we can't install the completion files in the Makefiles install task? I assume homebrew requires some special method for installing completion files vs the standard *nix /usr/local locations? /cc @FranklinYu

@FranklinYu
Copy link
Contributor Author

FranklinYu commented Dec 13, 2021

I am just curious why we can't install the completion files in the Makefiles install task?

I think it makes sense to include them in standard make install. My only concern is possible difference between platforms including macOS (Homebrew and MacPorts), Debian-families, Red Hat families, and Arch Linux families. For example, I think Homebrew installs Bash completions under $(DESTDIR)$(PREFIX)/etc/bash_completion.d, and they use /usr/local as the $(PREFIX) (depending on user installation). In contrast, many Linux simply install the completions under $(DESTDIR)/etc/bash_completion.d, i.e. ignoring the $(PREFIX), even if the prefix is typically non-empty (like /usr).

I haven’t made up my mind though. It does sound better to the community if each distribution doesn’t have to write it separately. Any suggestions? Would it be useful, for example, to have a Make target called install-completions? This way Homebrew (and MacPorts?) installs the completion in their package definition, while maintainers for most Linux distributions can have a shortcut.

@FranklinYu
Copy link
Contributor Author

By the way, I should have used chruby instead of _chruby for Bash completion in Homebrew formula. The underscore seems a convention only for Zsh (because it loads the completion lazily). Bash loads everything anyway, regardless of the name:

https://github.com/scop/bash-completion/blob/c32358bda69909cdf4957489a5b8944af946575d/bash_completion#L2419-L2422

so I think having a _ prefix seems unnecessary and non-idiomatic. It sounds like you have some local merge; please let me know if I can just append another commit to the branch (I assumed that appending a new one is better than amending the existing one, because I see other PRs being squashed).

@postmodern
Copy link
Owner

postmodern commented Dec 17, 2021

@FranklinYu I think we could auto-detect the bash completion directory in the Makefile. It could be as simple as a for loop that checks for various directories and picks the first match. A better detection strategy would be to check for the completion dir in /usr/share/ and then install into $(PREFIX)/share/$dir.

The traditional way to handle OS specific directories would be to create a configure script which builds a Makefile.in that your Makefile includes, or even generate a full Makefile. I'd like to avoid adding ./configure script for as long as possible and try to do everything in the Makefile.

I checked Fedora's bash completion setup and they do have a /etc/bash_completion.d/ directory, but the majority of the completion files live in /usr/share/bash-completion/completions/ which are loaded by /usr/share/bash-completion/bash_completion. The main bash_completion file also appears to load completions from $HOME/.local/share/bash-completion/completions, /usr/local/share/bash-completion/completions, and /usr/share/bash-completion/completions Ubuntu appears to be the same. It also appears that homebrew has some complex configuration to load completions from various different etc directories. I would be OK with installing into $(PREFIX)/etc/bash_completion.d/, as that appears to the commonly supported directory between Linux distros and Homebrew.

I rebased and merged your branch into 0.4.0 branch, but haven;t pushed it yet. You could push another commit(s) to your branch and I could cherry-pick those in as well.

@FranklinYu
Copy link
Contributor Author

Sorry, you are totally correct. I was under the impression that they put completions under /etc/bash_completions.d/, but in Debian the file /etc/bash_completion.d/bash_completion is actually provided by the bash-completion package, which is only one line loading /usr/share/bash-completion/bash_completion (the real file) as you said. I guess this file was kept as a shim for compatibility. The real file is from upstream, which indeed checks the directories you mentioned:

https://github.com/scop/bash-completion/blob/b0afc79c12f3d9fe7d5cc2e58a373eba0edc0d09/bash_completion#L2350

Fedora and Arch Linux don’t provide the shim, so user would directly source the real file. I think the above covers Debian family (including Ubuntu) and Fedora family, so it should be good enough.

From my understanding of Linux filesystem structure, I actually prefer the “better detection strategy” you mentioned, because the file belongs to part of the package, while files in /etc/ is expected to be modified by site admins. That said, I’m still considering how to be compatible with MacPorts, which puts Bash completions under $(PREFIX)/share/bash-completion/completions. (So we really only want to exclude weird Homebrew.)

A follow-up questions: now that we detect whether the directory exist, there are many ways to do conditionals:

  1. Variable expansion with wildcard and if functions.
  2. Use if statement with wildcard function.
  3. Use the if statement from shell.
  4. Use the for loop from shell

Option 1 allows user to override the $BASH_COMP_DIR from command line; option 2 is more readable (no trailing \, no Makefile function call); with option 4 we don’t install anything if neither directory exist. Which one would you prefer?

@postmodern
Copy link
Owner

@FranklinYu I would imagine that a simple if [ -d "$(PREFIX)/etc/bash_completion.d" ] ... check would be enough to determine if we're installing into Homebrew's root, otherwise assume we're installing into a /usr/local like environment.

@ixti
Copy link

ixti commented Dec 24, 2021

If it's possible to have separate auto-completions installation task – that will be wonderful for Gentoo ebuilds (zsh-autocompletions and bash-autocompletions are global USE flags, so completions would be installed for users asking for those).

If not, then I will have to copy relevant installation instructions from Makefile to ebuild :D

@FranklinYu FranklinYu marked this pull request as draft December 25, 2021 00:47
@FranklinYu
Copy link
Contributor Author

@ixti I’m happy to learn how Gentoo handles completions. Does Gentoo prefer a single Make target like install-shell-completions, or different targets install-bash-completion and install-zsh-completion?

@FranklinYu
Copy link
Contributor Author

FranklinYu commented Dec 25, 2021

@postmodern Thinking about it again, I’m concerned that $(DESTDIR)$(PREFIX)/etc/bash_completion.d might not be present when building Homebrew formulas. I think Homebrew uses $DESTDIR like other package managers. I also noticed that the only outlier seems to be Homebrew, so I have an alternative proposal:

Create 3 different installation target: install, install-bash-completion, and install-zsh-completion. On all other platforms, we run make install install-bash-completion install-zsh-completion; in Homebrew formula we run make install install-zsh-completion then manually install the Bash completion with Homebrew mechanism. No more if statement in

This at least covers Debian family, Red Hat family, Arch Linux family, Gentoo family (according to the discussion above), and MacPorts. I also verified that it works with Alpine Linux (which is consistent with other Linux distributions). BTW I’m pushing the commit for Zsh since it’s less messy than Bash.

I don’t have a chance to verify it on the SUSE family or the Slackware family.

Makefile Outdated Show resolved Hide resolved
@ixti
Copy link

ixti commented Dec 25, 2021

@FranklinYu

Here's current ebuild I maintain: https://github.com/gentoo/guru/blob/master/dev-ruby/chruby/chruby-0.3.9-r3.ebuild

If/when chruby will have *-autocompletion, I will change emake line to something like:

src_install() {
  local emakeargs=(
    DESTDIR="$D"
    PREFIX="${D}/usr"
  )

  emake "${emakeargs[@]}" install
  use zsh-completion && emake "${emakeargs[@]}" install-zsh-completion
  use bash-completion && emake "${emakeargs[@]}" install-bash-completion

  insinto "/etc/profile.d"
  newins "${FILESDIR}/systemwide.sh" "chruby.sh"
}

@FranklinYu
Copy link
Contributor Author

Thanks. Then it favors my proposal in #432 (comment).

By the way, I did some research and found this:

install -Dvm0644 completions/bash/yt-dlp "$b/%_datadir/bash-completion/completions/yt-dlp"

from the youtube-dl openSUSE package. This seems an official package maintained by the openSUSE team, so openSUSE probably joins the “normal Linux” team. I think it’s mainly up to @postmodern to make the call.

@ixti
Copy link

ixti commented Dec 26, 2021

In fact, for Gentoo ebuild most likely I will go a bit different direction (won't need make task):

use bash-completion && newbashcomp "path/to/bash-completions" "${PN}"
use zsh-completion && {
  insinto /usr/share/zsh/site-functions
  newins "path/to/zsh-completion" "_${PN}"
} 

@ixti
Copy link

ixti commented Dec 26, 2021

Just figured out Gentoo policy on small files: https://projects.gentoo.org/qa/policy-guide/installed-files.html#pg0301

Thus, in the ebuild I will install those unconditionally:

newbashcomp "path/to/bash-completions" "${PN}"

insinto /usr/share/zsh/site-functions
newins "path/to/zsh-completion" "_${PN}"

@postmodern
Copy link
Owner

@FranklinYu

@postmodern Thinking about it again, I’m concerned that $(DESTDIR)$(PREFIX)/etc/bash_completion.d might not be present when building Homebrew formulas. I think Homebrew uses $DESTDIR like other package managers. I also noticed that the only outlier seems to be Homebrew, so I have an alternative proposal:

Create 3 different installation target: install, install-bash-completion, and install-zsh-completion. On all other platforms, we run make install install-bash-completion install-zsh-completion; in Homebrew formula we run make install install-zsh-completion then manually install the Bash completion with Homebrew mechanism. No more if statement in

This seems reasonable. Zsh users would probably not want Bash completion rules to be installed, and Bash users would probably not care about Zsh completion rules. Having separate installation tasks and allowing the user (or package build script) to pick which additional installation tasks to run seems reasonable.

Copy link
Owner

@postmodern postmodern left a comment

Choose a reason for hiding this comment

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

For consistency there should also be a install-bash-completion target.

Additionally, you could skip installing zsh or bash completion rules if zsh/bash cannot be found (ex: command -v zsh && ...).

rpm/chruby.spec Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@postmodern
Copy link
Owner

We could also move the completion files into a separate complete directory, so they wont be installed by the main install task.

@FranklinYu
Copy link
Contributor Author

We could also move the completion files into a separate complete directory, so they wont be installed by the main install task.

I like the idea, but I’m thinking about a directory name other than complete because complete/completion.bash seems a bit repetitive. Since chruby would only ever support Zsh/Bash, we won’t have more completion files; how about a more generic name? This way we can put more stuff (that we don’t put in share) in this folder. Three options in my mind:

  1. resources
  2. support: files supporting the chruby functionality
  3. integration: shell integration, terminal integration, etc.

What do you think?

After this change, the `install` target will skip them, since they
should be installed separately.
@FranklinYu
Copy link
Contributor Author

I went with the third option. Please let me know if you prefer otherwise.

@FranklinYu FranklinYu marked this pull request as ready for review January 18, 2022 03:07
@ixti
Copy link

ixti commented Jan 18, 2022

I don't have a strong preference, but WDYT about:

.
└── completions
    ├── bash
    │   └── chruby
    └── zsh
        └── _chruby

That would make Makefile look a bit nicer too:

install-bash-completion:
	mkdir -p $(bash_comp_dir)
	cp completions/bash/chruby $(bash_comp_dir)

install-zsh-completion:
	mkdir -p $(zsh_comp_dir)
	cp completions/zsh/_chruby $(zsh_comp_dir)

And in homebrew formulae:

zsh_completion.install 'completions/zsh/_chruby'
bash_completion.install 'completions/bash/chruby'

@postmodern
Copy link
Owner

I was randomly searching around on GitHub trying to figure out if there was a defacto standard. I found the following directories:

  • completions/
  • contrib/completion/
  • etc/

Since there's no clear winner, I will accept integrations just to get this closed so we can move on.

@FranklinYu
Copy link
Contributor Author

For Zsh completion, I found that some distributions (notably Debian) actually prefers

/usr/share/zsh/vendor-completions

Do we want to follow this instead?

@ixti
Copy link

ixti commented Jan 18, 2022

For Zsh completion, I found that some distributions (notably Debian) actually prefers

IMO we better don't. :D
I don't think it's a standard path for Zsh. I can't see it anywhere in manuals.

postmodern added a commit that referenced this pull request Jan 19, 2022
…nklinYu).

* Added install-bash-completion and install-zsh-completion tasks.
* Updated homebrew formula to also install bash completions.
@postmodern
Copy link
Owner

postmodern commented Jan 19, 2022

Manually squash merged into the 0.4.0 branch (see e175d19). Thank you for your patients patience to help get this finally closed. Any additional fixes can be submitted as separate PRs.

@postmodern postmodern closed this Jan 19, 2022
@FranklinYu
Copy link
Contributor Author

FranklinYu commented Jan 19, 2022

For posterity: indeed the default directory is $(PREFIX)/share/zsh/site-functions/:

https://github.com/zsh-users/zsh/blob/master/configure.ac#L308-L315

Homebrew, MacPorts, Fedora, Arch Linux, and Alpine Linux preserve this default behavior. Debian, in contrast, sets it to /usr/local/share/zsh/site-functions/, then adds vendor-functions/ and vendor-completions/ as “additional fpath”:

https://salsa.debian.org/debian/zsh/-/blob/dc50ace5801fc79114993a0d9fb26fc674aa33e4/debian/rules#L35

The change was introduced in 2011.

@FranklinYu FranklinYu deleted the completion branch January 29, 2022 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add bash completion
4 participants