Skip to content
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.

Sign up for GitHub

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

Use GNUInstallDirs for specifying default directories #140

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use GNUInstallDirs for specifying default directories #140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b770ce9
Use GNUInstallDirs for specifying default directories
SoapGentoo May 25, 2017
1a7735c
Do not install README files in DATADIR
SoapGentoo May 25, 2017
38a00e9
Do not hardcode C++98
SoapGentoo May 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 17 additions & 35 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ PROJECT(violetland)
# identify Clang, otherwise we can go back to 2.6.0
#
# @see https://cmake.org/cmake/help/v2.8.10/cmake.html#variable:CMAKE_LANG_COMPILER_ID
CMAKE_MINIMUM_REQUIRED(VERSION 2.6.0 FATAL_ERROR)
CMAKE_MINIMUM_REQUIRED(VERSION 2.8)

CMAKE_MINIMUM_REQUIRED(VERSION 2.8.12)
Copy link
Owner

Choose a reason for hiding this comment

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

Why 2.8.12?

Copy link
Author

Choose a reason for hiding this comment

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

add_compile_options requires 2.8.12 and is the proper idiomatic way to add flags

Copy link
Owner

Choose a reason for hiding this comment

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

Since the oldest supported Ubuntu version (14.04) ships with CMake 2.8.12 I agree bumping the version requirement to get add_compile_options is the right thing to do 👍



# Adhere to GNU filesystem layout conventions
include(GNUInstallDirs)


# SDL
Expand Down Expand Up @@ -57,15 +57,8 @@ set(VIOLET_LIBRARIES ${VIOLET_LIBRARIES} ${Intl_LIBRARIES})



# LOCALEDIR must be set up before including po/CMakeLists.txt
if (DEFINED LOCALE_INSTALL_DIR)
set(LOCALEDIR "${LOCALE_INSTALL_DIR}")
else(DEFINED LOCALE_INSTALL_DIR)
set(LOCALEDIR "${CMAKE_INSTALL_PREFIX}/locale")
endif(DEFINED LOCALE_INSTALL_DIR)

add_definitions(-DINSTALL_PREFIX="${CMAKE_INSTALL_PREFIX}")
add_definitions(-DLOCALE_DIR="${LOCALEDIR}")
add_definitions(-DLOCALE_DIR="${CMAKE_INSTALL_FULL_LOCALEDIR}")
Copy link
Owner

Choose a reason for hiding this comment

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

If I understand the documentation of CMAKE_INSTALL_FULL_LOCALEDIR correctly this is always and absolute path. Therefore it's no longer possible to do a local build without system installation.

I would like to continue supporting LOCALE_INSTALL_DIR and only falling back to CMAKE_INSTALL_FULL_LOCALEDIR if undefined.

Copy link
Author

Choose a reason for hiding this comment

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

what do you mean by a "local" build? In the source tree? Not into a system dir?

Copy link
Owner

Choose a reason for hiding this comment

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

By local build I mean into any directory, specifically a system dir (for example install relative to ../dist so the final executable and resources can be moved freely).

I agree that this is not an issue for distributions but for development it's extremely helpful

Copy link
Author

Choose a reason for hiding this comment

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

installing relative to ../dist is fine as is, the problem with the previous solution was that it is brittle: resources /= "../share/violetland"; assumes a rigid structure. What if the executable is installed as ../dist/violetland (without an intermediate bin/ dir) and the data files in ../dist/share/violetland? Then the old solution will break either way. With GNUInstallDir, you can install into ../dist, you just cannot relocate it once you have make installed it, which, while a bit of a pity, I still think is much more stable than relying on an implicit directory structure.

Copy link
Author

Choose a reason for hiding this comment

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

@ooxi ping?


add_subdirectory(po)

