make zsh and bash completion installation more consistent

[markus2330]

Is it possible that pkg-config respects CMAKE_INSTALL_PREFIX? Then we could avoid BASH_COMPLETION_COMPLETIONSDIR hack.

[rautesamtr]

The current hack is using the bash-completion-config.cmake for systems with bash-completion >= 2.2. For bash-completion < 2.2 (debian still ships 2.1) it falls back to the pkg-config file as the cmake file is not available.

It could be possible to honor CMAKE_INSTALL_PREFIX with some work but then we do not honer the install paths provided by cmake or pkg-config. Those paths are generated on making the corresponding package/install based on the install "PREFIX" set for them. So if for example bash-completion got installed into /usr/local/ BASH_COMPLETION_COMPLETIONSDIR would point to /usr/local/share/bash-completion/completions.

I think there will be no easy way to honor both Elektras CMAKE_INSTALL_PREFIX and the PREFIX that was used on the install of the other package e.g. bash-completion.

Honoring the cmake and/or pkg-config paths means we will get the appropriate paths that where generated on package generation and that will get sourced or loaded automatically. e.g. homebrews pkg-config file of bash-completion will expose the correct folder for it to be sourced.

[markus2330]

With hack I mainly mean setting BASH_COMPLETION_COMPLETIONSDIR from outside, we already have similar occurrences such as GOOGLETEST_ROOT and of course OPENSSL_MANUAL_OSX_HACK. It would be better if we can avoid such a variable which can be influenced from outside.

Yes, and there is no guarantee that a modified path from pkg-config will work. So you are right, its better to take it as is. Maybe we can honor CMAKE_INSTALL_PREFIX for the fallback path, i.e. use CMAKE_INSTALL_PREFIX + share/bash-completion/completions as fallback.

What I do not like about this fallback is that in one case it would be a INSTALL_SYSTEM_FILE (the path from pkg-config), but not in the fallback case. Such logic is not good for a build system.

[markus2330]

In wheezy the pkg-config variable bash-completion completionsdir could not be found. Because we use the same debian installation rules, the different fallback caused wheezy packaging to fail: http://community.markus-raab.org:8080/job/elektra-git-buildpackage-wheezy/lastFailedBuild/consoleFull

I now changed the fallback to /usr/share/bash-completion/completions, which is also more consistent to zsh completion.

Still there are left-overs: for zsh no pkg-config path is searched for. Does one exist?

I am not really in favour of BASH_COMPLETION_COMPLETIONSDIR, it complicates the build system and introduces another variable to support. It is also inconsistent to zsh-completion.

[rautesamtr]

In wheezy the pkg-config variable bash-completion completionsdir could not be found.

Do you have the bash-completion package installed? As far as i can tell from the package it should have the pkg-config file with the relevant variable.
https://packages.debian.org/wheezy/bash-completion

Still there are left-overs: for zsh no pkg-config path is searched for. Does one exist?

As far as i can tell there is no pkgconfig file installed with zsh. We will have to suffice with a hardcoded path or find out if the path is exposed elsewhere somehow.

I am not really in favour of BASH_COMPLETION_COMPLETIONSDIR, it complicates the build system and introduces another variable to support. It is also inconsistent to zsh-completion.

I will look into refactoring the cmake instructions into making the variable INTERNAL.

[rautesamtr]

And just to be clear. The variable you see in the cmake cache is not BASH_COMPLETION_COMPLETIONSDIR but bash-completion_DIR which seems to be automatically created by find_package(…). If this variable has the value bla_NOT_FOUND it only means the package was not found with cmakes find_package macro. It does not tell you if the BASH_COMPLETION_COMPLETIONSDIR was set by the pkg_get_variable.

[rautesamtr]

BASH_COMPLETION_COMPLETIONSDIR, it complicates the build system and introduces another variable to support.

And making this statement. It is just plain wrong for variables we do not expose tough cache, as they are not meant to be user settable. We are not talking about opinions here, but plain and clearly about technical facts.
Variables not marked as CACHE do not even appear in any of cmakes GUIs (qt, ncurse) and are meant for internal usage.
But if you want to enforce this, here have the variable as CACHE and INTERNAL… which is probably a bad idea as it brings its own downsides.

In addition for modern systems with up to date packages BASH_COMPLETION_COMPLETIONSDIR is set by the cmake module provided by the bash-completion package and is the proposed way to use bash-completion in cmake:
https://github.com/scop/bash-completion
If you think doing it this way is wrong please explain yourself better.
It takes allot of time trying to explain the same technical things again and again, that could be used to work on non build system related stuff.

