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

Take CMAKE_INSTALL_PREFIX into account in MULTIMC_JARS_LOCATION #2103

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@freundlich
Contributor

freundlich commented Jan 13, 2018

When installing MultiMC with the lin-system layout and specifying an
install prefix that is not the empty string, then MultiMC looks for its
Jars in the wrong location. Fix this by appending CMAKE_INSTALL_PREFIX.

Take CMAKE_INSTALL_PREFIX into account in MULTIMC_JARS_LOCATION
When installing MultiMC with the lin-system layout and specifying an
install prefix that is not the empty string, then MultiMC looks for its
Jars in the wrong location. Fix this by appending CMAKE_INSTALL_PREFIX.
@srakitnican

This comment has been minimized.

Contributor

srakitnican commented Jan 14, 2018

CMAKE_INSTALL_PREFIX Should be /usr by default (It is actually /usr/local if not set), that means MultiMC_SHARE_DEST_DIR (and the rest MultiMC_*) should be changed to not include usr/ prefix.

I am not exactly sure, but I think new generation package managers could benefit from this as well.

@srakitnican

This comment has been minimized.

Contributor

srakitnican commented Jan 14, 2018

Something like the following, also adds the library suffix if LIB_SUFFIX is set as found on some distributions like Fedora.

--- /tmp/CMakeLists.txt	2018-01-14 12:08:30.395594381 +0100
+++ /tmp/CMakeLists.txt.patched	2018-01-14 12:07:16.604106019 +0100
@@ -393,9 +393,13 @@
 	install(PROGRAMS package/linux/MultiMC DESTINATION ${BUNDLE_DEST_DIR})
 
 elseif(MultiMC_LAYOUT_REAL STREQUAL "lin-system")
