Merge lp:~widelands-dev/widelands/bug-1509172-map-saving-paths into lp:widelands

[GunChleoc]

I hope that this will fix the map saving bug in Windows. I can reproduce the original bug, but I need the AppVeyor build to check if this fixes it.

[Tino (tino79)]

=> https://widelands.8-schuss.de/Widelands-bzr7830%5bbug-1509172-map-saving-paths%5d-nomusic-win64.exe

It does not work: Now ensure_dir_exists() throws an error.

review: Needs Fixing

[GunChleoc (gunchleoc)]

I can't download the linked build - do you have the error message?

[GunChleoc (gunchleoc)]

Nevermind, the link worked on the 2nd try.

[Tino (tino79)]

Probably i was still uploading...

I've done some debugging, but did not find a solution as of now:
- Click ok in save dialog
- The first string which arrives in ensure_dir_exists is "C:\data\wl_x64\src\maps" (my binary dir + maps)
- Then the drive letter "C:" get extracted
- Canonalize_name() makes "C:\\Users\\mit\\.widelands\\C:" from this.
- This seems to be done because is_path_absolute("C:") gives false
- This is because path_size < root_size => "C:" < "C:\\Users\\mit\\.widelands\\"

But why is a path relative just because it is shorter than your widelands home directory?

[GunChleoc (gunchleoc)]

In the error message I got, the path was missing all \. I need to add some more log output so we can get more information as to at what point the path gets messed up.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 709. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/109475085.
Appveyor build 558. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1509172_map_saving_paths-558.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 731. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/110103559.
Appveyor build 578. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1509172_map_saving_paths-578.

[Tino (tino79)]

Ok, it does work again on windows.
But i've really no clue what i've done with the last commit. No the file lock on the tmp save is gone and it can be renamed correctly.

If this does not break anything on linux i would remove the NOCOMs and merge.

review: Needs Resubmitting

[GunChleoc (gunchleoc)]

That broke it for Linus, but I have added an #ifndef that should fix it for everybody. I have tested with the zip option on and off. COuld you please test for Windows again to make sure that it's still working?

review: Approve

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

Running 'bzr pull --overwrite' failed. Output:

ssh_exchange_identification: Connection closed by remote host
ConnectionReset reading response for 'BzrDir.open_2.1', retrying
ssh_exchange_identification: Connection closed by remote host
bzr: ERROR: Connection closed: Unexpected end of message. Please check connectivity and permissions, and report a bug if problems persist.
Using saved parent location: bzr+ssh://bazaar.launchpad.net/~widelands-dev/widelands/bug-1509172-map-saving-paths/

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

Running 'bzr pull --overwrite' failed. Output:

ssh_exchange_identification: Connection closed by remote host
ConnectionReset reading response for 'BzrDir.open_2.1', retrying
ssh_exchange_identification: Connection closed by remote host
bzr: ERROR: Connection closed: Unexpected end of message. Please check connectivity and permissions, and report a bug if problems persist.
Using saved parent location: bzr+ssh://bazaar.launchpad.net/~widelands-dev/widelands/bug-1509172-map-saving-paths/

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 758. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/110807105.
Appveyor build 604. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1509172_map_saving_paths-604.

[GunChleoc (gunchleoc)]

This now works in Windows for the Zip Filesystem, but not for the normal file system.

review: Needs Fixing

[Tino (tino79)]

I would really like to avoid using different code pathes for windows and linux.
The current version does work for directory saves on windows.

But if fails on the second save for a zip save:
If you start the editor and save the map "abc" and "abc" already exists it gets overwritten correctly.
But you can't save again "abc" without restarting the editor (or save once under another name).

So the editor itself does put also a lock on the save file after the first save.

Why does creating a MapSaver object with new not work on linux?

[GunChleoc (gunchleoc)]

I'll test your changes :)

> Why does creating a MapSaver object with new not work on linux?

Why do we need this to be a pointer anyway? It only adds an extra line of code.

[GunChleoc (gunchleoc)]

Now I get it - it's because of file locks.

Everything works on Ubuntu now, so I am in favour of merging. Let's solve the problem with not being able to save twice in a new branch.

review: Approve

[GunChleoc (gunchleoc)]

> The current version does work for directory saves on windows.
>
> But if fails on the second save for a zip save:
> If you start the editor and save the map "abc" and "abc" already exists it
> gets overwritten correctly.
> But you can't save again "abc" without restarting the editor (or save once
> under another name).
>
> So the editor itself does put also a lock on the save file after the first
> save.
>

I can confirm this problem - I have created a new bug report so that we can merge this branch:

https://bugs.launchpad.net/widelands/+bug/1548932

[GunChleoc (gunchleoc)]

@bunnybot merge

