Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-uuid into lp:widelands
Looks mostly OK - I have added some nits.
We should test to make sure that 2 Widelands instances can be run from the same
computer and join the same game. I need to figure out how to run a metaserver.
Diff comments:
>
> === modified file 'src/network/internet_gaming.cc'
> --- src/network/internet_gaming.cc2017-08-16 04:31:56 +
> +++ src/network/internet_gaming.cc2017-10-14 06:58:24 +
> @@ -416,19 +405,24 @@
> if (state_ == CONNECTING) {
> if (cmd == IGPCMD_LOGIN) {
> // Clients request to login was granted
> - clientname_ = packet.string();
> + std::string assigned_name = packet.string();
> + if (clientname_ != assigned_name) {
> + format_and_add_chat("", "", true,
> + (boost::format(
> + _("You logged in as '%s' since
> your requested name is already in use or reserved."))
You have been logged in...
> + % assigned_name).str());
> + }
> + clientname_ = assigned_name;
> clientrights_ = packet.string();
> + if (clientrights_ == INTERNET_CLIENT_UNREGISTERED) {
> + // Might happen that we log in with less rights
> than we wanted to.
> + // Happens when we are already logged in with
> another client.
> + reg_ = false;
> + }
> state_ = LOBBY;
> log("InternetGaming: Client %s logged in.\n",
> clientname_.c_str());
> return;
>
> - } else if (cmd == IGPCMD_RELOGIN) {
> - // Clients request to relogin was granted
> - state_ = LOBBY;
> - log("InternetGaming: Client %s relogged in.\n",
> clientname_.c_str());
> - format_and_add_chat("", "", true, _("Successfully
> reconnected to the metaserver!"));
> - return;
> -
> } else if (cmd == IGPCMD_ERROR) {
> std::string errortype = packet.string();
> if (errortype != "LOGIN" && errortype != "RELOGIN") {
> @@ -451,9 +445,8 @@
> cmd.c_str());
> }
> }
> -
> try {
> - if (cmd == IGPCMD_LOGIN || cmd == IGPCMD_RELOGIN) {
> + if (cmd == IGPCMD_LOGIN) {// || cmd == IGPCMD_RELOGIN) {
Remove commented out code
> // Login specific commands but not in CONNECTING
> state...
> log("InternetGaming: Received %s cmd although client is
> not in CONNECTING state.\n",
> cmd.c_str());
> @@ -766,6 +759,10 @@
>
> /// ChatProvider: sends a message via the metaserver.
> void InternetGaming::send(const std::string& msg) {
> + // TODO(Notabilis): Messages can get lost when we are temporarily
> disconnected from the metaserver,
> + // even when we reconnect again. "Answered" messages like
> IGPCMD_GAME_CONNECT are resend but chat
resend -> resent
> + // messages are not. Resend them after some time when we did not
> received the matching IGPCMD_CHAT
received -> receive
> + // command from the server?
> if (!logged_in()) {
> format_and_add_chat(
> "", "", true, _("Message could not be sent: You are not
> connected to the metaserver!"));
> @@ -777,6 +774,7 @@
>
> if (msg.size() && *msg.begin() == '@') {
> // Format a personal message
> + // TODO(Notabilis): msg should be trimmed before checking for
> the space character
You can do that right now with boost::trim. Include
for that.
> std::string::size_type const space = msg.find(' ');
> if (space >= msg.size() - 1) {
> format_and_add_chat(
> @@ -834,6 +832,7 @@
> SendPacket m;
> m.string(IGPCMD_ANNOUNCEMENT);
> m.string(arg);
> + // NOCOM(Notabilis): This looks like a bug to me
Can you be a bit more specific? We shouldn't have NOCOM stuff in trunk.
> net->send(s);
> return;
> } else
>
> === modified file 'src/network/internet_gaming.h'
> --- src/network/internet_gaming.h 2017-07-01 08:22:54 +
> +++ src/network/internet_gaming.h 2017-10-14 06:58:24 +
> @@ -57,10 +57,12 @@
> void reset();
>
> // Login and logout
> + // pwd should contain either the password for a registered account or
> the UUID otherwise
> void initialize_connection();
> bool login(const std::string& nick,
> const std::string& pwd,
> bool registered,
> +
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-uuid into lp:widelands
Thanks for the review! And for the grammar errors... ehem. After some testing I am no longer so sure about the trimming of chat messages. Currently it is possible to send space-only messages to everyone or as a whisper message to a single player. So we should probably remove trailing (and leading?) space from all messages. Shall I do so? (A slight disadvantage would be that I can no longer explain the @whisper syntax by prepending it with a space.) The first NOCOM is related to the fact that the SendMessage m is written to but the SendMessage s is send. It looks pretty broken to me, but has no-one ever tried to send an announcement and discovered this bug? I would just like to have a short confirmation that this is a bug (looking at the code is good enough for me). As far as I understand it (language-wise), sent_uuid is correct. The variable is a boolean specifying whether the uuid should be sent, not the uuid itself. (The checkbox in the login dialog.) I can't reproduce the bug of the second NOCOM, probably it was something unrelated to the code. Maybe on layer 8. All on my local computer: Starting a metaserver and three clients. All Clients try to login with the name "Notabilis", second and third clients receive the names "Notabilis1" and "Notabilis2" respectively. First client opens a game, other two join it. As far as I can tell, everything worked fine. If this answers your test, fine. But feel free to do some tests on your own. If you want me to setup a metaserver for it, just say so. (Hint: There is a --metaserver=IP command line switch in the game. Took me much too long to notice it.) -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/net-uuid 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/net-uuid into lp:widelands
Review: Approve I opened a new issue over at the metaserver, because I don't know how to run one. So, I'll just rely on your testing here. As to the 2 NOCOMs, best open new bugs for them and note that they need investigating whether they are bugs. As to the trimming, you could use it for an .empty() check only to prevent sending empty messages. The font renderer does some trimming on its own, so it shouldn't be an issue when the messages are displayed. For explaining the @whisper command, you could also rephrase it to have the explanation start with "Use @whisper at the beginning of the line to... " Feel free to merge and deploy this once your'e ready. -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
The NOCOM-bug is really a bug and has been fixed. I fear I introduced it myself a few month ago. Trimming is also added now. Space is removed in front and at the end of messages. When this results in an empty message, it is dropped. This is also done with the message-part of the @whisper, /motd and /announcement commands. As far as I am concerned, this can be merged as soon as the metaserver is ready. -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
Changes LGTM :) -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
Review: Needs Fixing Reviewing now. Sorry that this waited so long for me. 1) third_party/crypto is under "public domain" which is not a license in most parts of the world. I expect issues with debian about this. Can we ask the author to relicense under Apache 2 or MIT? Thes licenses are mostly what the author intended with public domain, but more formal. We can merge this before that happens of course. 2) I am not a fan of 'send_uuid' as a variable name - its semantics are unclear. I do not understand if it means: 'client expects server to send a uuid' or 'server expects client to send a uuid'. Maybe that becomes clearer while I read on, but a better name would be appreciated. 3) internet_gaming_protocol.h: remove all mentions of the nounce implementation? e.g. you write that the UUID replaces the nounce. But the nounce was only an interim version of the metaserver that we no longer support - so why keep this historic backage in the documentation? I added some consts, and a few clarifying comments. After addressing these changes this lgtm. Will review the go code next. -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
Thanks for the review! 1) I replaced the third party crypto code with a call to a boost function. This means that instead of SHA-3 it uses SHA-1 now. This is less secure (not that it matters in this case) but SHA-1 will most likely be needed in a future branch of mine anyway. A slight problem with the boost function: It is part of a ::detail:: namespace so it might become removed at some time. 2) I renamed send_uuid to use_permanent_uuid. Not sure if this is better. What is an improvement is that I removed the variable from the InternetGaming class. It does not need to now whether it is a temporary or permanent ID and calling classes can handle this themselves. 3) Done. Another point: I replaced my primitive "random string" code with boost::uuid. And then I realized that a simple random number would be just as good. So we can remove the random_string() function if we want to. -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
1) SHA1 is sufficient for our crypto needs for sure. And using a boost function is preferable for now too. We deal with it going away once it happens. Thanks! 2) I was thinking about this a bit and I suggest removing the option and always use the "permanent", i.e. 24h valid UUID. We could reduce the 24 hours to 2h or 5h or so to make sure the UUID shuffles daily and does not become personally identifiable information. Its only use is to make reconnecting possible with the same nickname, for this we do not need the uuid to stay alive long - it must basically survive the crash and restart of Widelands. The option is very confusing for users - they need to know what a UUID is and understand how it is used inside of the metaserver. That is an impossible ask. Also, relogin into the same ID is expected behavior for users, so even those that would opt out would still not be happy that they get a new ID assigned on relogin. So: Get rid of the option and make the behavior default? 3) Yes please! I like uuid() much better and it conveys the semantic better than random_string(). I also like that we use a true uuid in the places where we call our variables uuid. Much better! -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
2) Would be fine by me. The "temporary"-solution got in before the 24h lifetime was suggested by GunChleoc and I never reconsidered it. So it can go now. Reducing the lifetime can be done, too, but 2 hours is probably not enough. The timestamp is stored when starting the game so when I have the game running for over 2 hours and it crashes then a new UUID will be generated. So maybe something like 12 hours? 3) I don't really understand what you mean here. I am now using boost::uuid() which seems to be fine. I am not sure whether something else should be changed. Do you want me to drop the string/uuid-string and use a simple integer? Or rename random_string() to generate_uuid()? Or... ? -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
2) 12h sounds good to me. We could go more fancy and rewrite the last_started_timestamp every few real world seconds, but just doing 12h is fine IMHO. 3) Sorry, that was unclear from me. I suggest renaming the function. I did this real quick while thinking how it should be named and investigating if the boost generator could be static in the function. I missed that during review - c++ global variables should be avoided like the plague. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
The option to use a temporary UUID has been removed and the interval for the "permanent" ones reduced to 12 hours. So I guess this branch is ready for merge now? Thanks for the link, will be an interesting read. Is that the style guide Widelands (or you?) are trying to follow? -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
> The option to use a temporary UUID has been removed and the interval for the > "permanent" ones reduced to 12 hours. So I guess this branch is ready for > merge now? Will have another quick read through in a minute, but yes, I think so. I will merge this and also merge the metaserver branch as soon as this is in. Thanks for working on this! Now on to the even more important relay stuff :). > Thanks for the link, will be an interesting read. Is that the style guide > Widelands (or you?) are trying to follow? Unfortunately, Widelands predates the Google style guide. Therefore it is not really enforcable in our code base anymore. Also, the style guide mixes style (indent, whitespace, curly braces positions) with good practices (no globals, no non-const references as arguments). Generally it is the best guideline for writing good c++ that I know and I really like the thorough explanations that come with each point. Having worked at Google for 5 years, I also came to appreciate how many bugs following the style guide avoids. So I usually adhere to it whenever I write new code. So I'd say, having a read through will give you a lot of food for thought. -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
Review: Approve Looks great! Thanks again. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
r8472 has messed up the export to GitHub and the git repository state in the related git branch. Travis will not run again for this branch and merging this into trunk will likely mess up the trunk states in git as well. I will do some surgery on this branch and only retain the diff, throwing away all revisions. -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
For the record, this was the surgery I did: In the net-uuid branch: $ bzr diff --diff-options="-u" -r ancestor:lp:widelands > /tmp/diff.patch Then get a fresh trunk branched: $ bzr branch lp:widelands $ patch -p0 < /tmp/diff.patch $ bzr commit -m "All changes in one clean diff on master." --author "Notabilis " $ bzr push --overwrite lp:~widelands-dev/widelands/net-uuid Unfortunately that made bunnybot unhappy and I had to manually intervene and fix some state. Next time, we should just close the merge request and reopen a new one with the clean patch. @Notabilis: sorry for loosing your history, but at least the work is still credited to you in full. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
No problem. As long as we have one commit for the merge the single steps aren't that important. Seems like I am quite talented with breaking bunnybot though, sorry. So the problem was merging with the same branch? I would have thought that this is a quite normal operation for bzr and git, isn't it? What would have been the bunnybot-friendly way to merge(?) the diverged branch heads? -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
@bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
> Seems like I am quite talented with breaking bunnybot though, sorry. Hardly your fault :/. The whole setup with launchpad -> bzr -> git -> github is kinda fragile by design. > So the problem was merging with the same branch? Yes, that was fine for bzr. But in the git import it deleted all the files except for the ones changed in this branch. > What would have been the bunnybot-friendly way to merge(?) the diverged > branch heads? I think a rebase would have been better. Unfortunately rebase is not part of the core bzr tools, but requires a bzr plugin to be installed: https://launchpad.net/bzr-rewrite. Then a bzr rebase does the job. You are right that merging the diverged branch is the bzr way to do it - it is a bummer that this apparently does not work in our setup. -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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/net-uuid into lp:widelands
This branch has been merged and the metaserver has been updated, too. This means that previous versions of trunk can no longer connect to the metaserver. Branches based on older trunk are also affected, build 19 should not be affected. -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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
