-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add bash completion (#4534) #4604
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
Conversation
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()
#/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" |
How about this:
|
It's non-standard and a general bad idea, but fine.
No more whitelist please, please review the latest proposal. Less code, more scalable. |
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. |
Make bash-completion a cmake module
Awesome. Now time for testing. I think Fedora, Ubuntu and Arch should cover our bases. I can test the first two. |
I made some tests on Manjaro (~Arch). It's mostly OK, I only noticed:
|
* Remove irrelevant CMP0053 settings * Include FindUnixCommands to fix the shebang * Fix the install script call * Fix whitespaces * Fix Windows/macOS build * Minor: replace ugo+r with a+r
Should be fixed in 5c69315.
I also think it's weird, but fixing it requires a whitelisting. If @tresf is okay with this, please implement this! |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Although the /usr/local will likely (eventually, long-term) benefit the Homebrew formula... :/ |
Tested and working 👍 |
Since it's functioning, we can handle some special cases later, e.g. |
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! |
This should now work for
/usr
,/usr/local
, user installs and AppImages.