widelands-dev team mailing list archive

[Hans Joachim Desserud <launchpad@xxxxxxxxxxxx>]

Hans Joachim Desserud has proposed merging lp:~widelands-dev/widelands/reducing-paths into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/reducing-paths/+merge/239645

Removes 
INSTALL_PREFIX
INSTALL_LOCALEDIR
WL_INSTALL_BINDIR
as Cmake variables because they are strictly not needed.

Things which should be considered:
r7228 was a hack to be buildable. That should probably be fixed in a better way before we merge. See comment for details.

What has been tested:
Building/running with compile.sh: Works
Using the PPA (with modified packaging): Works

The game runs without any warnings it can't find datadir. The language list in options is populated and working.


What needs to be tested:
I haven't had the chance to test how well things work on other platforms with this:
Microsoft Windows
Mac OS X
Also, it would be nice if someone who runs make install regularly could take a check. I believe this should work fine, but I don't run it normally so I don't know whether it requires special set up for the normal case.


(This is the WL-half of this patch, the other part is for the debian packaging. We will need both merged for the PPA to continue working.)
-- 
https://code.launchpad.net/~widelands-dev/widelands/reducing-paths/+merge/239645
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/reducing-paths into lp:widelands.

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2014-10-16 04:54:10 +0000
+++ CMakeLists.txt	2014-10-25 17:16:34 +0000
@@ -13,14 +13,7 @@
 # If not specified, we are going to use the current directory. Also on Linux.
 # Packagers (or people using make install) have to set those variables.
 wl_set_if_unset(WL_PATHS_ARE_ABSOLUTE "false")
-wl_set_if_unset(WL_INSTALL_PREFIX ".")
 wl_set_if_unset(WL_INSTALL_DATADIR ".")
-wl_set_if_unset(WL_INSTALL_LOCALEDIR "locale")
-wl_set_if_unset(WL_INSTALL_BINDIR ".")
-
-if (CMAKE_INSTALL_PREFIX STREQUAL CMAKE_BINARY_DIR)
-  message(FATAL_ERROR "Build directory and install directory must not be the same.")
-endif (CMAKE_INSTALL_PREFIX STREQUAL CMAKE_BINARY_DIR)
 
 if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
   if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS 4.7)
@@ -189,7 +182,7 @@
 if (NOT DEFINED WL_VERSION)
   add_custom_target (
     BzrRevision ALL
-    COMMAND ${CMAKE_COMMAND} -DWL_INSTALL_PREFIX=${WL_INSTALL_PREFIX} -DWL_INSTALL_BINDIR=${WL_INSTALL_BINDIR} -DWL_INSTALL_DATADIR=${WL_INSTALL_DATADIR} -DWL_INSTALL_LOCALEDIR=${WL_INSTALL_LOCALEDIR} -DPYTHON_EXECUTABLE=${PYTHON_EXECUTABLE} -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DSOURCE_DIR=${CMAKE_CURRENT_SOURCE_DIR} -DBINARY_DIR=${CMAKE_CURRENT_BINARY_DIR} -P ${CMAKE_CURRENT_SOURCE_DIR}/cmake/BzrRevision.cmake
+    COMMAND ${CMAKE_COMMAND} -DWL_INSTALL_DATADIR=${WL_INSTALL_DATADIR} -DPYTHON_EXECUTABLE=${PYTHON_EXECUTABLE} -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DSOURCE_DIR=${CMAKE_CURRENT_SOURCE_DIR} -DBINARY_DIR=${CMAKE_CURRENT_BINARY_DIR} -P ${CMAKE_CURRENT_SOURCE_DIR}/cmake/BzrRevision.cmake
   )
 
   # Detect version now
