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.

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

Conversation

SoapGentoo
Copy link

@SoapGentoo SoapGentoo commented May 25, 2017

  • Being able to change the default directories is important
    for distros. Even more so with a consistent interface.
    GNUInstallDirs is the only Kitware supported way to change
    the defaults without downstream consumers of CMake
    making up new and inconsistent options between each other.

Associated PR for the Gettext module: ooxi/CMake-Gettext#1

* Being able to change the default directories is important
  for distros. Even more so with a consistent interface.
  GNUInstallDirs is the only Kitware supported way to change
  the defaults without downstream consumers of CMake
  making up new and inconsistent options between each other.
* It is customary not to install README files in data
  directories but rather in DOCDIR.
* Different distros have compiled boost with different
  -std= options. Hardcoding this only wreaks massive
  havoc with parts of the ecosystem already using C++11.
@ooxi
Copy link
Owner

ooxi commented May 26, 2017

While I really like the patch I cannot merge as is, because of the following reasons:

  • b770ce9 changes too many unrelated options at once. I started a GitHub review with the issues
  • 38a00e9 I'm not sure if I can omit hard coding C++98 since I need at least C++98 support but do not require newer versions. I get your point about libraries in a distribution compiled with another standard. Would it be possible to define C++98 iff no other standard is required?

Nevertheless thanks your contribution, I'd really like to support GNUInstallDirs! Are you planning on packaging Violetland on a distribution?

@SoapGentoo
Copy link
Author

@ooxi

  1. how do you mean you need "at least" C++98? Literally every compiler since 2003 supports C++98 and (below GCC 6) also runs in C++98 mode by default. Hardcoding the C++ standard causes issues for us on Gentoo, where Boost is compiled in C++14 mode. There is no need to explicitly request C++98 support. In fact, things are even worse on Windows: MSVC in general doesn't allow for changing the language standard, hence trying to enforce C++98 is imo futile. Just use the compiler default, and aim for a codebase that works with both C++98 and C++11 and everything will work fine.
  2. I can split up the CMakeLists.txt changes, but imo a lot of that is generally pointless, as they change lines in close proximity which will make reverting impossible either way.

We have violetland packaged in Gentoo currently, but using GNUInstallDirs would not just help Gentoo, but all distributions, as its the only idiomatic way to specify the dirs.

@ooxi
Copy link
Owner

ooxi commented May 26, 2017

Thanks for your feedback. I will integrate support for GNUInstallDirs but with the possibility to overwrite.

@SoapGentoo
Copy link
Author

@ooxi GNUInstallDir is perfectly overridable (that's the point of the GNU conventions). I recommend adding something along the lines of

if(WIN32)
	set(CMAKE_INSTALL_LOCALEDIR "mylocaledir" CACHE STRING "Windows locale dir")
	GNUInstallDirs_get_absolute_install_dir(CMAKE_INSTALL_FULL_LOCALEDIR CMAKE_INSTALL_LOCALEDIR)
endif(WIN32)

instead of making the inclusion of GNUInstallDir conditional. What would you like me to change?

@ooxi
Copy link
Owner

ooxi commented May 26, 2017

That's even better! Essentially cmake -DCMAKE_INSTALL_PREFIX=../dist .. should still work after this patch :-)

@SoapGentoo
Copy link
Author

@ooxi also I dont see the Github review? Did you forget to click request changes maybe?

Copy link
Owner

@ooxi ooxi left a comment

Choose a reason for hiding this comment

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

Very good, would like to merge when resolved

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 👍

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?

CMakeLists.txt Outdated
@@ -173,16 +166,15 @@ set(VIOLET_SOURCES
# 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")
add_compile_options(-std=c++98)
Copy link
Owner

Choose a reason for hiding this comment

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

Changing set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} XXX") to add_compile_options(XXX) should be done in a separate commit.

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?

@ooxi
Copy link
Owner

ooxi commented May 26, 2017

@SoapGentoo you are correct I did not know you had to submit the review (never worked with reviews before). Thanks for your patience :-)

@@ -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 :/

@ooxi ooxi added this to the Release 0.6 milestone Dec 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants