Fix bug #8537: LyX creates the environment variable LC_ALL

The current code is not able to unset an environment variable, only to set it to an empty value. This patch refactors a bit the Message class and uses a new EnvChanger helper class that allows to change temporarily an environment variable and that is able to unset variables if needed.

The patch also adds new functions hasEnv and unsetEnv in environment.cpp.

Open issues:
 * there may be systems where unsetenv is not available and putenv("name=") does not do the right thing;
 * unsetenv may lead to leaks on some platforms.
 * when using unsetenv, we may need to remove strings from the internal map that setEnv uses.

configure.ac

@@ -186,7 +186,7 @@ AC_TYPE_UID_T
 
 LYX_CHECK_DEF(PATH_MAX, limits.h, [int n = PATH_MAX;])
 
-AC_CHECK_FUNCS(chmod close _close fork getpid _getpid lstat mkfifo open _open pclose _pclose popen _popen readlink strerror)
+AC_CHECK_FUNCS(chmod close _close fork getpid _getpid lstat mkfifo open _open pclose _pclose popen _popen readlink strerror unsetenv)
 # Check the form of mkdir()
 AC_FUNC_MKDIR
 AC_FUNC_SELECT_ARGTYPES

development/cmake/ConfigureChecks.cmake

@@ -56,6 +56,7 @@ check_function_exists(_getpid HAVE__GETPID)
 check_function_exists(mkdir  HAVE_MKDIR)
 check_function_exists(_mkdir HAVE__MKDIR)
 check_function_exists(setenv HAVE_SETENV)
+check_function_exists(unsetenv HAVE_UNSETENV)
 check_function_exists(putenv HAVE_PUTENV)
 check_function_exists(fcntl HAVE_FCNTL)
 check_function_exists(strerror HAVE_STRERROR)

src/support/Messages.cpp

@@ -133,42 +133,11 @@ bool Messages::available(string const & c)
 
 }
 
+namespace {
 
-docstring const Messages::get(string const & m) const
+// Trivial wrapper around gettext()
+docstring const getText(string const & m)
 {
-	if (m.empty())
-		return docstring();
-
-	// Look for the translated string in the cache.
-	TranslationCache::iterator it = cache_.find(m);
-	if (it != cache_.end())
-		return it->second;
-
-	// The string was not found, use gettext to generate it
-	static string oldLC_ALL;
-	static string oldLANGUAGE;
-	if (!lang_.empty()) {
-		oldLC_ALL = getEnv("LC_ALL");
-		// This GNU extension overrides any language locale
-		// wrt gettext.
-		LYXERR(Debug::LOCALE, "Setting LANGUAGE to " << lang_);
-		oldLANGUAGE = getEnv("LANGUAGE");
-		if (!setEnv("LANGUAGE", lang_))
-			LYXERR(Debug::LOCALE, "\t... failed!");
-		// However, setting LANGUAGE does nothing when the
-		// locale is "C". Therefore we set the locale to
-		// something that is believed to exist on most
-		// systems. The idea is that one should be able to
-		// load German documents even without having de_DE
-		// installed.
-		LYXERR(Debug::LOCALE, "Setting LC_ALL to en_US");
-		if (!setEnv("LC_ALL", "en_US"))
-			LYXERR(Debug::LOCALE, "\t... failed!");
-#ifdef HAVE_LC_MESSAGES
-		setlocale(LC_MESSAGES, "");
-#endif
-	}
-
 	// FIXME: gettext sometimes "forgets" the ucs4_codeset we set
 	// in init(), which leads to severe message corruption (#7371)
 	// We set it again here unconditionally. A real fix must be found!
@@ -191,23 +160,50 @@ docstring const Messages::get(string const & m) const
 
 	cleanTranslation(trans);
 
-	// Reset environment variables as they were.
+	return trans;
+}
+
+}
+
+
+docstring const Messages::get(string const & m) const
+{
+	if (m.empty())
+		return docstring();
+
+	// Look for the translated string in the cache.
+	TranslationCache::iterator it = cache_.find(m);
+	if (it != cache_.end())
+		return it->second;
+
+	// The string was not found, use gettext to generate it
+	docstring trans;
 	if (!lang_.empty()) {
-		// Reset everything as it was.
-		LYXERR(Debug::LOCALE, "restoring LANGUAGE from " 
-		       << getEnv("LANGUAGE")
-		       << " to " << oldLANGUAGE);
-		if (!setEnv("LANGUAGE", oldLANGUAGE))
-			LYXERR(Debug::LOCALE, "\t... failed!");
-		LYXERR(Debug::LOCALE, "restoring LC_ALL from " << getEnv("LC_ALL")
-			<< " to " << oldLC_ALL);
-		if (!setEnv("LC_ALL", oldLC_ALL))
-			LYXERR(Debug::LOCALE, "\t... failed!");
+		// This GNU extension overrides any language locale
+		// wrt gettext.
+		LYXERR(Debug::LOCALE, "Setting LANGUAGE to " << lang_);
+		EnvChanger language_chg("LANGUAGE", lang_);
+		// However, setting LANGUAGE does nothing when the
+		// locale is "C". Therefore we set the locale to
+		// something that is believed to exist on most
+		// systems. The idea is that one should be able to
+		// load German documents even without having de_DE
+		// installed.
+		LYXERR(Debug::LOCALE, "Setting LC_ALL to en_US");
+		EnvChanger lc_all_chg("LC_ALL", "en_US");
 #ifdef HAVE_LC_MESSAGES
 		setlocale(LC_MESSAGES, "");
 #endif
-	}
+		trans = getText(m);
+	} else
+		trans = getText(m);
+		
 
