Add bash completion (#4534)

[JohannesLorenz]

This should now work for /usr, /usr/local, user installs and AppImages.

[tresf]

Due to all the whitelisting problems, I propose we go back to a shell script...

here's my proposal:

# install bash completion support
# - Windows is not supported
# - TODO: macOS support can be provided through Homebrew
IF(NOT LMMS_BUILD_WIN32 AND NOT LMMS_BUILD_APPLE)
	PKG_CHECK_MODULES(BASH_COMPLETION bash-completion)
	PKG_GET_VARIABLE(BASHCOMP_PKG_PATH bash-completion completionsdir)

	IF("${BASHCOMP_PKG_PATH}" STREQUAL "")
		SET(BASHCOMP_PKG_PATH "/usr/share/bash-completion/completions")
	ENDIF()

	# fallback for non-root installs
	SET(BASHCOMP_USER_PATH "${CMAKE_INSTALL_PREFIX}${BASHCOMP_PKG_PATH}")

        # use script to handle permission issues gracefully for non-root installs
	CONFIGURE_FILE("install_bash_completion.sh.in" "install_bash_completion.sh" @ONLY)
	INSTALL(CODE "EXECUTE_PROCESS(COMMAND chmod u+x install_bash_completion.sh WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} )")
	INSTALL(CODE "EXECUTE_PROCESS(COMMAND install_bash_completion.sh WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} )")

	MESSAGE(STATUS "Bash completions will be installed to ${BASHCOMP_PKG_PATH} or fallback to ${BASHCOMP_USER_PATH} if unwritable.")
ENDIF()

install_bash_completion.sh.in

#/bin/bash

set -e

BASHCOMP_PKG_PATH="@BASHCOMP_USER_PATH@"
if [ -w "@BASHCOMP_PKG_PATH@" ]; then
	BASHCOMP_PKG_PATH="@BASHCOMP_PKG_PATH@"
fi

echo -e "\nInstalling bash completion..."
mkdir -p "$BASHCOMP_PKG_PATH"
cp "@CMAKE_CURRENT_SOURCE_DIR@/lmms" "$BASHCOMP_PKG_PATH"
chmod ugo+r "$BASHCOMP_PKG_PATH/lmms"
echo -e "Bash completion installed to $BASHCOMP_PKG_PATH"

[PhysSong]

How about this:

• Add an option to force the install path for the completion script, e.g. BASHCOMP_INSTALL_PATH
• If no path specified, install the script into the install prefix for some whitelisted locations

[tresf]

Add an option to force the install path for the completion script, e.g. BASHCOMP_INSTALL_PATH

It's non-standard and a general bad idea, but fine.

If no path specified, install the script into the install prefix for some whitelisted locations

No more whitelist please, please review the latest proposal. Less code, more scalable.

[tresf]

Created a new PR against this PR's branch to bring all of this conversation/concerns/logic into a dedicated cmake module. JohannesLorenz#1. Feedback appreciated.

[tresf]

Awesome. Now time for testing. I think Fedora, Ubuntu and Arch should cover our bases. I can test the first two.

[JohannesLorenz]

I made some tests on Manjaro (~Arch). It's mostly OK, I only noticed:

• travis-ci failed
• The shebang line of the install script is just #! for me.
• It's a bit weird that a prefix of /usr/local says it wants to install to /usr/share and only fallback to /usr/local/share. Though I know this is part of the design.

[PhysSong]

• travis-ci failed
• The shebang line of the install script is just #! for me.

Should be fixed in 5c69315.

It's a bit weird that a prefix of /usr/local says it wants to install to /usr/share and only fallback to /usr/local/share. Though I know this is part of the design.

I also think it's weird, but fixing it requires a whitelisting. If @tresf is okay with this, please implement this!

[tresf]

If pkg-config and cmake doesn't care about the prefix, I'm not sure why we should. The prefix is just as strange looking when installing to /opt. It's just a spaghetti code mess if we don't trust the modules provided by the OS.

[tresf]

👍

[tresf]

Although the /usr/local will likely (eventually, long-term) benefit the Homebrew formula... :/

[JohannesLorenz]

Tested and working 👍
I don't want to propose whitelists again, we'd be spinning in circles. No further changes required from my side.

[PhysSong]

Since it's functioning, we can handle some special cases later, e.g. /usr/local. 👍

[tresf]

Testing passed on Fedora 27 and Ubuntu 18.04. Merging. Thanks @JohannesLorenz and @PhysSong for all of the back-and forth on this. We've been in PM for about a week trying to tackle this and now we have a cmake module to do it. Excellent team work!