postmodern / chruby

Changes the current Ruby
MIT License
2.85k stars 190 forks source link

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

Closed FranklinYu closed 2 years ago

FranklinYu commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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

ms-ati commented 2 years ago

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

postmodern commented 2 years ago

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

postmodern commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

ixti commented 2 years ago

@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 commented 2 years ago

Thanks. Then it favors my proposal in https://github.com/postmodern/chruby/pull/432#issuecomment-1001000660.

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

postmodern commented 2 years ago

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

FranklinYu commented 2 years ago

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?

FranklinYu commented 2 years ago

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

ixti commented 2 years ago

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 commented 2 years ago

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

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

FranklinYu commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

FranklinYu commented 2 years ago

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.