Replace ggz implementation with own InternetGaming function + metaserver

[Hans Joachim Desserud]

I noticed a bug report in Debian which basically suggests to drop the GGZ packages because it is unmaintained upstreams. Since Widelands depends on ggz, this of course affects us there (not to mention Debian derivaties like Ubuntu and Linux Mint).

I know the ggz dependency has been discussed a bit before, but we should probably figure out what the long-term plan is.

[Jens Beyer (qcumber-some)]

There is a discussion currently ongoing on widelands-public mailing list, as the packager of widelands in Debian has contacted us there, asking for our thoughts.

[SirVer (sirver)]

Debian will ship with a widelands build stripped of GGZ functionality (and therefore internet meta lobby play; direct IP and LAN support is still supported inside the debian build). We will leave GGZ behind and implement a lean (that is: feature poor) metaserver concept ourselves for b17. I consider this mission critical for b17.

[Jens Beyer (qcumber-some)]

This patch removes the dependencies to GGZ, as done in the branch. This is basically a diff between rev. 5903 and 5902.

[Nasenbaer (nasenbaer)]

I assigned it to Widelands Developers, as this is a major rewrite which more than one person is working on.

Currently I started with the refactoring of the current internet gaming implementation, to ease the implementation of a new one. First changes will hopefully be ready soon.

Should we open a new branch for this purpose, or better keep this in main trunk to directly force our playtesters to use the new implementation once it is in testing stage? ;)

As my next changes won't break ggz implementation I would vote for main trunk. What do YOU think?

[SirVer (sirver)]

>Should we open a new branch for this purpose, or better keep this in
>main trunk to directly force our playtesters to use the new
>implementation once it is in testing stage? ;)
We should use a new branch and merged it back as soon as it is in
testing stage. We need to force our users to switch asap.

>As my next changes won't break ggz implementation I would vote for main
>trunk. What do YOU think?
see no apparent reason to not work in a branch directly. We will sooner
or later rip ggz out of the code while players will want to continue GGZ
for as long as we are not ready for deployment. I am therefore for a
branch.

[Nasenbaer (nasenbaer)]

Alright.
My changes are currently not ready for a branch, however I took the time to write the initial protocol for communication between metaserver and client.
My intention is to reuse the network package code from Nicolai, which we already use for quite some time without any known issue - no need to write new network package code and no need to take care about sepearate implementation.

Is this alright, or should we go a different direction? Any comments or additions for the protocol?

[SirVer (sirver)]

I didn't seem anything that was not alright. However, I strongly urge
you to define an ASCII protocol -> it makes debugging much easier and
there is so little traffic on the metaserver that it makes no
difference.

[Nasenbaer (nasenbaer)]

Alright. Seems like a good idea to me as well. So what's the best way to go? When we use asci strings, we do not need an enum...

So should I just write the documentation as "human readable" doxygen in that protocol file, but without any c++ defines - that makes debugging of typos harder, as no compile time check for a definition is done - or should I add #define IGPCMD_LOGIN "IGPCMD_LOGIN", which makes it some how doubled and looks weired, but would ensure the gcc checking and would even ease later changes of a command string if needed for any reason.

What about the list for discussion?:

 LIST for discussion:
  - some messages are hardly translateable as they are generated dynamically or set during setup time
    like the motd. Others are hardcoded and thus could be translated on client side.
    Pro: users that don't understand english that well will better understand those messages
    Con: users might think they are in the channel for their language and thus might be rude to others
         not writing in their language or just be rude because other people won't answer in their
         mother tounge. JEPP, that's how people are sometimes. Unfortunally
    If we implement this, some INGCMD should just send the number for a certain message and the number will
    lead to a message in this (or a seperate) file.
  - how should the crosswork for this protocol file be done if the metaserver is not written in c/c++ ? <- already cleared up, as we use asci strings
  - what features should be accessible for superusers? I would like to have:
       * set motd
       * announcements (message that is even shown in running games in main stream)
       * kicking of rude players
       * (banning user for a certain time - not sure about that, there was no need before, but who knows?)

[Nasenbaer (nasenbaer)]

Attached is the new version of the protocol.

1) Commands are now strings. I decided to #define the command codes, as this seems more c++ish for me - else the code might look like as if you could send any string you want and the server would have to know what to do with it, and someone unfamiliar to the code would not have known which commands are actually available (somehow like magical strings - equivalent to magical numbers ;) )

2) I added the metaserver address (widelands.org) and the port (for now 7395 - Widelands already uses 7396 for games and 7397 for local game discovery)

I am currently working on the refactoring, hope to get the first version online soon. This will include the protocol file as well

[Hans Joachim Desserud (hjd)]

I am not sure how feasible this is, so feel free to ignore my comment.

