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

Move bash completion file to rignt place and reformat to use tabs #413

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kloczek
Copy link

@kloczek kloczek commented Aug 5, 2020

  • Move bash completion file to $(datadir)/bash-completion/completions
  • Reformat to use only tabs (to make taht file a bit smaller)
  • sort OPTIONS entries
  • Rename bash completion file to just hwloc

@bgoglin
Copy link
Contributor

bgoglin commented Aug 5, 2020

Hello
We'll need some rationale for all this:

  • renaming: isn't it better to keep the bash suffix for non-bash users that may want to import it? (zsh has a way to import bash completion)
  • reformat with tabs: we actually try to avoid tabs entirely in the repository now.
  • sort: if I remember correctly, we tried to keep options in a logical order
  • datadir: isn't /etc/bash-completion.d the actual correct location? if you use datadir, users won't be able to install there directly anymore

@kloczek
Copy link
Author

kloczek commented Aug 5, 2020

Hello
We'll need some rationale for all this:

  • renaming: isn't it better to keep the bash suffix for non-bash users that may want to import it? (zsh has a way to import bash completion)

I'm not using zsh. If that syntax is acceptable and it imports bash files it should be no problem.

  • reformat with tabs: we actually try to avoid tabs entirely in the repository now.

That is understandable for source code however this file is kind of "executable" form and it should be as short as possible. Using tabs allows reduce size of that file. When file is installed no one should be looking on that file as long as it will have no any bugs. Installed form should be as small as possible.

  • sort: if I remember correctly, we tried to keep options in a logical order

Sorting alphabetically is logical.
Order of the OPTIONS has no impact on speed. Sorting alphabetically should allow avoid in the future add accidentally duplicates.

  • datadir: isn't /etc/bash-completion.d the actual correct location? if you use datadir, users won't be able to install there directly anymore
$ cat /usr/share/pkgconfig/bash-completion.pc
prefix=/usr
datadir=/usr/share
sysconfdir=/etc

compatdir=${sysconfdir}/bash_completion.d
completionsdir=${datadir}/bash-completion/completions
helpersdir=${datadir}/bash-completion/helpers

Name: bash-completion
Description: programmable completion for the bash shell
URL: https://github.com/scop/bash-completion
Version: 2.11

As you see you been using compatdir is for local completion. All packages should be installing own resources in completionsdir which is ${datadir}/bash-completion/completions. The best way should be instead hardcoding ${datadir}/bash-completion/completions just use pkgconfig to read completionsdir variable and install in path provided by it hwloc file.

@bgoglin
Copy link
Contributor

bgoglin commented Aug 5, 2020

  • datadir: ok, thanks.
  • zsh/bash: there are two different issues here. I think you're talking about the installed file (no need for suffix if going to a bash specific directory) while I was talking about the file in the source code (having the suffix in the source code is more clear, and better if some people provide a zsh-specific file). I am fine with renaming to hwloc in the bash install directory but I'd like to keep hwloc.bash in the source.
  • ordering: what I meant by logical is keeping related options together, like we do in usage (ideally the manpage would use the same ordering/grouping but it doesn't really yet). bash reorders alphabetically anyway.
  • tabs: I am not convinced it will change anything (the file still spans 3 pages, and I hope bash is able to parse spaces quickly) but ok. However you have 2 lines to fix because spaces are mixed with tabs:
312: 	local OPTIONS=(--ci --ri --cu --cd -h --help)
475:			  	COMPREPLY=( "<index of cpu to operate on>" "" )

By the way, we'll need a Signed-off-by line in each commit (that's why the PR is currently marked as not successful). See the last section of https://github.com/open-mpi/ompi/wiki/Administrative-rules

Thanks!

bgoglin added a commit to bgoglin/hwloc that referenced this pull request Sep 17, 2020
The old /etc/bash_completions.d/ is still available by
installing with something like:
  make install bashcompletionsdir='/etc/bash_completions.d

Thanks to Tomasz Kłoczko for the suggestion (see open-mpi#413 for details)

Signed-off-by: Brice Goglin <[email protected]>
bgoglin added a commit to bgoglin/hwloc that referenced this pull request Sep 17, 2020
No need to have "-completion" in the name.

Remove the .bash suffix so that the install path is similar to others
(renaming isn't trivial in automake).

Move to a bash subdirectory in case we ever ship a zsh completion file.

Thanks to Tomasz Kłoczko for the suggestion (see open-mpi#413)

Signed-off-by: Brice Goglin <[email protected]>
bgoglin added a commit that referenced this pull request Sep 17, 2020
No need to have "-completion" in the name.

Remove the .bash suffix so that the install path is similar to others
(renaming isn't trivial in automake).

Move to a bash subdirectory in case we ever ship a zsh completion file.

Thanks to Tomasz Kłoczko for the suggestion (see #413)

Signed-off-by: Brice Goglin <[email protected]>
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.

2 participants