widelands-dev team mailing list archive

[Hans Joachim Desserud <launchpad@xxxxxxxxxxxx>]

Not had much time to work on this, but I've just removed the localedir option from the commandline help and usage. Rather untested but should take care of the most parts. Not sure whether the part in options should be updated alongside this. (See also the comment around those parts)

I think that's the only remaining issue, otherwise this should be good to go. (The corresponding packaging branch has already been approved)

Diff comments:

> === modified file 'CMakeLists.txt'
> --- CMakeLists.txt	2014-10-17 14:26:08 +0000
> +++ CMakeLists.txt	2014-11-03 19:51:55 +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-10-13 15:04:50 +0000
> +++ cmake/WlFunctions.cmake	2014-11-03 19:51:55 +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-11-03 19:51:55 +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-10-11 13:26:59 +0000
> +++ src/logic/map_info.cc	2014-11-03 19:51:55 +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-10-28 14:57:52 +0000
> +++ src/ui_fsmenu/options.cc	2014-11-03 19:51:55 +0000
> @@ -54,7 +54,37 @@
>  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?).
> +	// NOCOM(#codereview): this code only works when "localedir" is in one of
> +	// the directories that we added to the LayeredFileSystem that is g_fs. If
> +	// "localedir" is /usr/share/locale or something like that this code will
> +	// not work I think. This makes me wonder: does selecting different languages even work on the PPA or so, and if so, is
> +	// the localepath then inside the datadir? If the answer is yes, we should kill the localepath option.
> +	//
> +	// The further code works like this: It lists localedir/*, removes "." and
> +	// ".." and asks txts/languges for the long name of the code it saw in the
> +	// directory (i.e. de -> "Deutsch"). It adds the long codes to the list, but
> +	// remember the related code (i.e. "de") so that the languge can be
> +	// selected.
> +

To keep the conversation flowing:

Well, the further code is rather ok. I was thinking about inside s->get_string(). It looks very much like looking up in a map and then using the fallback parameter if nothing is found, but for some it's not quite like that...

> +	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-11-03 07:46:06 +0000
> +++ src/wlapplication.cc	2014-11-03 19:51:55 +0000
> @@ -252,12 +252,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));
>  	}
> @@ -783,17 +778,12 @@
>  	// Initialize locale and grab "widelands" textdomain
>  	i18n::init_locale();
>  
> -	if (s.has_val("localedir")) {
> -		// 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;
> -		if (!PATHS_ARE_ABSOLUTE) {
> -			localedir = relative_to_executable_to_absolute(localedir);
> -		}
> -		i18n::set_localedir(localedir);
> +	// Use default localedir, residing in INSTALL_DATADIR which was configured at compile time.
> +	std::string localedir = std::string(INSTALL_DATADIR) + "/locale";
> +	if (!PATHS_ARE_ABSOLUTE) {
> +		localedir = relative_to_executable_to_absolute(localedir);
>  	}
> +	i18n::set_localedir(localedir);
>  	i18n::grab_textdomain("widelands");
>  
>  	// Set locale corresponding to selected language
> 
> === modified file 'src/wlapplication_messages.cc'
> --- src/wlapplication_messages.cc	2014-10-16 09:10:54 +0000
> +++ src/wlapplication_messages.cc	2014-11-03 19:51:55 +0000
> @@ -61,7 +61,6 @@
>  			  "                      using the SDL") << endl
>  		<< _(" --language=[de_DE|sv_SE|...]\n"
>  			  "                      The locale to use.") << endl
> -		<< _(" --localedir=DIRNAME  Use DIRNAME as location for the locale") << endl
>  			/** TRANSLATORS: You may translate true/false, also as on/off or yes/no, but */
>  			/** TRANSLATORS: it HAS TO BE CONSISTENT with the translation in the widelands textdomain */
>  		<< _(" --remove_syncstreams=[true|false]\n"
> 


-- 
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.