Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
Travis error is a timeout @bunnybot merge force -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
Don't worry about it - I know you're still finding your way around Launchpad and I'm happy to help. If somebody decides to mess with the temp while playing, on their own head be it. At least everybody else will know it's temp files if they want to manually clean it out at a sensible point in time ;) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
Oh, I see you had some other nits that you fixed. Fair enough. I should have checked more thoroughly. Or did you mention some specifics before somewhere and I had missed them? As for the naming of the temp dir, I also had it named "temp" first but felt that might encourage players to just delete it (possibly while the game is up, thus messing up their next save). I guess I was overreacting because a friend of mine notoriously does this kind of thing, always complaining that stupid programmers never clean up their temp stuff. But honestly, "temp" is perfectly fine. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
Huh? I had already fixed them. Did I miss something? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
Review: Approve I have now fixed the nits myself, so that we can start reviewing your other branches @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
You have introduces a "rnrnrn " string in your last commit in a license header ;) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
The automatic builds are triggered by every push to a branch that has an active merge request. For our testing environment, including our codecheck, see: https://wl.widelands.org/wiki/RegressionTests/ If you scroll up to my first comment, you'll see a green "Show diff comments" link. Click on it and you'll see my comments in the diff. For longer diffs, I use the browser's search function and search for the reviewer's username to skip through the comments. I agree with your reasoning about an "is writable" check, let's keep this branch as is. I'd like my diff comments addressed though. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
Btw, this autosave-thing you observed was a (I think even relatively recently introduced and somewhat dirty) error handling attempt by someone. When the wl_autosave_00.wgf couldn't be renamed (for backup purposes) then there was some "let's just try other filenames" approach, and the game would just find the next autosave filename (up to 09) where there didn't exist any file. At 09 it would just stop and not check any further, but use this as the target name, so from this point on wl_autosave_09.wgf would just have been overwritten with every autosave. (Which worked in your case, because this one wasn't file-locked.) My latest branch is stricter in this regard. If some relevant renaming or deletion fails, then the autosaving is simply aborted, and there is a little message. I don't think the game needs to handle this in some special way. The player gets a message and can try to fix a problem (if there's a file lock for example) or has to save manually. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
1. Regarding the autosave stuff: This wasn't anything this branch tried to solve. It only solved the the conflicts that arose from trying to modify files where the map fs pointed to, resulting in either some crash or error (if the file was currently locked) or resulting in corrupt savefiles. Those other file conflicts (like trying to modify a savefile that is locked because you have it open with 7zip, or having a file write-protected or some such) are solved with the latest branch: https://code.launchpad.net/~widelands-dev/widelands/robust-saving As for trying to specifically test "is_writable" instead of "file_exists" (or other potentially problematic file states) beforehand, I don't think it's worth it. For one thing, we might have to deal with the specifics of the various access control models on the different platforms. (Just looking at this stuff makes me nauseaous.) There is also the more general problem with any filesystem stuff that you can never be sure that the file state you just checked is still the same when you try your file operation next. Sure, it's very likely, but there can always be race conditions, and some other process might just have snatched the file away between your state check and your attempt to operate on a file. So aside from a few minor exist checks I favour a simple, practical approach: just try the file operation, but catch and handle any errors. The error codes give a hint about what the actual problem was, but usually that doesn't matter much anyway: either it worked and we continue or it didn't and we abort. Anyway, the latest branch https://code.launchpad.net/~widelands-dev/widelands/robust-saving does exactly that and handles file errors in all the map/game saving routines and cleanup routines. I still need to check in what other places we don't catch file errors but should. (Definitely when deleting files in the load/save menu, maybe synchstream creation, probably some other places.) That's going to be part of future branches though. 2. Those travis build errors I hadn't actually seen this before. (I'm still a noob on Launchpad and don't know my way around very well, but I am trying to catch up. If there are any important features I might not but should know about, feel free to point something out.) Anyway, those errors seem like minor nitpicks. I'll fix them. Btw, are those builds made automatically here for any new branches? Or do I have to initiate them manually? (I haven't checked out the online building possibilities yet. Still on my todo list.) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
I have tested this by allowing only 1 autosave file and sitting on wl_autosave_00.wgf with 7zip on Windows. Since wl_autosave_01.wgf - wl_autosave_05.wgf already existed, the autosaves that I got were wl_autosave_06.wgf - wl_autosave_09.wgf I think this branch is good to go in after the other nits have been fixed, because it already improves the situation greatly, but maybe we could have some form of "is_writable" test rather than "file_exists"? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into lp:widelands
Code LGTM - just some minor nits.
Diff comments:
>
> === modified file 'src/editor/ui_menus/main_menu_save_map.cc'
> --- src/editor/ui_menus/main_menu_save_map.cc 2018-06-01 08:50:29 +
> +++ src/editor/ui_menus/main_menu_save_map.cc 2018-10-22 18:50:51 +
> @@ -272,16 +272,14 @@
> if (mbox.run() ==
> UI::Panel::Returncodes::kBack)
> return false;
> }
> -
> - // save to a tmp file/dir first, rename later
> - // (important to keep script files in the script directory)
> - const std::string tmp_name = complete_filename + ".tmp";
> - if (g_fs->file_exists(tmp_name)) {
> +
> + // Try deleting file (if it exists). If it fails, give a message and
> let the player choose a new name.
> + try {
> + g_fs->fs_unlink(complete_filename);
> + } catch (const std::exception& e) {
> const std::string s =
> -(boost::format(
> -_("A file with the name ‘%s.tmp’ already exists. You
> have to remove it manually.")) %
> - FileSystem::fs_filename(filename.c_str()))
> - .str();
> +(boost::format(_("File ‘%s.tmp’ could not be deleted.")) %
> FileSystem::fs_filename(filename.c_str())).str()
The .tmp ending is no longer used here
> ++ " " + _("Try saving under a different name!");
> UI::WLMessageBox mbox((), _("Error Saving Map!"), s,
> UI::WLMessageBox::MBoxType::kOk);
> mbox.run();
> return false;
>
> === modified file 'src/logic/editor_game_base.cc'
> --- src/logic/editor_game_base.cc 2018-09-28 05:41:33 +
> +++ src/logic/editor_game_base.cc 2018-10-22 18:50:51 +
> @@ -67,12 +70,86 @@
> : gametime_(0),
> lua_(lua_interface),
> player_manager_(new PlayersManager(*this)),
> - ibase_(nullptr) {
> + ibase_(nullptr),
> + tmp_fs_(nullptr) {
> if (!lua_) // TODO(SirVer): this is sooo ugly, I can't say
> lua_.reset(new LuaEditorInterface(this));
> }
>
> EditorGameBase::~EditorGameBase() {
> + delete_tempfile();
> +}
> +
> +/**
> + * deletes the temporary file/dir
> + * also resets the map filesystem if it points to the temporary file
> + */
> +void EditorGameBase::delete_tempfile() {
> + if (!tmp_fs_)
Please add {} here. We are trying to avoid dropping optional {}, because that
can lead to bugs.
> + return;
> + std::string fs_filename = tmp_fs_->get_basename();
> + std::string mapfs_filename = map_.filesystem()->get_basename();
> + if (mapfs_filename == fs_filename)
> + map_.reset_filesystem();
> + tmp_fs_.reset();
> + try {
> + g_fs->fs_unlink(fs_filename);
> + } catch (const std::exception& e) {
> + // if file deletion fails then we have an abaondoned file lying
> around, but otherwise that's unproblematic
abaondoned -> abandoned
> + log("EditorGameBase::delete_tempfile: deleting temporary
> file/dir failed: %s\n", e.what());
> + }
> +}
> +
> +/**
> + * creates a new file/dir, saves the map data, and reassigns the map
> filesystem
> + * does not delete the former temp file if one exists
> + * throws an exception if something goes wrong
> + */
> +void EditorGameBase::create_tempfile_and_save_mapdata(FileSystem::Type const
> type) {
> + // should only be called when a map was already loaded
> + assert(map_.filesystem());
> +
> + g_fs->ensure_directory_exists(kTempFileDir);
> +
> + std::string filename = kTempFileDir + g_fs->file_separator() +
> timestring() + "_mapdata";
> + std::string complete_filename = filename + kTempFileExtension;
> +
> + // if a file with that name already exists, then try a few name
> modifications
> + if (g_fs->file_exists(complete_filename))
> + {
> + int suffix;
> + for (suffix = 0; suffix <= 9; suffix++)
> + {
> + complete_filename = filename + "-" +
> std::to_string(suffix) + ".tmp";
> + if (!g_fs->file_exists(complete_filename))
> + break;
> + }
> + if (suffix > 9) {
> + throw
> wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all
> considered filenames a file already existed");
> + }
> + }
> +
> + // create tmp_fs_
> + tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename, type));
> +
> + // save necessary map data (we actually save the whole map)
> + Widelands::MapSaver* wms = new Widelands::MapSaver(*tmp_fs_, *this);
We're trying to get rid of raw pointers. Try if Widelands::MapSaver*
wms(Widelands::MapSaver(*tmp_fs_, *this)); does the job, and it not, use a
unique_ptr.
> + wms->save();
> + delete wms;
> +
> + // swap map fs
> + std::unique_ptr mapfs(tmp_fs_->make_sub_file_system("."));
> + map_.swap_filesystem(mapfs);
> +