That said, it would be nice if a test suite was added at the same time as the reimplementation so that we could make sure it was working and spot regressions. From remember from previous discussion, the current test coverage is fairly low, so it would be nice to cover a new (and not to mention rather important) section. Depending on whether we want the same behaviour as the current one it could also be used to to check that the new implementation is working according to what we want. Though if too much is changed anyways the latter is of course not relevant.

[Nasenbaer (nasenbaer)]

Hi hjd.

Generally I like the idea of automatic tests. However I am not sure how an automatic test could be used in this case. Maybe I missunderstood you, but:
* During the refactoring of the code (moving ggz code to it's own place and simplifying the internet code to prepare it for the rewrite) it would really be cool to check, that the massive move and partly rewrite of code does not break anything. However an automatic test seems to overpowered for me, as this is the minor part of the rewrite and functionality checks can be done (and are done by me) via connecting to the metaserver and starting+ ending a game.
* After the refactoring, we will write a new server and completely new client code. The only thing this has in common, with the ggz code is the wish, to at least have the same features as the ggz code had (we do not use the old ggz metaserver and can not use any part of the old ggz code for testing within the new code)- so to say "login, list of players, list of games, chat, possibility to open a game, possibility to join a game and possibility to send the game state "open" or "close" to the server) - an automatic check would not work in this place, as the first step would logically be the login - but at that state all other tests for the other features would always fail. If after that we implement the next feature and the login gets messed up somehow... well we will hopefully notice :)

[Nasenbaer (nasenbaer)]

branch is now online, with protocol file and good progress in refactoring. lp:~widelands-dev/widelands/ggz-replacement

[SirVer (sirver)]

protocol looks good to me. I would just suggest using static const std::string = "COMMANDNAME". #defines are not typesafe and have no benefits in this case (IMHO). Otherwise the protocol looks good to me. I am also pro ignoring translation of motd and therelike. The other messages should imho be templates that are translated inside widelands.

I agree with hjd that a testsuite would be desirable. The only way to do this properly is to run the "server" locally and propagate it with "fake" user and game data, then let the client run some automated tests against this. This would be easiest if the internet protocol where a stand alone library that is only linked against widelands. Obviously that would be desirable for all parts of widelands but in this case we can really achieve this because it is a small seperated part inside the code - especially after the refactoring. Could you try this out, peter?

[Nasenbaer (nasenbaer)]

alright I will change the strings to static const std::string IGPCMD_LOGIN = "LOGIN"; etc.

about the testsuite:
unfortunally the InternetGaming class is not *that* much "standalone" - it at least needs the network code for sending and receiving data packages + it has to be a chat controller (we could move all the chat stuff to the internet_lobby code, but that would only complicate things, as normally this is, what the InternetGaming class should handle). I don't know how much dependencies the chat controller and the network code has though... maybe it isn't that much and we could compile it as library quite easily (???)
I am still in the process of refactoring, so perhaps I will see things different in a few days ;) ...

anyways do you think the test suite should be written "in writing process" or only after the first completely working implementation? To be honest: I never wrote a testsuite before and all time I spend at the moment in the internet code is at the border of "too much time" ;) ... reading into the testsuit writing documentation would surely take a lot of that time... so i would prefare to get it working and once it is working to add a testsuit to ensure all changes that anyone does afterwards does not break anything.

[Nasenbaer (nasenbaer)]

okay.

Changes to the protocol are in the branch, as well as my initial version of InternetGamingMessages - mini class that returns a translated string for a message code from the metaserver like "WRONG_PASSWORD". Any comments on that?

[Hans Joachim Desserud (hjd)]

As I said, I wasn't sure how easy it would be. It just seemed like a nice opportunity to add tests to one of the important components. Like SirVer mentioned, I imagined something like running a local server and seeing if that responds in the expected way would be the easist way. (Mocking [1] might also be appropriate.)

I have to admit I wasn't really sure to what extent there would be to create an identical replacement or rather a server performing the same tasks. (If that makes sense?) In the latter case, tests for the old server would obviously not work for the new one. So that part is probably not relevant.

All in all though, it is probably more important to get something which work than to be able to automatically check whether it works. ;) So while I ideally would appreciate to see a test suite added at the same time, I'm perfectly happy that someone is capable of adding a replacement. It can also be argued that as long as for instance the protocol is still discussed writing tests might be a waste of time as those parts end up being changed later anyways, so it might make more sense to add them later, when we have a working implementation. I also realize that developer time is limited.

[1] http://en.wikipedia.org/wiki/Mock_object

[Nasenbaer (nasenbaer)]

Fix committed in bzr rev. 6217

[SirVer (sirver)]

Released in build17-rc1.