Expand Down Expand Up @@ -168,21 +161,13 @@ set(VIOLET_SOURCES



# Specify C++ standard, I would love to use C++11 but at least on Ubuntu boost
# is compiled with C++98 which will result in linking errors. Seems to be fixed
# in boost 1.57 but Ubuntu before wily does not ship that version.
#
# @see https://svn.boost.org/trac/boost/ticket/6779
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++98")

# Compiler diagnostics are most useful
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wextra -Wno-ignored-qualifiers -Wno-unused-parameter")
add_compile_options(-Wall -Wextra -Wno-ignored-qualifiers -Wno-unused-parameter)

if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
# clang++ does not understand boost_static_assert_typedef's unused
# attribute
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unused-local-typedef")
add_compile_options(-Wno-unused-local-typedef)
endif()


Expand All @@ -198,7 +183,7 @@ if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
#
# @see https://gcc.gnu.org/onlinedocs/gcc-5.3.0/gcc/x86-Windows-Options.html#x86-Windows-Options
if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -mwindows")
add_compile_options(-mwindows)
endif()

endif()
Expand All @@ -213,33 +198,30 @@ target_link_libraries(violetland ${VIOLET_LIBRARIES})


# Install executable and assets
if(DEFINED DATA_INSTALL_DIR)
set(relResPath ${DATA_INSTALL_DIR})
add_definitions(-DDATA_INSTALL_DIR="${DATA_INSTALL_DIR}/")
else(DEFINED DATA_INSTALL_DIR)
set(relResPath share/violetland/)
endif(DEFINED DATA_INSTALL_DIR)

install(TARGETS violetland DESTINATION bin)
add_definitions(-DDATA_INSTALL_DIR="${CMAKE_INSTALL_FULL_DATADIR}/violetland/")

install(TARGETS violetland DESTINATION ${CMAKE_INSTALL_BINDIR})
Copy link
Owner

Choose a reason for hiding this comment

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

Same as before, doesn't this demand a system installation?

Copy link
Author

Choose a reason for hiding this comment

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

CMAKE_INSTALL_BINDIR has nothing to do with a system installation, you can still install it in ~/my_test_dir

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry I misplaced my review comment: it was about setting DATA_INSTALL_DIR to CMAKE_INSTALL_FULL_DATADIR which would imply an absolute path, wouldn't it?

install(FILES
icon-light.png
DESTINATION ${CMAKE_INSTALL_DATADIR}/violetland)
install(FILES
README.md
README_RU.TXT
icon-light.png
DESTINATION ${relResPath})
DESTINATION ${CMAKE_INSTALL_DOCDIR})
install(DIRECTORY
fonts
sounds
images
monsters
weapon
music
DESTINATION ${relResPath}
DESTINATION ${CMAKE_INSTALL_DATADIR}/violetland
PATTERN ".*" EXCLUDE
PATTERN "*~" EXCLUDE)
install(FILES
violetland.desktop
DESTINATION share/applications)
DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/applications)
install(FILES
icon-light.png
DESTINATION share/pixmaps
DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/pixmaps
RENAME violetland.png)
10 changes: 1 addition & 9 deletions src/system/utility/FileUtility.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,13 @@ violet::FileUtility* violet::FileUtility::ofUnix() {

/* Application binary
*/
#ifndef INSTALL_PREFIX
#define INSTALL_PREFIX "/usr/local";
#endif //INSTALL_PREFIX
boost::filesystem::path application = INSTALL_PREFIX;
Copy link
Author

Choose a reason for hiding this comment

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

@ooxi I just noticed this too. By hardcoding the prefix into the source, your installation is never relocatable, because everything is relative to it. Hence, I would just settle for full paths unconditionally, given that it didnt work previously anyhow.

Copy link
Author

Choose a reason for hiding this comment

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

@ooxi Tell me how you'd like to proceed. I believe we should just use the GNUInstallDir solution, as the current solution is not relocatable either.

Copy link
Owner

Choose a reason for hiding this comment

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

Normally the installation is relocatable since INSTALLATION_PREFIX etc. can be set to a relative path. I'm actually not sure on how to proceed :/

application /= "bin";


/* Application resources
*/
#ifndef DATA_INSTALL_DIR
boost::filesystem::path resources = application;
resources /= "../share/violetland";
#else //DATA_INSTALL_DIR
resources = DATA_INSTALL_DIR;
#endif //DATA_INSTALL_DIR
boost::filesystem::path resources = DATA_INSTALL_DIR;


/* User data
Expand Down