+#ifdef HAVE_LC_MESSAGES
+	setlocale(LC_MESSAGES, "");
+#endif
+
+	// store translation in cache
 	pair<TranslationCache::iterator, bool> result =
 		cache_.insert(make_pair(m, trans));
 

src/support/environment.cpp

@@ -28,10 +28,17 @@ using namespace std;
 namespace lyx {
 namespace support {
 
-string const getEnv(string const & envname)
+
+bool hasEnv(string const & name)
+{
+	return getenv(name.c_str());
+}
+
+
+string const getEnv(string const & name)
 {
 	// f.ex. what about error checking?
-	char const * const ch = getenv(envname.c_str());
+	char const * const ch = getenv(name.c_str());
 	return ch ? to_utf8(from_local8bit(ch)) : string();
 }
 
@@ -117,5 +124,20 @@ void prependEnvPath(string const & name, string const & prefix)
 	setEnvPath(name, env_var);
 }
 
+
+bool unsetEnv(string const & name)
+{
+#if defined(HAVE_UNSETENV)
+	// FIXME: does it leak?
+	return unsetenv(name.c_str()) == 0;
+#elif defined(HAVE_PUTENV)
+	// This is OK with MSVC and MinGW at least.
+	return putenv((name + "=").c_str()) == 0;
+#else
+#error No environment-unsetting function has been defined.
+#endif
+}
+
+
 } // namespace support
 } // namespace lyx

src/support/environment.h

@@ -18,8 +18,11 @@
 namespace lyx {
 namespace support {
 
+/// @returns true if the environment variable @c name exists.
+bool hasEnv(std::string const & name);
+
 /// @returns the contents of the environment variable @c name encoded in utf8.
-std::string const getEnv(std::string const & envname);
+std::string const getEnv(std::string const & name);
 
 /** @returns the contents of the environment variable @c name,
  *  split into path elements using the OS-dependent separator token
@@ -54,6 +57,42 @@ void setEnvPath(std::string const & name, std::vector<std::string> const & env);
  */
 void prependEnvPath(std::string const & name, std::string const & prefix);
 
+/** Remove the variable @c name from the environment.
+ *  @returns true if the variable was unset successfully.
+ */
+bool unsetEnv(std::string const & name);
+
+
+/** Utility class to change temporarily an environment variable. The
+ * variable is reset to its original state when the dummy EnvChanger
+ * variable is deleted.
+ */
+class EnvChanger {
+public:
+	///
+	EnvChanger(std::string const & name, std::string const & value)
+	: name_(name), set_(hasEnv(name)), value_(getEnv(name))
+	{
+			setEnv(name, value);
+	}
+	///
+	~EnvChanger()
+	{
+		if (set_)
+			setEnv(name_, value_);
+		else
+			unsetEnv(name_);
+	}
+
+private:
+	/// the name of the variable
+	std::string name_;
+	/// was the variable set?
+	bool set_;
+	///
+	std::string value_;
+};
+
 } // namespace support
 } // namespace lyx
 

status.20x

@@ -35,6 +35,7 @@ What's new
 * TEX2LYX IMPROVEMENTS
 
 - support for listings with options (bug #8066).
+
 - add new option -m to select needed modules (bug #8393).
 
 
@@ -131,6 +132,8 @@ What's new
 
 - File format viewer & editor combo boxes are correctly initialized (bug 8237).
 
+-  Do not create an empty environment variable LC_ALL when launching
+   external processes (bug 8537).
 
 * DOCUMENTATION AND LOCALIZATION