=== modified file 'src/editor/ui_menus/editor_main_menu_save_map.cc'
--- src/editor/ui_menus/editor_main_menu_save_map.cc	2016-01-31 10:57:58 +0000
+++ src/editor/ui_menus/editor_main_menu_save_map.cc	2016-02-22 10:26:20 +0000
@@ -126,7 +126,7 @@
 	if (filename == "" && table_.has_selection()) {  //  Maybe a directory is selected.
 		complete_filename = filename = maps_data_[table_.get_selected()].filename;
 	} else {
-		complete_filename = curdir_ + "/" + filename;
+		complete_filename = curdir_ + g_fs->file_separator() + filename;
 	}
 
 	if (g_fs->is_directory(complete_filename.c_str()) &&
@@ -155,9 +155,7 @@
 	if (md.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kOk) {
 		g_fs->ensure_directory_exists(curdir_);
 		//  create directory
-		std::string fullname = curdir_;
-		fullname += "/";
-		fullname += md.get_dirname();
+		std::string fullname = curdir_ + g_fs->file_separator() + md.get_dirname();
 		g_fs->make_directory(fullname);
 		fill_table();
 	}
@@ -234,9 +232,7 @@
 		filename += WLMF_SUFFIX;
 
 	//  append directory name
-	std::string complete_filename = curdir_;
-	complete_filename += "/";
-	complete_filename += filename;
+	const std::string complete_filename = curdir_ + g_fs->file_separator() + filename;
 
 	//  Check if file exists. If so, show a warning.
 	if (g_fs->file_exists(complete_filename)) {
@@ -250,7 +246,7 @@
 
 	// save to a tmp file/dir first, rename later
 	// (important to keep script files in the script directory)
-	std::string tmp_name = complete_filename + ".tmp";
+	const std::string tmp_name = complete_filename + ".tmp";
 	if (g_fs->file_exists(tmp_name)) {
 		const std::string s = (boost::format(_
 				("A file with the name ‘%s.tmp’ already exists. You have to remove it manually."))
@@ -266,9 +262,7 @@
 
 	{ // fs scope
 		std::unique_ptr<FileSystem> fs
-			(g_fs->create_sub_file_system(tmp_name.empty() ? complete_filename : tmp_name,
-				binary ? FileSystem::ZIP : FileSystem::DIR));
-		Widelands::MapSaver wms(*fs, egbase);
+			(g_fs->create_sub_file_system(tmp_name, binary ? FileSystem::ZIP : FileSystem::DIR));
 
 		// Recompute seafaring tag
 		if (map.allows_seafaring()) {
@@ -284,17 +278,16 @@
 		}
 
 		try {
-			wms.save();
+			Widelands::MapSaver* wms = new Widelands::MapSaver(*fs, egbase);
+			wms->save();
+			delete wms;
+			//reset filesystem to avoid file locks on saves
+			fs.reset();
 			eia().set_need_save(false);
-
-			// if saved to a tmp file earlier, rename now
-			if (!tmp_name.empty()) {
-				g_fs->fs_unlink(complete_filename);
-				g_fs->fs_rename(tmp_name, complete_filename);
-				// also change fs, as we assign it to the map below
-				fs.reset(g_fs->make_sub_file_system(complete_filename));
-			}
-
+			g_fs->fs_unlink(complete_filename);
+			g_fs->fs_rename(tmp_name, complete_filename);
+			// also change fs, as we assign it to the map below
+			fs.reset(g_fs->make_sub_file_system(complete_filename));
 			// set the filesystem of the map to the current save file / directory
 			map.swap_filesystem(fs);
 			// DONT use fs as of here, its garbage now!
@@ -310,9 +303,7 @@
 			mbox.run<UI::Panel::Returncodes>();
 
 			// cleanup tmp file if it was created
-			if (!tmp_name.empty()) {
-				g_fs->fs_unlink(tmp_name);
-			}
+			g_fs->fs_unlink(tmp_name);
 		}
 	} // end fs scope, dont use it
 

=== modified file 'src/graphic/animation.cc'
--- src/graphic/animation.cc	2016-02-13 12:15:29 +0000
+++ src/graphic/animation.cc	2016-02-22 10:26:20 +0000
@@ -114,7 +114,7 @@
 
 			const std::string name = sound_effects->get_string("name");
 			const std::string directory = sound_effects->get_string("directory");
-			sound_effect_ = directory + "/" + name;
+			sound_effect_ = directory + g_fs->file_separator() + name;
 			g_sound_handler.load_fx_if_needed(directory, name, sound_effect_);
 		}
 

=== modified file 'src/io/filesystem/disk_filesystem.cc'
--- src/io/filesystem/disk_filesystem.cc	2016-02-15 20:37:29 +0000
+++ src/io/filesystem/disk_filesystem.cc	2016-02-22 10:26:20 +0000
@@ -447,7 +447,11 @@
 {
 	const std::string fullname1 = canonicalize_name(old_name);
 	const std::string fullname2 = canonicalize_name(new_name);
-	rename(fullname1.c_str(), fullname2.c_str());
+	if (rename(fullname1.c_str(), fullname2.c_str()) != 0)
+		throw wexception("DiskFileSystem: unable to rename %s to %s: %s",
+							fullname1.c_str(),
+							fullname1.c_str(),
+							strerror(errno));
 }
 
 

=== modified file 'src/logic/map_objects/tribes/production_program.cc'
--- src/logic/map_objects/tribes/production_program.cc	2016-02-18 08:42:46 +0000
+++ src/logic/map_objects/tribes/production_program.cc	2016-02-22 10:26:20 +0000
@@ -1524,7 +1524,7 @@
 		bool reached_end;
 		const std::string& filepath = next_word(parameters, reached_end);
 		const std::string& filename = next_word(parameters, reached_end);
-		name = filepath + "/" + filename;
+		name = filepath + g_fs->file_separator() + filename;
 
 		if (!reached_end) {
 			char * endp;

=== modified file 'src/logic/save_handler.cc'
--- src/logic/save_handler.cc	2016-02-18 18:12:48 +0000
+++ src/logic/save_handler.cc	2016-02-22 10:26:20 +0000
@@ -162,9 +162,7 @@
 std::string SaveHandler::create_file_name(const std::string& dir, const std::string& filename) const
 {
 	// Append directory name
-	std::string complete_filename = dir;
-	complete_filename += "/";
-	complete_filename += filename;
+	std::string complete_filename = dir + g_fs->file_separator() + filename;
 
 	// Now check if the extension matches (ignoring case)
 	if (!boost::iends_with(filename, WLGF_SUFFIX)) {