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);
}
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
