widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/windows-logging into lp:widelands.

Commit message:
Write Windows log output to homedir instead of the program dir.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1398536 in widelands: "Logging on windows"
  https://bugs.launchpad.net/widelands/+bug/1398536
  Bug #1630110 in widelands: "Widelands crashes on startup when installed into C:\Programs"
  https://bugs.launchpad.net/widelands/+bug/1630110

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/windows-logging/+merge/354397
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/windows-logging into lp:widelands.

=== modified file 'src/CMakeLists.txt'
--- src/CMakeLists.txt	2018-07-08 10:05:27 +0000
+++ src/CMakeLists.txt	2018-09-06 14:23:15 +0000
@@ -45,7 +45,6 @@
       ${WIN32_ICON_O}
     USES_SDL2
     DEPENDS
-      base_log
       base_exceptions
       widelands_ball_of_mud
       build_info
@@ -56,7 +55,6 @@
       main.cc
     USES_SDL2
     DEPENDS
-      base_log
       base_exceptions
       widelands_ball_of_mud
       build_info

=== modified file 'src/base/CMakeLists.txt'
--- src/base/CMakeLists.txt	2017-03-04 12:37:17 +0000
+++ src/base/CMakeLists.txt	2018-09-06 14:23:15 +0000
@@ -12,6 +12,7 @@
   DEPENDS
     base_macros
     base_exceptions
+    build_info
 )
 
 wl_library(base_exceptions

=== modified file 'src/base/log.cc'
--- src/base/log.cc	2018-04-07 16:59:00 +0000
+++ src/base/log.cc	2018-09-06 14:23:15 +0000
@@ -19,10 +19,12 @@
 
 #include "base/log.h"
 
+#include <cassert>
 #include <cstdarg>
 #include <cstdio>
 #include <fstream>
 #include <iostream>
+#include <memory>
 
 #include <SDL.h>
 #ifdef _WIN32
@@ -31,6 +33,9 @@
 
 #include "base/macros.h"
 #include "base/wexception.h"
+#ifdef _WIN32
+#include "build_info.h"
+#endif
 
 namespace {
 
@@ -38,7 +43,6 @@
 void sdl_logging_func(void* userdata, int, SDL_LogPriority, const char* message);
 
 #ifdef _WIN32
-
 std::string get_output_directory() {
 // This took inspiration from SDL 1.2 logger code.
 #ifdef _WIN32_WCE
@@ -57,12 +61,17 @@
 // This Logger emulates the SDL1.2 behavior of writing a stdout.txt.
 class WindowsLogger {
 public:
-	WindowsLogger() : stdout_filename_(get_output_directory() + "\\stdout.txt") {
+	WindowsLogger(const std::string& dir) : stdout_filename_(dir + "\\stdout.txt") {
 		stdout_.open(stdout_filename_);
 		if (!stdout_.good()) {
-			throw wexception("Unable to initialize logging to stdout.txt");
+			throw wexception("Unable to initialize stdout logging destination: %s", stdout_filename_.c_str());
 		}
 		SDL_LogSetOutputFunction(sdl_logging_func, this);
+		std::cout << "Log output will be written to: " << stdout_filename_ << std::endl;
+
+		// Repeat version info so that we'll have it available in the log file too
+		stdout_ << "This is Widelands Version " << build_id() << " (" << build_type() << ")" << std::endl;
+		stdout_.flush();
 	}
 
 	void log_cstring(const char* buffer) {
@@ -107,23 +116,42 @@
 	static_cast<Logger*>(userdata)->log_cstring(message);
 }
 #endif
-
 }  // namespace
 
 // Default to stdout for logging.
 bool g_verbose = false;
 
+#ifdef _WIN32
+// Start with nullptr so that we won't initialize an empty file in the logging directory
+std::unique_ptr<WindowsLogger> logger(nullptr);
+
+// Set the logging dir to the given homedir
+bool set_logging_dir(const std::string& homedir) {
+	try {
+		logger.reset(new WindowsLogger(homedir));
+	} catch (const std::exception& e) {
+		std::cout << e.what() << std::endl;
+		return false;
+	}
+	return true;
+}
+
+// Set the logging dir to the program's dir. For running test cases where we don't have a homedir.
+void set_logging_dir() {
+	logger.reset(new WindowsLogger(get_output_directory()));
+}
+
+#else
+std::unique_ptr<Logger> logger(new Logger());
+#endif
+
 void log(const char* const fmt, ...) {
-#ifdef _WIN32
-	static WindowsLogger logger;
-#else
-	static Logger logger;
-#endif
+	assert(logger != nullptr);
+
 	char buffer[2048];
 	va_list va;
-
 	va_start(va, fmt);
 	vsnprintf(buffer, sizeof(buffer), fmt, va);
 	va_end(va);
-	logger.log_cstring(buffer);
+	logger->log_cstring(buffer);
 }

=== modified file 'src/base/log.h'
--- src/base/log.h	2018-04-07 16:59:00 +0000
+++ src/base/log.h	2018-09-06 14:23:15 +0000
@@ -28,4 +28,13 @@
 
 extern bool g_verbose;
 
+#ifdef _WIN32
+/** Set the directory that stdout.txt shall be written to.
+ *  This should be the same dir where widelands writes its config file. Returns true on success.
+ */
+bool set_logging_dir(const std::string& homedir);
+// Set the directory that stdout.txt shall be written to to the directory the program is started from. Use this only for test cases.
+void set_logging_dir();
+#endif
+
 #endif  // end of include guard: WL_BASE_LOG_H

=== modified file 'src/economy/test/CMakeLists.txt'
--- src/economy/test/CMakeLists.txt	2017-06-20 12:38:50 +0000
+++ src/economy/test/CMakeLists.txt	2018-09-06 14:23:15 +0000
@@ -4,6 +4,7 @@
     test_road.cc
     test_routing.cc
   DEPENDS
+    base_log
     base_macros
     economy
     io_filesystem

=== modified file 'src/economy/test/test_road.cc'
--- src/economy/test/test_road.cc	2018-04-07 16:59:00 +0000
+++ src/economy/test/test_road.cc	2018-09-06 14:23:15 +0000
@@ -21,6 +21,9 @@
 
 #include <boost/test/unit_test.hpp>
 
+#ifdef _WIN32
+#include "base/log.h"
+#endif
 #include "economy/flag.h"
 #include "economy/road.h"
 #include "io/filesystem/layered_filesystem.h"
@@ -51,6 +54,9 @@
 /*************************************************************************/
 struct WlTestFixture {
 	WlTestFixture() {
+#ifdef _WIN32
+		set_logging_dir();
+#endif
 		g_fs = new LayeredFileSystem();
 	}
 	~WlTestFixture() {

=== modified file 'src/main.cc'
--- src/main.cc	2018-04-07 16:59:00 +0000
+++ src/main.cc	2018-09-06 14:23:15 +0000
@@ -24,23 +24,17 @@
 #include <SDL_main.h>
 #include <unistd.h>
 
-#include "base/log.h"
 #include "base/wexception.h"
 #include "build_info.h"
 #include "config.h"
 #include "wlapplication.h"
 #include "wlapplication_messages.h"
 
-using std::cout;
-using std::cerr;
-using std::endl;
-using std::flush;
-
 /**
  * Cross-platform entry point for SDL applications.
  */
 int main(int argc, char* argv[]) {
-	log("This is Widelands Version %s (%s)\n", build_id().c_str(), build_type().c_str());
+	std::cout << "This is Widelands Version " << build_id() << " (" << build_type() << ")" << std::endl;
 
 	WLApplication* g_app = nullptr;
 	try {
@@ -53,7 +47,7 @@
 		return 0;
 	} catch (const ParameterError& e) {
 		//  handle wrong commandline parameters
-		cerr << endl << e.what() << endl << endl;
+		std::cerr << std::endl << e.what() << std::endl << std::endl;
 		show_usage(build_id(), build_type());
 		delete g_app;
 
@@ -61,20 +55,20 @@
 	}
 #ifdef NDEBUG
 	catch (const WException& e) {
-		cerr << "\nCaught exception (of type '" << typeid(e).name()
+		std::cerr << "\nCaught exception (of type '" << typeid(e).name()
 		     << "') in outermost handler!\nThe exception said: " << e.what()
 		     << "\n\nThis should not happen. Please file a bug report on version " << build_id()
 		     << '(' << build_type() << ')' << ".\n"
-		     << "and remember to specify your operating system.\n\n" << flush;
+		     << "and remember to specify your operating system.\n\n" << std::flush;
 		delete g_app;
 
 		return 1;
 	} catch (const std::exception& e) {
-		cerr << "\nCaught exception (of type '" << typeid(e).name()
+		std::cerr << "\nCaught exception (of type '" << typeid(e).name()
 		     << "') in outermost handler!\nThe exception said: " << e.what()
 		     << "\n\nThis should not happen. Please file a bug report on version " << build_id()
 		     << '(' << build_type() << ')' << ".\n"
-		     << "and remember to specify your operating system.\n\n" << flush;
+		     << "and remember to specify your operating system.\n\n" << std::flush;
 		delete g_app;
 
 		return 1;

=== modified file 'src/notifications/test/CMakeLists.txt'
--- src/notifications/test/CMakeLists.txt	2017-06-20 12:38:50 +0000
+++ src/notifications/test/CMakeLists.txt	2018-09-06 14:23:15 +0000
@@ -2,6 +2,7 @@
   SRCS
     notifications_test.cc
   DEPENDS
+    base_log
     base_macros
     notifications
 )

=== modified file 'src/notifications/test/notifications_test.cc'
--- src/notifications/test/notifications_test.cc	2018-04-07 16:59:00 +0000
+++ src/notifications/test/notifications_test.cc	2018-09-06 14:23:15 +0000
@@ -23,6 +23,7 @@
 #define BOOST_TEST_MODULE Notifications
 #include <boost/test/unit_test.hpp>
 
+#include "base/log.h"
 #include "base/macros.h"
 #include "notifications/notifications.h"
 
@@ -41,6 +42,10 @@
 BOOST_AUTO_TEST_SUITE(NotificationsTestSuite)
 
 BOOST_AUTO_TEST_CASE(SimpleTest) {
+#ifdef _WIN32
+	set_logging_dir();
+#endif
+
 	std::vector<SimpleNote> received1;
 	auto subscriber1 = Notifications::subscribe<SimpleNote>(
 	   [&received1](const SimpleNote& got) { received1.push_back(got); });

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2018-07-29 11:27:33 +0000
+++ src/wlapplication.cc	2018-09-06 14:23:15 +0000
@@ -223,21 +223,32 @@
 
 }  // namespace
 
+
+// Set up the homedir. Exit 1 if the homedir is illegal or the logger couldn't be initialized for Windows.
 void WLApplication::setup_homedir() {
-	// If we don't have a home directory don't do anything
-	if (homedir_.size()) {
-		// Assume some dir exists
+	// If we don't have a home directory, we exit with an error
+	if (homedir_.empty()) {
+		std::cout << "Unable to start Widelands, because the given homedir is empty" << std::endl;
+		delete g_fs;
+		exit(1);
+	} else {
 		try {
-			log("Set home directory: %s\n", homedir_.c_str());
-
 			std::unique_ptr<FileSystem> home(new RealFSImpl(homedir_));
 			home->ensure_directory_exists(".");
 			g_fs->set_home_file_system(home.release());
 		} catch (const std::exception& e) {
-			log("Failed to add home directory: %s\n", e.what());
-		}
-	} else {
-		// TODO(unknown): complain
+			std::cout << "Unable to start Widelands, because we were unable to add the home directory: " << e.what() << std::endl;
+			delete g_fs;
+			exit(1);
+		}
+#ifdef _WIN32
+		// Initialize the logger for Windows. Exit on failure.
+		if (!set_logging_dir(homedir_)) {
+			delete g_fs;
+			exit(1);
+		}
+#endif
+		log("Set home directory: %s\n", homedir_.c_str());
 	}
 }