@@ -205,7 +198,7 @@
 else (NOT DEFINED WL_VERSION)
   add_custom_target (
     InputRevision ALL
-    COMMAND ${CMAKE_COMMAND} -DWL_INSTALL_PREFIX=${WL_INSTALL_PREFIX} -DWL_INSTALL_BINDIR=${WL_INSTALL_BINDIR} -DWL_INSTALL_DATADIR=${WL_INSTALL_DATADIR} -DWL_INSTALL_LOCALEDIR=${WL_INSTALL_LOCALEDIR} -DWL_VERSION=${WL_VERSION} -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DSOURCE_DIR=${CMAKE_CURRENT_SOURCE_DIR} -DBINARY_DIR=${CMAKE_CURRENT_BINARY_DIR} -P ${CMAKE_CURRENT_SOURCE_DIR}/cmake/InputRevision.cmake
+    COMMAND ${CMAKE_COMMAND} -DWL_INSTALL_DATADIR=${WL_INSTALL_DATADIR} -DWL_VERSION=${WL_VERSION} -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DSOURCE_DIR=${CMAKE_CURRENT_SOURCE_DIR} -DBINARY_DIR=${CMAKE_CURRENT_BINARY_DIR} -P ${CMAKE_CURRENT_SOURCE_DIR}/cmake/InputRevision.cmake
   )
 endif (NOT DEFINED WL_VERSION)
 

=== modified file 'cmake/WlFunctions.cmake'
--- cmake/WlFunctions.cmake	2014-07-17 14:34:32 +0000
+++ cmake/WlFunctions.cmake	2014-10-25 17:16:34 +0000
@@ -255,5 +255,7 @@
 
   _common_compile_tasks()
 
-  install(TARGETS ${NAME} DESTINATION ${WL_INSTALL_BINDIR} COMPONENT ExecutableFiles)
+  #Quoting the CMake documentation on DESTINATION:
+  #"If a relative path is given it is interpreted relative to the value of CMAKE_INSTALL_PREFIX"
+  install(TARGETS ${NAME} DESTINATION "." COMPONENT ExecutableFiles)
 endfunction()

=== modified file 'src/config.h.cmake'
--- src/config.h.cmake	2014-10-16 04:54:10 +0000
+++ src/config.h.cmake	2014-10-25 17:16:34 +0000
@@ -4,35 +4,12 @@
 // Defines if paths are absolute or relative to the executable directory.
 constexpr bool PATHS_ARE_ABSOLUTE = @WL_PATHS_ARE_ABSOLUTE@;
 
-// This is the install path prefix;
-// Absolute path if PATHS_ARE_ABSOLUTE, otherwise relative to the directory
-// where the executable is in.
-// For portable installation, this needs to be "." and PATHS_ARE_ABSOLUTE must be false.
-// For a standard Linux installation, this should be "/usr/local" and PATHS_ARE_ABSOLUTE should be true
-#define INSTALL_PREFIX "@WL_INSTALL_PREFIX@"
-
-// This is the path where the executable binary is located;
-// Path is always relative to INSTALL_PREFIX, ignoring PATHS_ARE_ABSOLUTE.
-// For portable installation, this needs to be ".".
-// For a standard Linux installation, this should be "games"
-#define INSTALL_BINDIR "@WL_INSTALL_BINDIR@"
-
 // This is the path where the data files are located;
-// Path is always relative to INSTALL_PREFIX, ignoring PATHS_ARE_ABSOLUTE.
+// It ignores PATHS_ARE_ABSOLUTE.
 // For portable installation, this needs to be ".".
 // For a standard Linux installation, this should be "share/games/widelands"
 #define INSTALL_DATADIR "@WL_INSTALL_DATADIR@"
 
-// This is the locale directory, usually located within the data directory;
-// Absolute path if PATHS_ARE_ABSOLUTE, otherwise relative to the directory
-// where the executable is in.
-// For portable installation, this needs to be "locale" and PATHS_ARE_ABSOLUTE
-// must be false.
-// For a standard Linux installation, this should be
-// "/usr/local/share/games/widelands/locale" and PATHS_ARE_ABSOLUTE should be
-// true.
-#define INSTALL_LOCALEDIR "@WL_INSTALL_LOCALEDIR@"
-
 // don't know if this causes problems on Windows but it solves the problems
 // finding a user's home directory on Linux
 #define HAS_GETENV

=== modified file 'src/logic/map_info.cc'
--- src/logic/map_info.cc	2014-09-20 09:37:47 +0000
+++ src/logic/map_info.cc	2014-10-25 17:16:34 +0000
@@ -47,7 +47,7 @@
 	SDL_Init(SDL_INIT_VIDEO);
 
 	g_fs = new LayeredFileSystem();
-	g_fs->add_file_system(&FileSystem::create(INSTALL_PREFIX + std::string("/") + INSTALL_DATADIR));
+	g_fs->add_file_system(&FileSystem::create(INSTALL_DATADIR));
 
 #ifdef HAS_GETENV
 	char dummy_video_env[] = "SDL_VIDEODRIVER=dummy";

=== modified file 'src/ui_fsmenu/options.cc'
--- src/ui_fsmenu/options.cc	2014-09-29 19:25:24 +0000
+++ src/ui_fsmenu/options.cc	2014-10-25 17:16:34 +0000
@@ -54,7 +54,26 @@
 void add_languages_to_list(UI::Listselect<std::string>* list, const std::string& language) {
 
 	Section* s = &g_options.pull_section("global");
-	FilenameSet files = g_fs->list_directory(s->get_string("localedir", INSTALL_LOCALEDIR));
+	//TODO(code review): I don't think I change the behaviour here, but this doesn't seem to take into account whether
+	//the constant we use as base is relative or absolute. Maybe it doesn't need it?
+	//Though, this is the second time I concatenate datadir/locale so I wonder if that should be in some constant
+	//so I only have to do it once.
+	//Questions:
+	//Now that I've had time to think about this, shouldn't we be able to guarantee localedir path has already been set
+	//when we bootstrapped in wlapplication.cc?
+	//Though, it's not clear to me what the purpose of the section is, get_string seem to wrap a keybased
+	//lookup with a fallback value. Under the hood it ends up looking up the key in what looked very much like a map
+	//except it marked whether the value had been looked at (why?), and had an implementation instead of just
+	//using a map (again, why?).
+
+	const std::string directory = std::string(INSTALL_DATADIR) + "/locale";
+	FilenameSet files = g_fs->list_directory(
+		s->get_string(
+			"localedir",
+			//TODO(code review): I'd rather avoid this workaround, but it worked at the moment.
+			directory.c_str()
+		)
+	);
 	Profile ln("txts/languages");
 	s = &ln.pull_section("languages");
 	bool own_selected = "" == language || "en" == language;

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2014-10-16 05:54:05 +0000
+++ src/wlapplication.cc	2014-10-25 17:16:34 +0000
@@ -254,12 +254,7 @@
 	init_settings();
 
 	if (m_use_default_datadir) {
-		std::string install_prefix = INSTALL_PREFIX;
-		if (!PATHS_ARE_ABSOLUTE) {
-			install_prefix = relative_to_executable_to_absolute(install_prefix);
-		}
-		const std::string default_datadir =
-		   install_prefix + "/" + std::string(INSTALL_DATADIR);
+		const std::string default_datadir = std::string(INSTALL_DATADIR);
 		log("Adding directory: %s\n", default_datadir.c_str());
 		g_fs->add_file_system(&FileSystem::create(default_datadir));
 	}
@@ -770,11 +765,13 @@
 	i18n::init_locale();
 
 	if (s.has_val("localedir")) {
+		//TODO(code review): Should this still be possible, or do we want to tie it with
+		//datadir so that people using --datadir will also load locales from that?
 		// Localedir has been specified on the command line or in the config file.
 		i18n::set_localedir(s.get_safe_string("localedir"));
 	} else {
 		// Use default localedir, as configured at compile time.
-		std::string localedir = INSTALL_LOCALEDIR;
+		std::string localedir = std::string(INSTALL_DATADIR) + "/locale";
 		if (!PATHS_ARE_ABSOLUTE) {
 			localedir = relative_to_executable_to_absolute(localedir);
 		}