-	set(MultiMC_BINARY_DEST_DIR "usr/bin" CACHE STRING "Relative path from packaging root to the binary directory")
-	set(MultiMC_LIBRARY_DEST_DIR "usr/lib" CACHE STRING "Relative path from packaging root to the library directory")
-	set(MultiMC_SHARE_DEST_DIR "usr/share/multimc" CACHE STRING "Relative path from packaging root to the shared data directory")
+	if(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
+          SET(CMAKE_INSTALL_PREFIX "/usr" CACHE PATH "Base directory for executables and libraries" FORCE)
+       endif(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
+
+	set(MultiMC_BINARY_DEST_DIR "${CMAKE_INSTALL_PREFIX}/bin" CACHE STRING "Relative path from packaging root to the binary directory")
+	set(MultiMC_LIBRARY_DEST_DIR "${CMAKE_INSTALL_PREFIX}/lib${LIB_SUFFIX}" CACHE STRING "Relative path from packaging root to the library directory")
+	set(MultiMC_SHARE_DEST_DIR "${CMAKE_INSTALL_PREFIX}/share/multimc" CACHE STRING "Relative path from packaging root to the shared data directory")
 	set(MultiMC_APP_BINARY_NAME "multimc" CACHE STRING "Name of the MultiMC binary for the purposes of linux packaging")
 	set(JARS_DEST_DIR "${MultiMC_SHARE_DEST_DIR}")
@freundlich

This comment has been minimized.

Contributor

freundlich commented Jan 14, 2018

I agree that removing usr from all of these paths is the right thing to do. However, I think that changing the default value of CMAKE_INSTALL_PREFIX isn't really needed and might add additional confusion.

@peterix

This comment has been minimized.

Member

peterix commented Jan 16, 2018

OK. I clicked a github button again and it broke things by adding merge commits -- again...

I have no idea how to get rid of this now. Probably will cherry-pick just the actual patch.

@peterix

This comment has been minimized.

Member

peterix commented Jan 16, 2018

Cherry-picked the change.

@srakitnican I'm going to make some larger changes to the build system and include that. Looks good.

@peterix peterix closed this Jan 16, 2018

peterix added a commit that referenced this pull request Jan 16, 2018

GH-2103 Take CMAKE_INSTALL_PREFIX into account in MULTIMC_JARS_LOCATION
When installing MultiMC with the lin-system layout and specifying an
install prefix that is not the empty string, then MultiMC looks for its
Jars in the wrong location. Fix this by appending CMAKE_INSTALL_PREFIX.

peterix added a commit that referenced this pull request Jan 16, 2018

@peterix

This comment has been minimized.

Member

peterix commented Jan 16, 2018

Ok, this was extremely messy... Basically everything broke.

A few force-pushes and some fixing of the AUR package later, this is how this looks:

elseif(MultiMC_LAYOUT_REAL STREQUAL "lin-system")
if(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
SET(CMAKE_INSTALL_PREFIX "/usr" CACHE PATH "Base directory for executables and libraries" FORCE)
endif()
set(MultiMC_BINARY_DEST_DIR "bin" CACHE STRING "Path to the binary directory")
set(MultiMC_LIBRARY_DEST_DIR "lib${LIB_SUFFIX}" CACHE STRING "Path to the library directory")
set(MultiMC_SHARE_DEST_DIR "share/multimc" CACHE STRING "Path to the shared data directory")
set(MultiMC_APP_BINARY_NAME "multimc" CACHE STRING "Name of the MultiMC binary for the purposes of linux packaging")
set(JARS_DEST_DIR "${MultiMC_SHARE_DEST_DIR}")
set(BINARY_DEST_DIR ${MultiMC_BINARY_DEST_DIR})
set(LIBRARY_DEST_DIR ${MultiMC_LIBRARY_DEST_DIR})
MESSAGE(STATUS "Compiling for linux system with ${MultiMC_SHARE_DEST_DIR} and MULTIMC_LINUX_DATADIR")
set_target_properties(MultiMC PROPERTIES OUTPUT_NAME ${MultiMC_APP_BINARY_NAME})
target_compile_definitions(MultiMC PRIVATE "-DMULTIMC_JARS_LOCATION=${CMAKE_INSTALL_PREFIX}/${MultiMC_SHARE_DEST_DIR}/jars" "-DMULTIMC_LINUX_DATADIR"
)
# install as bundle with no dependencies included
set(INSTALL_BUNDLE "nodeps")

@srakitnican

This comment has been minimized.

Contributor

srakitnican commented Jan 16, 2018

What exactly did not work? I tested my changes and it was fine for me. With my changes applied, change from @freundlich is redundant.

I believe this would result in jars being installed in /usr/usr/share/multimc:
https://github.com/MultiMC/MultiMC5/blob/d0e58acd843fcc04463497e9ffbbcab0dee72d55/application/CMakeLists.txt#L402,L411

@srakitnican

This comment has been minimized.

Contributor

srakitnican commented Jan 16, 2018

I agree that removing usr from all of these paths is the right thing to do. However, I think that changing the default value of CMAKE_INSTALL_PREFIX isn't really needed and might add additional confusion.

Well I did it like that to not make any further noticeable changes in terms where it installs. By default prefix is /usr/local, without this it would be required to manually specify -DCMAKE_INSTALL_PREFIX=/usr if you want to install to /usr. I am fine with /usr/local being default, but I didn't want to suggest to break that for existing users.

@srakitnican

This comment has been minimized.

Contributor

srakitnican commented Jan 17, 2018

I believe this would result in jars being installed in /usr/usr/share/multimc:
https://github.com/MultiMC/MultiMC5/blob/d0e58acd843fcc04463497e9ffbbcab0dee72d55/application/CMakeLists.txt#L402,L411

I did some tests and apparently I was wrong. It seems CMake automatically combines multiple variables into one, not sure how this works.

What it also does is that it prepends CMAKE_INSTALL_PREFIX to all files with relative path in install command so specifying it manually is not necessary.

DESTINATION
Specify the directory on disk to which a file will be installed. If a full path (with a leading slash or drive letter) is given it is used directly. If a relative path is given it is interpreted relative to the value of the CMAKE_INSTALL_PREFIX variable. The prefix can be relocated at install time using the DESTDIR mechanism explained in the CMAKE_INSTALL_PREFIX variable documentation.

I have found that the following is all that is needed to MultiMC 0.6.0.

--- application/CMakeLists.txt	2017-12-31 19:39:46.000000000 +0100
+++ application/CMakeLists.txt.patched	2018-01-17 09:21:28.018546384 +0100
@@ -393,9 +393,12 @@
 	install(PROGRAMS package/linux/MultiMC DESTINATION ${BUNDLE_DEST_DIR})
 
 elseif(MultiMC_LAYOUT_REAL STREQUAL "lin-system")
-	set(MultiMC_BINARY_DEST_DIR "usr/bin" CACHE STRING "Relative path from packaging root to the binary directory")
-	set(MultiMC_LIBRARY_DEST_DIR "usr/lib" CACHE STRING "Relative path from packaging root to the library directory")
-	set(MultiMC_SHARE_DEST_DIR "usr/share/multimc" CACHE STRING "Relative path from packaging root to the shared data directory")
+	if(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
+		set(CMAKE_INSTALL_PREFIX "/usr" CACHE PATH "Base directory for executables and libraries" FORCE)
+	endif()
+	set(MultiMC_BINARY_DEST_DIR "bin" CACHE STRING "Relative path from packaging root to the binary directory")
+	set(MultiMC_LIBRARY_DEST_DIR "lib${LIB_SUFFIX}" CACHE STRING "Relative path from packaging root to the library directory")
+	set(MultiMC_SHARE_DEST_DIR "share/multimc" CACHE STRING "Relative path from packaging root to the shared data directory")
 	set(MultiMC_APP_BINARY_NAME "multimc" CACHE STRING "Name of the MultiMC binary for the purposes of linux packaging")
 	set(JARS_DEST_DIR "${MultiMC_SHARE_DEST_DIR}")

Also even setting the CMAKE_INSTALL_PREFIX is not needed since it was probably broken before as well since the relative path was used, thus if CMAKE_INSTALL_PREFIX was not set it would install it to /usr/local/usr.... So with this new change we are breaking this use case anyway so we might as well not set the CMAKE_INSTALL_PREFIX by default as suggested by @freundlich . Users will have to adjust their CMAKE_INSTALL_PREFIX anyway so we might as well set the sane defaults since /usr is meant to be used by packages. /usr/local is the right place for manually compiled stuff.

--- application/CMakeLists.txt	2017-12-31 19:39:46.000000000 +0100
+++ application/CMakeLists.txt.patched	2018-01-17 09:21:28.018546384 +0100
@@ -393,9 +393,12 @@
 	install(PROGRAMS package/linux/MultiMC DESTINATION ${BUNDLE_DEST_DIR})
 
 elseif(MultiMC_LAYOUT_REAL STREQUAL "lin-system")
-	set(MultiMC_BINARY_DEST_DIR "usr/bin" CACHE STRING "Relative path from packaging root to the binary directory")
-	set(MultiMC_LIBRARY_DEST_DIR "usr/lib" CACHE STRING "Relative path from packaging root to the library directory")
-	set(MultiMC_SHARE_DEST_DIR "usr/share/multimc" CACHE STRING "Relative path from packaging root to the shared data directory")
+	set(MultiMC_BINARY_DEST_DIR "bin" CACHE STRING "Relative path from packaging root to the binary directory")
+	set(MultiMC_LIBRARY_DEST_DIR "lib${LIB_SUFFIX}" CACHE STRING "Relative path from packaging root to the library directory")
+	set(MultiMC_SHARE_DEST_DIR "share/multimc" CACHE STRING "Relative path from packaging root to the shared data directory")
 	set(MultiMC_APP_BINARY_NAME "multimc" CACHE STRING "Name of the MultiMC binary for the purposes of linux packaging")
 	set(JARS_DEST_DIR "${MultiMC_SHARE_DEST_DIR}")

srakitnican added a commit to srakitnican/MultiMC5 that referenced this pull request Jan 18, 2018

MultiMCGH-2103 Make /usr/local the default prefix for lin-system
/usr/local is a sane default since /usr is meant to be used by packages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment