widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

SirVer has proposed merging lp:~widelands-dev/widelands/syncstream_on_option into lp:widelands.

Commit message:
- Remove --remove-replays and --remove-syncstreams. We now always delete them if they were autogenerated and older than 4 weeks.
- Adds --write-syncstreams option which defaults to true for now. This will give us more debug information for future desyncs.


Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1545098 in widelands: "Desync in bzr7818[trunk] "
  https://bugs.launchpad.net/widelands/+bug/1545098

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/syncstream_on_option/+merge/286004

Now that I finished this debug aid, I think I have another idea why my last lockstep change was not correct.... Still, this get's rid of two useless options and adds a more useful one (though I'd argue that option should probably never be set to false anyways, so maybe we could just remove it).
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/syncstream_on_option into lp:widelands.

=== modified file 'src/network/netclient.cc'
--- src/network/netclient.cc	2016-02-11 06:56:47 +0000
+++ src/network/netclient.cc	2016-02-14 21:20:55 +0000
@@ -168,9 +168,7 @@
 	d->server_is_waiting = true;
 
 	Widelands::Game game;
-#ifndef NDEBUG
-	game.set_write_syncstream(true);
-#endif
+	game.set_write_syncstream(g_options.pull_section("global").get_bool("write_syncstreams", true));
 
 	try {
 		UI::ProgressWindow* loader_ui = new UI::ProgressWindow("images/loadscreens/progress.png");

=== modified file 'src/network/nethost.cc'
--- src/network/nethost.cc	2016-02-07 07:41:45 +0000
+++ src/network/nethost.cc	2016-02-14 21:20:55 +0000
@@ -709,9 +709,7 @@
 	broadcast(s);
 
 	Widelands::Game game;
-#ifndef NDEBUG
-	game.set_write_syncstream(true);
-#endif
+	game.set_write_syncstream(g_options.pull_section("global").get_bool("write_syncstreams", true));
 
 	try {
 		std::unique_ptr<UI::ProgressWindow> loader_ui;

=== modified file 'src/ui_fsmenu/options.cc'
--- src/ui_fsmenu/options.cc	2016-02-07 16:31:06 +0000
+++ src/ui_fsmenu/options.cc	2016-02-14 21:20:55 +0000
@@ -202,17 +202,9 @@
 		 "",
 		 g_gr->images().get("images/ui_basic/but3.png"), UI::SpinBox::Type::kBig),
 
-	sb_remove_replays_
-		(&box_saving_, 0, 0, column_width_, 250,
-		 opt.remove_replays, 0, 365, _("Remove replays older than:"),
-		 /** TRANSLATORS: Options: Remove Replays older than: */
-		 /** TRANSLATORS: This will have a number added in front of it */
-		 ngettext("day", "days", opt.remove_replays),
-		 g_gr->images().get("images/ui_basic/but3.png"), UI::SpinBox::Type::kBig),
-
 	nozip_(&box_saving_, Point(0, 0), _("Do not zip widelands data files (maps, replays and savegames)"),
 			 "", column_width_),
-	remove_syncstreams_(&box_saving_, Point(0, 0), _("Remove Syncstream dumps on startup"),
+	write_syncstreams_(&box_saving_, Point(0, 0), _("Write syncstreams in network games to debug desyncs."),
 							  "", column_width_),
 
 	// Game options
@@ -278,9 +270,8 @@
 	// Saving
 	box_saving_.add(&sb_autosave_, UI::Align::kLeft);
 	box_saving_.add(&sb_rolling_autosave_, UI::Align::kLeft);
-	box_saving_.add(&sb_remove_replays_, UI::Align::kLeft);
 	box_saving_.add(&nozip_, UI::Align::kLeft);
-	box_saving_.add(&remove_syncstreams_, UI::Align::kLeft);
+	box_saving_.add(&write_syncstreams_, UI::Align::kLeft);
 
 	// Game
 	box_game_.add(&auto_roadbuild_mode_, UI::Align::kLeft);
@@ -306,14 +297,6 @@
 					(&FullscreenMenuOptions::update_sb_autosave_unit,
 					 boost::ref(*this)));
 	}
-	/** TRANSLATORS Options: Remove Replays older than: */
-	sb_remove_replays_.add_replacement(0, _("Never"));
-	for (UI::Button* temp_button : sb_remove_replays_.get_buttons()) {
-		temp_button->sigclicked.connect
-				(boost::bind
-					(&FullscreenMenuOptions::update_sb_remove_replays_unit,
-					 boost::ref(*this)));
-	}
 	for (UI::Button* temp_button : sb_dis_panel_.get_buttons()) {
 		temp_button->sigclicked.connect
 				(boost::bind
@@ -386,7 +369,7 @@
 
 	// Saving options
 	nozip_                .set_state(opt.nozip);
-	remove_syncstreams_   .set_state(opt.remove_syncstreams);
+	write_syncstreams_   .set_state(opt.write_syncstreams);
 
 	// Game options
 	auto_roadbuild_mode_  .set_state(opt.auto_roadbuild_mode);
@@ -403,10 +386,6 @@
 	sb_autosave_.set_unit(ngettext("minute", "minutes", sb_autosave_.get_value()));
 }
 
-void FullscreenMenuOptions::update_sb_remove_replays_unit() {
-	sb_remove_replays_.set_unit(ngettext("day", "days", sb_remove_replays_.get_value()));
-}
-
 void FullscreenMenuOptions::update_sb_dis_panel_unit() {
 	sb_dis_panel_.set_unit(ngettext("pixel", "pixels", sb_dis_panel_.get_value()));
 }
@@ -505,9 +484,8 @@
 	// Saving options
 	os_.autosave              = sb_autosave_.get_value();
 	os_.rolling_autosave      = sb_rolling_autosave_.get_value();
-	os_.remove_replays        = sb_remove_replays_.get_value();
 	os_.nozip                 = nozip_.get_state();
-	os_.remove_syncstreams    = remove_syncstreams_.get_state();
+	os_.write_syncstreams = write_syncstreams_.get_state();
 
 	// Game options
 	os_.auto_roadbuild_mode   = auto_roadbuild_mode_.get_state();
@@ -574,9 +552,8 @@
 	// Saving options
 	opt.autosave = opt_section_.get_int("autosave", DEFAULT_AUTOSAVE_INTERVAL * 60);
 	opt.rolling_autosave = opt_section_.get_int("rolling_autosave", 5);
-	opt.remove_replays = opt_section_.get_int("remove_replays", 0);
 	opt.nozip = opt_section_.get_bool("nozip", false);
-	opt.remove_syncstreams = opt_section_.get_bool("remove_syncstreams", true);
+	opt.write_syncstreams = opt_section_.get_bool("write_syncstreams", true);
 
 	// Game options
 	opt.auto_roadbuild_mode = opt_section_.get_bool("auto_roadbuild_mode", true);
@@ -618,9 +595,8 @@
 	// Saving options
 	opt_section_.set_int("autosave",               opt.autosave * 60);
 	opt_section_.set_int("rolling_autosave",       opt.rolling_autosave);
-	opt_section_.set_int("remove_replays",         opt.remove_replays);
 	opt_section_.set_bool("nozip",                 opt.nozip);
-	opt_section_.set_bool("remove_syncstreams",    opt.remove_syncstreams);
+	opt_section_.set_bool("write_syncstreams",    opt.write_syncstreams);
 
 	// Game options
 	opt_section_.set_bool("auto_roadbuild_mode",   opt.auto_roadbuild_mode);

=== modified file 'src/ui_fsmenu/options.h'
--- src/ui_fsmenu/options.h	2016-01-02 22:07:47 +0000
+++ src/ui_fsmenu/options.h	2016-02-14 21:20:55 +0000
@@ -63,9 +63,8 @@
 		// Saving options
 		int32_t autosave; // autosave interval in minutes
 		int32_t rolling_autosave; // number of file to use for rolling autosave
-		uint32_t remove_replays;
 		bool nozip;
-		bool remove_syncstreams;
+		bool write_syncstreams;
 
 		// Game options
 		bool auto_roadbuild_mode;
@@ -100,7 +99,6 @@
 
 private:
 	void update_sb_autosave_unit();
-	void update_sb_remove_replays_unit();
 	void update_sb_dis_panel_unit();
 	void update_sb_dis_border_unit();
 
@@ -151,9 +149,8 @@
 	// Saving options
 	UI::SpinBox                 sb_autosave_;
 	UI::SpinBox                 sb_rolling_autosave_;
-	UI::SpinBox                 sb_remove_replays_;
 	UI::Checkbox                nozip_;
-	UI::Checkbox                remove_syncstreams_;
+	UI::Checkbox                write_syncstreams_;
 
 	// Game options
 	UI::Checkbox                auto_roadbuild_mode_;

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2016-02-07 07:16:24 +0000
+++ src/wlapplication.cc	2016-02-14 21:20:55 +0000
@@ -97,6 +97,10 @@
 
 namespace {
 
+// The time in seconds for how long old replays/syncstreams should be kept
+// around, in seconds. Right now this is 4 weeks.
+constexpr double kReplayKeepAroundTime = 4 * 7 * 24 * 60 * 60;
+
 /**
  * Shut the hardware down: stop graphics mode, stop sound handler
  */
@@ -174,6 +178,49 @@
 	chdir(get_executable_directory().c_str());
 #endif
 }
+
+// Extracts a long from 'text' into 'val' returning true if all of the string
+// was valid. If not, the content of 'val' is undefined.
+bool to_long(const std::string& text, long* val) {
+	const char* start = text.c_str();
+	char* end;
+	*val = strtol(start, &end, 10);
+	return *end == '\0';
+}
+
+// Extracts the creation date from 'path' which is expected to
+// match "YYYY-MM-DD*". Returns false if no date could be extracted.
+bool extract_creation_day(const std::string& path, tm* tfile) {
+	const std::string filename = FileSystem::fs_filename(path.c_str());
+	memset(tfile, 0, sizeof(tm));
+
+	long day, month, year;
+	if (!to_long(filename.substr(8, 2), &day)) {
+		return false;
+	}
+	if (!to_long(filename.substr(5, 2), &month)) {
+		return false;
+	}
+	if (!to_long(filename.substr(0, 4), &year)) {
+		return false;
+	}
+
+	tfile->tm_mday = day;
+	tfile->tm_mon = month - 1;
+	tfile->tm_year = year - 1900;
+	return tfile;
+}
+
+// Returns true if 'filename' was autogenerated, i.e. if 'extract_creation_day' can return a date
+// and it is old enough to be deleted.
+bool is_autogenerated_and_expired(const std::string& filename) {
+	tm tfile;
+	if (!extract_creation_day(filename, &tfile)) {
+		return false;
+	}
+	return std::difftime(time(nullptr), mktime(&tfile)) > kReplayKeepAroundTime;
+}
+
 }  // namespace
 
 void WLApplication::setup_homedir() {
@@ -676,14 +723,13 @@
 	s.get_int("panel_snap_distance");
 	s.get_int("autosave");
 	s.get_int("rolling_autosave");
-	s.get_int("remove_replays");
 	s.get_bool("single_watchwin");
 	s.get_bool("auto_roadbuild_mode");
 	s.get_bool("workareapreview");
 	s.get_bool("nozip");
 	s.get_bool("snap_windows_only_when_overlapping");
 	s.get_bool("dock_windows_to_edges");
-	s.get_bool("remove_syncstreams");
+	s.get_bool("write_syncstreams");
 	s.get_bool("sound_at_message");
 	s.get_bool("transparent_chat");
 	s.get_string("registered");
@@ -1379,58 +1425,22 @@
  */
 void WLApplication::cleanup_replays()
 {
-	FilenameSet files;
-
-	Section & s = g_options.pull_section("global");
-
-	if (s.get_bool("remove_syncstreams", true)) {
-		files =
-			filter(g_fs->list_directory(REPLAY_DIR),
-		          [](const std::string& fn) {return boost::ends_with(fn, REPLAY_SUFFIX ".wss");});
-
-		for
-			(FilenameSet::iterator filename = files.begin();
-			 filename != files.end();
-			 ++filename)
-		{
-			log("Delete syncstream %s\n", filename->c_str());
-			g_fs->fs_unlink(*filename);
+	for (const std::string& filename :
+	     filter(g_fs->list_directory(REPLAY_DIR),
+	            [](const std::string& fn) { return boost::ends_with(fn, REPLAY_SUFFIX ".wss"); })) {
+		if (is_autogenerated_and_expired(filename)) {
+			log("Delete syncstream %s\n", filename.c_str());
+			g_fs->fs_unlink(filename);
 		}
 	}
 
-	time_t tnow = time(nullptr);
-
-	if (s.get_int("remove_replays", 0)) {
-		files =
-			filter(g_fs->list_directory(REPLAY_DIR),
-		          [](const std::string& fn) {return boost::ends_with(fn, REPLAY_SUFFIX);});
-
-		for
-			(FilenameSet::iterator filename = files.begin();
-			 filename != files.end();
-			 ++filename)
-		{
-			std::string file = g_fs->fs_filename(filename->c_str());
-			std::string timestr = file.substr(0, file.find(' '));
-
-			if (19 != timestr.size())
-				continue;
-
-			tm tfile;
-			memset(&tfile, 0, sizeof(tm));
-
-			tfile.tm_mday = atoi(timestr.substr(8, 2).c_str());
-			tfile.tm_mon  = atoi(timestr.substr(5, 2).c_str()) - 1;
-			tfile.tm_year = atoi(timestr.substr(0, 4).c_str()) - 1900;
-
-			double tdiff = std::difftime(tnow, mktime(&tfile)) / 86400;
-
-			if (tdiff > s.get_int("remove_replays")) {
-				log("Delete replay %s\n", file.c_str());
-
-				g_fs->fs_unlink(*filename);
-				g_fs->fs_unlink(*filename + ".wgf");
-			}
+	for (const std::string& filename :
+	     filter(g_fs->list_directory(REPLAY_DIR),
+	            [](const std::string& fn) { return boost::ends_with(fn, REPLAY_SUFFIX); })) {
+		if (is_autogenerated_and_expired(filename)) {
+			log("Deleting replay %s\n", filename.c_str());
+			g_fs->fs_unlink(filename);
+			g_fs->fs_unlink(filename + ".wgf");
 		}
 	}
 }

=== modified file 'src/wlapplication_messages.cc'
--- src/wlapplication_messages.cc	2016-02-06 11:11:24 +0000
+++ src/wlapplication_messages.cc	2016-02-14 21:20:55 +0000
@@ -63,11 +63,8 @@
 			  "                      The locale to use.") << 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"
-			  "                      Remove syncstream files on startup") << endl
-		<< _(" --remove_replays=[...]\n"
-			  "                      Remove replays after this number of days.\n"
-			  "                      If this is 0, replays are not deleted.") << endl
+		<< _(" --write_syncstreams=[true|false]\n"
+			  "                      Create syncstreams to help debug network games.") << endl
 		<< _(" --autosave=[...]     Automatically save each n minutes") << endl
 		<< _(" --rolling_autosave=[...]\n"
 			  "                      Use this many files for rolling autosaves") << endl << endl