An it is not a hack to just use the same variable name for older systems that to not set this variable themselves but a clean way to provide backward compatibility for 4 year old software.

I already explained at the GNU_INSTALL_DIR that such introduction of internal variables is not the same as exposing them trough cache. You clearly ignored this statements, and come now again with the same statement without providing a clear arguments stating why my previous explanation of this not making the build system more complicated where wrong.

Trough doing this we are repeating the same discourse again and again. And the fault is not on my side but on yours in this case. As like i said just ignore my arguments and repeat yours without adding any facts or arguments for your statement which is pretty much like a personal insult. Your implicit statement by doing so is that you do not even see worth in answering to them.

[markus2330]

Yes, CACHE and INTERNAL also seems overkill for me, but i did not want to annoy you for such irrelevant stuff. Unfortunately CMake does not define a clear interface for the outside world, every variable can be manipulated. Thus basically every variable (that has an effect which can be exploited) is a question of documentation, legacy and complications.

A usual compromise is to not care about what is not documented. So we simply remove the information that BASH_COMPLETION_COMPLETIONSDIR exists, and everyone using it must expect that it won't work in the next release. But then why have if (VAR) at all if we do not expect VAR to be set from the outside world? I think its more clear if it is not exploitable at all, but I have to agree that it is a matter of taste.

Another issue is the inconsistency between bash and zsh, so if the user can manipulate one, the other should be changable, too.

So what about having no if (BASH_COMPLETION_COMPLETIONSDIR)? Then its not exploitable and simply an internal variable set only by us.

[rautesamtr]

So what about having no if (BASH_COMPLETION_COMPLETIONSDIR)? Then its not exploitable and simply an internal variable set only by us.

This has been done #841 . The idea to check for BASH_COMPLETION_COMPLETIONSDIR was to check if the variable was set by the bash-completion module, but you are right that could have been user overridden even when not explicitly set as cache variable. With the current restructuring in #841 I had to drop that check anyway and I hope we now have a solution satisfying both our views.

[markus2330]

Yes, I think it does, thank you!

I only wanted to say that #841 fixes the BASH_COMPLETION_COMPLETIONSDIR issue twice and CACHE INTERNAL might not be needed. But I am not sure and further investigations are overkill, too.

Lets concentrate on the zsh issues #726 now.

[edusantana]

For me I had to use:

$ sudo chmod o+rw /usr/share/bash-completion/completions/ /usr/share/fish/vendor_completions.d/ /usr/share/zsh/vendor-completions/
eduardo@eduardo-Inspiron-7472:~$ brew install elektrainitiative/elektra/elektra 
==> Installing elektra from elektrainitiative/elektra
==> Downloading https://www.libelektra.org/ftp/elektra/releases/elektra-0.8.26.tar.gz
Already downloaded: /home/eduardo/.cache/Homebrew/downloads/9b3f4a8687a4ca17fbdcf3ad47b5801411ef9597313652cc087f7969622c67fe--elektra-0.8.26.tar.gz
==> cmake .. -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/home/eduardo/.linuxbrew/Cellar/elektra/0.8.26 -DBINDINGS='cpp' -DTOOLS='kdb;gen' -DPLUGINS='NODEP'
==> make install
==> Caveats
Bash completion has been installed to:
  /home/eduardo/.linuxbrew/etc/bash_completion.d
==> Summary
🍺  /home/eduardo/.linuxbrew/Cellar/elektra/0.8.26: 2,045 files, 43.8MB, built in 2 minutes 4 seconds

[markus2330]

@edusantana thank you for your report! I assume without chmod you get some error? Can you open a new issue describing the error?

@sanssecours is it possible with homebrew to move some files to different locations if a user runs brew?

[edusantana]

@markus2330 I had: ElektraInitiative/homebrew-elektra#2

[sanssecours]

is it possible with homebrew to move some files to different locations if a user runs brew?

Since Homebrew uses a Ruby DSL I would assume the answer is yes.

I think the problem is that the CMake variable INSTALL_SYSTEM_FILES is enabled by default on Linux:

libelektra/cmake/ElektraCache.cmake

Lines 238 to 242 in 09ef913

	if (${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
	option (INSTALL_SYSTEM_FILES "Install files to system directories" OFF)
	else ()
	option (INSTALL_SYSTEM_FILES "Install files to system directories" ON)
	endif ()

. I just pushed a commit that should hopefully fix the problem.