Skip to content

Conversation

JohannesLorenz
Copy link
Contributor

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

@tresf
Copy link
Member

tresf commented Sep 21, 2018

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
Copy link
Member

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
Copy link
Member

tresf commented Sep 22, 2018

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
Copy link
Member

tresf commented Sep 22, 2018

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
@tresf
Copy link
Member

tresf commented Sep 22, 2018

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

@JohannesLorenz
Copy link
Contributor Author

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.

* 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
@PhysSong
Copy link
Member

  • 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
Copy link
Member

tresf commented Sep 23, 2018

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.

Copy link
Member

@tresf tresf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@tresf
Copy link
Member

tresf commented Sep 23, 2018

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

@JohannesLorenz
Copy link
Contributor Author

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
Copy link
Member

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

@tresf
Copy link
Member

tresf commented Sep 24, 2018

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!

@tresf tresf merged commit 93dc557 into LMMS:stable-1.2 Sep 24, 2018
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants