Page MenuHomeWildfire Games

Major improvements to the lobby bots XpartaMuPP and EcheLOn.
Needs ReviewPublic

Authored by user1 on Feb 11 2020, 6:11 PM.

Details

Reviewers
elexis
Dunedan
Summary

Merge from https://github.com/Dunedan/XpartaMuPP

This code is based from r19467 .

The following revisions are lost because of this:

r22460 | 2019-07-12 13:40:40 -0400 (Fri, 12 Jul 2019) Fix lineendings. (partially lost)
r21927 | 2018-11-07 12:32:21 -0500 (Wed, 07 Nov 2018) The files in this path were not marked as moved in rP21926...
r21926 | Nov 7, 2018, 6:23:56 PM Split XpartaMuPP and EcheLOn into separate directories and rename parent folder to "lobbybots" following rP18609.
r21924 | 2018-11-07 12:31:01 -0100 Support connecting the lobby bots without TLS errors if the server does not devlier a valid, non-selfsigned certificate.

It should be that those commits are now merged into this patch.

NOTICE:

  • The database url can be specified now. This enables the use of a different database engine. Recently the ratings database was migrated to a MySQL(mariaDB) database. This has been tested and there appears to be the need for some changes to get this working without issue. (#5634)
  • The README.md from this git repo was not included. It should be a merged with our version.
  • This changes the directory structure and file naming convention.

I found at least the following patch that would need to be changed after this:


Changes

General Improvements:

Up until now XpartaMuPP was able to relay requests meant for EcheLOn to EcheLOn. As 0ad now sends such requests directly to EcheLOn we don't need that functionality anymore, which makes the code way more readable. Additionally this closes a vulnerability which allowed clients to manipulate the ratings of other players by submitting requests directly to EcheLOn, but including the in the sent XML, which normally only XpartaMuPP did for requests forwarded to EcheLOn.

It turns out the XEP-0045 plugin for SleekXMPP already stores information about all participants in a MUC room. So instead of collecting and storing this information in the bots again, simply rely on the information provided by the plugin. This reduces the required memory for the additional storage of nick-JID-relations in the bots and reduces the code complexity.

Previously EcheLOn was assumed offline if he wasn't present in the MUC nick list maintained by XpartaMuPP. This was prone to race conditions on EcheLOn joining or leaving the room. Now instead of assuming if EcheLOn is online or not a IQ query is sent in every case and in case EcheLOn isn't online the error response is handled appropriately. Additionally this commit sends responses back to the 0ad clients in case EcheLOn is offline to better comply with the request/response model of IQ queries. These responses are currently empty result responses, as 0ad shows nasty error messages in the chat window for error responses.

For all requests made from clients to XpartaMuPP, which get forwarded to EcheLOn, return the correct stanza id to the client. This is implemented using a size limited dictionary for the mapping of ids between clients and XpartaMuPP and ids between XpartaMuPP and EcheLOn so ids without a response from EcheLOn get evicted over time while avoiding an ever growing memory consumption of XpartaMuPP in such a case.

As we have two separate bots with clearly defined responsibilities now, it adds a nice touch to let them tell what their purpose is, when they're asked. As part of that "Ratings" got a nick change to "RatingsBot" to not trigger the message accidentally when talking about ratings and "WFGbot" got a nick change to "WFGBot" to be in line with "RatingsBot" in terms of capitalization.

Using callbacks has the advantages, that only expected iq results will be processed and not all iq results which might happen to find its way to XpartaMuPP.

Remove the checks if player is in the room, when sending a response, as there is proper error handling for the case that the player isn't online anymore.

Previously XpartaMuPP relayed information of players coming online to EcheLOn. As EcheLOn has to be part of the lobby-MUC as well (e.g. to post game results) it can watch and handle this information without having to rely on XpartaMuPP.

The gamelist stanza is now created only once when sending it to all players instead of once per player.

Major Fixes:

At some point sleekxmpp seemed to have changed the data structure or iq.plugins, making it incompatible with xpartamupp. This commit removes the affected pieces, which weren't necessary anyway.

Minor Fixes:

XpartaMuPP was vulnerable to DoS, by opening an infinitive amount of games, which causes the responses to occupy all available memory and make the lobby unusable, because the responses become too big and to compute intensive. Even 128 might be still too high for a useable lobby, but at least it's not possible anymore for clients to use it to fill up all available memory. The proper fix for all related performance problems is PubSub, which will be introduced later.

The threaded-parameter for SleekXMPP got deprecated several years ago, probably because it got replaced by the block-parameter. In any case its name was misleading, as all it does is to control whether the event dispatcher will run in a separate thread or not and not if threads are used at all or not (hint: they are).

Code Style/Smells:

Also remove PlayerXmppPlugin from echelon, as it apparently wasn't used there.

Reduces the complexity of the iq handling code.

Following "explicit is better than implicit". Also uses loggings functionality for printing stacktraces instead of using the traceback module.

Database:

Allows the use of alternative file locations for the SQLite database as well as completely

Tests:

Docstrings:

Ensure all methods have proper docstrings including the notion of arguments and returned values, although those aren't perfectly documented yet.

Textual:

  • double quotes for every human readable message - single quotes for everything else - exception if something in the quoted string would need quoting
Test Plan

The functionality of the bots as seen from the lobby should not change after this.

Verify that all bot functions behave as they did before.

Specifically test:
host game
join game
change match settings
launch game
rejoin?
rated match

Unit tests:
Run the unit tests
chmod +x setup.py
./setup.py test

New unit tests:
Descriptions: [TODO] (do we the descriptions here?)

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

user1 created this revision.Feb 11 2020, 6:11 PM
user1 created this object with visibility "user1 (Aaron C)".
user1 created this object with edit policy "user1 (Aaron C)".
user1 added a comment.EditedFeb 11 2020, 7:07 PM

https://github.com/Dunedan/XpartaMuPP/commit/014f52bc4d8f36fc570d3139b4fd90f54c072e47 Initial commit with code from SVN

General Improvements:
https://github.com/Dunedan/XpartaMuPP/commit/aa7b7ed2c97c0f318d30eb874fc7f6f6d8334a22 Untangle XpartaMuPP and EcheLOn Up until now XpartaMuPP was able to relay requests meant for EcheLOn to EcheLOn. As 0ad now sends such requests directly to EcheLOn we don't need that functionality anymore, which makes the code way more readable. Additionally this closes a vulnerability which allowed clients to manipulate the ratings of other players by submitting requests directly to EcheLOn, but including the in the sent XML, which normally only XpartaMuPP did for requests forwarded to EcheLOn.
https://github.com/Dunedan/XpartaMuPP/commit/597b7960efbb4ddfd679a4a697ee88162a369be2 Use MUC-plugin provided participant information It turns out the XEP-0045 plugin for SleekXMPP already stores information about all participants in a MUC room. So instead of collecting and storing this information in the bots again, simply rely on the information provided by the plugin. This reduces the required memory for the additional storage of nick-JID-relations in the bots and reduces the code complexity.
https://github.com/Dunedan/XpartaMuPP/commit/c5beb0c741bb83a64a975d2f4f969b0f6b9d7792 Change handling for offline EcheLOn Previously EcheLOn was assumed offline if he wasn't present in the MUC nick list maintained by XpartaMuPP. This was prone to race conditions on EcheLOn joining or leaving the room. Now instead of assuming if EcheLOn is online or not a IQ query is sent in every case and in case EcheLOn isn't online the error response is handled appropriately. Additionally this commit sends responses back to the 0ad clients in case EcheLOn is offline to better comply with the request/response model of IQ queries. These responses are currently empty result responses, as 0ad shows nasty error messages in the chat window for error responses.
https://github.com/Dunedan/XpartaMuPP/commit/188b324c11f0bc5563ff4a8bba41700643880b60 Return correct stanza id for requests to EcheLOn For all requests made from clients to XpartaMuPP, which get forwarded to EcheLOn, return the correct stanza id to the client. This is implemented using a size limited dictionary for the mapping of ids between clients and XpartaMuPP and ids between XpartaMuPP and EcheLOn so ids without a response from EcheLOn get evicted over time while avoiding an ever growing memory consumption of XpartaMuPP in such a case.
https://github.com/Dunedan/XpartaMuPP/commit/f245250ec8e33d55917519cb39b144d0f9001bbe Process events in the main event loop
https://github.com/Dunedan/XpartaMuPP/commit/016c5744bf7161a570fdf2fd5cbb42ecb9853b50 Use the send queue when sending messages
https://github.com/Dunedan/XpartaMuPP/commit/806c4b895206f4673c381ce39a9942d9e19387c5 Separate usage of JIDs and nicks
https://github.com/Dunedan/XpartaMuPP/commit/0a6b833a331dcc6c5e6d01f35f9da446df180f4b Let the bots answer with meaningful messages As we have two separate bots with clearly defined responsibilities now, it adds a nice touch to let them tell what their purpose is, when they're asked. As part of that "Ratings" got a nick change to "RatingsBot" to not trigger the message accidentally when talking about ratings and "WFGbot" got a nick change to "WFGBot" to be in line with "RatingsBot" in terms of capitalization.
https://github.com/Dunedan/XpartaMuPP/commit/7b39038edb9280eadcf2c7dd4cc51ea35468656f Use JID objects instead of passing around strings
https://github.com/Dunedan/XpartaMuPP/commit/1e6ea63f37ed6481c2d31d6d00a4705851ae3ee4 Use callbacks for EcheLOn responses Using callbacks has the advantages, that only expected iq results will be processed and not all iq results which might happen to find its way to XpartaMuPP.
https://github.com/Dunedan/XpartaMuPP/commit/55898c720f2b0d101b3e862f46582f469a29b3ac Don't try to predict if player can get response Remove the checks if player is in the room, when sending a response, as there is proper error handling for the case that the player isn't online anymore.
https://github.com/Dunedan/XpartaMuPP/commit/1c5a3a66d0b018a7c9d69cc024da8f5d1bda2749 Remove relay of player online information Previously XpartaMuPP relayed information of players coming online to EcheLOn. As EcheLOn has to be part of the lobby-MUC as well (e.g. to post game results) it can watch and handle this information without having to rely on XpartaMuPP.
https://github.com/Dunedan/XpartaMuPP/commit/02ad11d5681bf07ed304daf73e74571d4953a315 Improve performance for gamelist sending The gamelist stanza is now created only once when sending it to all players instead of once per player.

Major Fixes:
https://github.com/Dunedan/XpartaMuPP/commit/93f2a38fced76d80eb67609c45c038e587df36dc Fix handling with newer sleekxmpp versions At some point sleekxmpp seemed to have changed the data structure or iq.plugins, making it incompatible with xpartamupp. This commit removes the affected pieces, which weren't necessary anyway.
https://github.com/Dunedan/XpartaMuPP/commit/0e6a8060843475b2de023dc1eb9b63929e85fa18 Replace deprecated optparse with argparse

Minor Fixes:
https://github.com/Dunedan/XpartaMuPP/commit/6fd9ceb9b5f765bf949de8727bd3d2f95db26096 Remove unused attribute
https://github.com/Dunedan/XpartaMuPP/commit/bcb78e02f97d2aa8121f61a2e2f64637ceb44b52 Fix stanza ids when broadcasting gamelists
https://github.com/Dunedan/XpartaMuPP/commit/cd0d593f96babc728caf6591ba281e73a15d123c Limit the max amount of open games to 128 XpartaMuPP was vulnerable to DoS, by opening an infinitive amount of games, which causes the responses to occupy all available memory and make the lobby unusable, because the responses become too big and to compute intensive. Even 128 might be still too high for a useable lobby, but at least it's not possible anymore for clients to use it to fill up all available memory. The proper fix for all related performance problems is PubSub, which will be introduced later.
https://github.com/Dunedan/XpartaMuPP/commit/cfa2f30565f35717739b93520030ad0f5ffba38e Fix a small log formatting bug
https://github.com/Dunedan/XpartaMuPP/commit/d064798045999fe3f8e8f315ba0d50d454c73ca1 Prevent use of non-0ad resources
https://github.com/Dunedan/XpartaMuPP/commit/398eda1706893e6db5bc26cd2e81deaf1019d41f Remove deprecated threaded-parameter The threaded-parameter for SleekXMPP got deprecated several years ago, probably because it got replaced by the block-parameter. In any case its name was misleading, as all it does is to control whether the event dispatcher will run in a separate thread or not and not if threads are used at all or not (hint: they are).
https://github.com/Dunedan/XpartaMuPP/commit/65b2d1e308397083dc5bfe0c6891b64fe5f9f3de Use XMPP Pings instead of "whitespace pings"
https://github.com/Dunedan/XpartaMuPP/commit/1ba2ddbf0cde193bdfb2ebca0c9de3d2d3160dd6 Fix module for sleekxmpp.jid.JID
https://github.com/Dunedan/XpartaMuPP/commit/5f5b1b291605ec70039d35ed7289f00588434c46 Provide proper variable types for calling methods
https://github.com/Dunedan/XpartaMuPP/commit/09c7f0abb3994321c4130a84a70cfe450c5ac4a8 Nicer variable interpolation for log statements

Code Style/Smells:
https://github.com/Dunedan/XpartaMuPP/commit/1ae27e082980e09d1a97e65c28e7ee97a5f62bd9 Make it a proper Python package
https://github.com/Dunedan/XpartaMuPP/commit/d76457e45f287560373a4b1a4cdee1954ced2c4f First round of Python style fixes
https://github.com/Dunedan/XpartaMuPP/commit/7a5894604bb296a0aa0af633bf63c30f872bca9d Move common stanzas into a separate module
https://github.com/Dunedan/XpartaMuPP/commit/9b3c59c479fe8e476617711e2789d91efd8ccc9a Move PlayerXmppPlugin to stanzas Also remove PlayerXmppPlugin from echelon, as it apparently wasn't used there.
https://github.com/Dunedan/XpartaMuPP/commit/5a6073d081c1171aa8fe86bdd5b5830a5fd69054 Fix some smaller smells
https://github.com/Dunedan/XpartaMuPP/commit/ce80bab844e26e5a4cc4f2c03fa5c3ff46cc1552 Some code style improvements
https://github.com/Dunedan/XpartaMuPP/commit/f85e37da5d33c6ce23764265af1b4077b06c2bc7 Some style fixes
https://github.com/Dunedan/XpartaMuPP/commit/cea2b63b695d762b3a17617d6ddf4c9127ad3ed0 Clean up iq handling
https://github.com/Dunedan/XpartaMuPP/commit/ca7b65d80874675a7b607b38e46a5102cfd4d237 Mark internal methods as internal
https://github.com/Dunedan/XpartaMuPP/commit/ce5788cf3a158352abb749b8af22f7bfa3b9afd1 Split iq handler functions Reduces the complexity of the iq handling code.
https://github.com/Dunedan/XpartaMuPP/commit/16b4ed73b4a590d56acc63be95034dd59efb790e Replace bare-except with broad-except Following "explicit is better than implicit". Also uses loggings functionality for printing stacktraces instead of using the traceback module.
https://github.com/Dunedan/XpartaMuPP/commit/b02e5a50b4fcab87731e2c5deff693706b8bcb03 Improve structure of relay methods
https://github.com/Dunedan/XpartaMuPP/commit/ae3ce1af8d928d00385a75dc58726c81401c7bbd Rename GamesList to Games
https://github.com/Dunedan/XpartaMuPP/commit/d8e6dd30a4773849d8bc70395b0e324ebe3fe8dc Rename LeaderboardList to Leaderboard
https://github.com/Dunedan/XpartaMuPP/commit/0a77daa6b6d1f2057a40169aa28c5c08646ab30f Inline _relay_rating_list_request

Database:
https://github.com/Dunedan/XpartaMuPP/commit/7a02844f1f236c570414f748f05e3c58613caaf5 Make database initialization non-global
https://github.com/Dunedan/XpartaMuPP/commit/6c2c87b320e09f410a26eafe4351e11285be5f00 Add cmd script for database operations
https://github.com/Dunedan/XpartaMuPP/commit/bd80adda4cb22e8606c368cdc06247b5df74374c Use thread-local sessions for SQLAlchemy
https://github.com/Dunedan/XpartaMuPP/commit/652a5868d24a31c878903c19f08840ae299abfa8 Add the option to specify a database url Allows the use of alternative file locations for the SQLite database as well as completely

Tests:
https://github.com/Dunedan/XpartaMuPP/commit/e0544197b4161a2e451dbb0ab13bf9dc6af34e7c Add first unit tests
https://github.com/Dunedan/XpartaMuPP/commit/f1858ba4ba9f09182406b450807ee8c27fbfbf73 Fix failing tests

Docstrings:
https://github.com/Dunedan/XpartaMuPP/commit/04c1fd97b931d8acd739b5472d3a5fa0250beb25 Add module level docstrings
https://github.com/Dunedan/XpartaMuPP/commit/e9cad9d5b6ad79d5c007db0aa51db926a3f3f84d Improve some docstrings
https://github.com/Dunedan/XpartaMuPP/commit/c69a0fb5704f7fc74e36b82ab6c3981ba45d90df Improved docstrings for XpartaMuPP
https://github.com/Dunedan/XpartaMuPP/commit/482a342d8834574a733326623712f99910cccee8 Improve docstrings
https://github.com/Dunedan/XpartaMuPP/commit/b72609134f4794216e62877574f5e42a1ad26b21 Improve docstrings Ensure all methods have proper docstrings including the notion of arguments and returned values, although those aren't perfectly documented yet.

Textual:
https://github.com/Dunedan/XpartaMuPP/commit/bd6707363d0b4119f764a72578e7dd2c21b9dfe9 Change indentation from 2 spaces to 4 spaces
https://github.com/Dunedan/XpartaMuPP/commit/f8bafb8410398a5fd103e5a106e79f2b9feda0bb Unify quotes - double quotes for every human readable message - single quotes for everything else - exception if something in the quoted string would need quoting
https://github.com/Dunedan/XpartaMuPP/commit/0e2648e6e60b44cae999c75662d96a6e528d5898 Small wording fix
https://github.com/Dunedan/XpartaMuPP/commit/6c1908371635769c69a1cf422b82a23abed0b7a1 Update copyright year

user1 changed the visibility from "user1 (Aaron C)" to "Custom Policy".
user1 edited the summary of this revision. (Show Details)Feb 11 2020, 7:16 PM
user1 updated this revision to Diff 11329.Feb 12 2020, 12:26 AM

added tests folder

user1 edited the summary of this revision. (Show Details)Feb 15 2020, 1:57 PM
user1 changed the visibility from "Custom Policy" to "Public (No Login Required)".Feb 15 2020, 3:36 PM
Stan awarded a token.Feb 15 2020, 5:12 PM
user1 edited the summary of this revision. (Show Details)Feb 15 2020, 6:31 PM
user1 edited the test plan for this revision. (Show Details)
user1 changed the edit policy from "user1 (Aaron C)" to "Subscribers".
user1 added subscribers: elexis, Dunedan.
user1 edited the summary of this revision. (Show Details)Feb 15 2020, 10:15 PM
Itms awarded a token.Feb 16 2020, 10:37 AM
Itms added a subscriber: Itms.Feb 16 2020, 10:40 AM

Do you plan to run the tests manually, or would you be interested in having Jenkins run the tests when a patch touches source/tools/lobbybots?

Itms changed the edit policy from "Subscribers" to "All Users".Feb 16 2020, 11:52 AM

Thanks for keeping up with it. Don't forget to update the copyrights. Pinging @linkmauve in casee he wants to help with XMPP stuff
I'm not sure where it is currently versioned but the path look wrong

setup.py
9 ↗(On Diff #11329)

Do we need to follow 0 A.D.' s versioning scheme? If so 23.1?

tests/test_echelon.py
1 ↗(On Diff #11329)

2020

xpartamupp/stanzas.py
1 ↗(On Diff #11329)

2020

xpartamupp/utils.py
1 ↗(On Diff #11329)

2020

xpartamupp/xpartamupp.py
3 ↗(On Diff #11329)

2020

user1 edited the summary of this revision. (Show Details)Feb 25 2020, 2:48 PM
user1 edited the test plan for this revision. (Show Details)
user1 updated this revision to Diff 11423.EditedFeb 25 2020, 3:24 PM

I used svn mv to move and rename the files. This way allows comparison of the files.
I also updated the copyright year.

user1 updated this revision to Diff 11424.Feb 25 2020, 3:37 PM

Missed some copyrights

user1 edited the summary of this revision. (Show Details)

The following revisions are lost because of this:

r22460 | 2019-07-12 13:40:40 -0400 (Fri, 12 Jul 2019) Fix lineendings. (partially lost)
r21927 | 2018-11-07 12:32:21 -0500 (Wed, 07 Nov 2018) The files in this path were not marked as moved in rP21926...

Not sure what's your take on this, but is there anything which prevents applying the changes from these revisions to this patch as well?

r21926 | 2018-11-07 12:23:56 -0500 (Wed, 07 Nov 2018) Split XpartaMuPP and EcheLOn into separate directories and rename parent folder to "lobbybots" following rP18609.

I believe while the motivationg for this revision was valid, its approach was the wrong one: The decision which bot to run and how to run it, doesn't have to relate to its source code layout, especially as it's common to have multiple different binaries in Python packages. The directory layout as it is right now makes it easier to package both bots as a single Python package in case we desire that later on. So I'd leave this revision lost.

I found at least the following patch that this invalidates:

https://code.wildfiregames.com/D1661

Invalidates as in requires additional changes to this revision after merging? Because to have systemd unit files seems still be to be desirable to me.

In D2630#110507, @Itms wrote:

Do you plan to run the tests manually, or would you be interested in having Jenkins run the tests when a patch touches source/tools/lobbybots?

I'd like to see tests running in Jenkins to ensure new patches don't introduce breaking changes. However keep in mind, that test coverage is currently very, very low.

I haven't looked through the changes in detail yet, but am I right that they're aside from copyright year changes exactly the changes from my git repository?

As said, I haven't looked through all the details yet, but will do so during the next days. So far I'm already really glad that my changes made it to this diff and are probably going to be included in 0ad. Also keep in mind that all these changes were all just meant as a beginning; bringing the code into a state where it's easier to continue developing new features.

Disable TLS option from rP21924 has not been rebased as well.

I would suggest to rename "utils" to LimitedDict or similar, otherwise it invites developers to add many unrelated things. We can see in binaries/data/mods/public/gui/common/functions_utility.js an example of this, and then the difficulty to split it into logically cohesive components accumulates.

rP21926 | 2018-11-07 12:23:56 -0500 (Wed, 07 Nov 2018) Split XpartaMuPP and EcheLOn into separate directories and rename parent folder to "lobbybots" following rP18609.

I believe while the motivationg for this revision was valid, its approach was the wrong one: The decision which bot to run and how to run it, doesn't have to relate to its source code layout, especially as it's common to have multiple different binaries in Python packages. The directory layout as it is right now makes it easier to package both bots as a single Python package in case we desire that later on.
So I'd leave this revision lost.

The directory layout following that commit is:

./EcheLOn/EcheLOn.py
./EcheLOn/ELO.py
./EcheLOn/LobbyRanking.py
./ejabberd_example.yml
./mod_ipstamp/COPYING
./mod_ipstamp/mod_ipstamp.spec
./mod_ipstamp/README.txt
./mod_ipstamp/src/mod_ipstamp.erl
./README.md
./XpartaMuPP/XpartaMuPP.py

Having XPartaMupp.py in a separate folder allows the reader to instantly see that it has no connection to EcheLOn, and that the three files in EcheLOn have no connection to XPartaMuPP.
The actual reason to revert the folder split is not because that folder split isnt providing an advantage but because LimitedSizeDict should be used for both bots?

The proposed folder layout:

xpartamupp/elo.py

is wrong, correct?
Becuase ELO is not XPartaMupp and, nor started using XPartaMupp, correct?

Consider someone would add a third lobby bot, for example a moderation bot (not unlikely since one is already programmed),
then we'd have three different programs with different helper files all in the same folder, that's an antipattern to not split them.

I believe while the motivationg for this revision was valid, its approach was the wrong one: The decision which bot to run and how to run it, doesn't have to relate to its source code layout, especially as it's common to have multiple different binaries in Python packages. The directory layout as it is right now makes it easier to package both bots as a single Python package in case we desire that later on.

We speak about rP18609 or rP21926? If it's rP21926, I don't understand why you say the motivation is correct but the approach was wrong. What the motivation that you refer to? The motivation that I actually had in mind was to split the directories to make it clear that the files are of different binaries.

The directory layout as it is right now makes it easier to package both bots as a single Python package in case we desire that later on.

The two bots in fact already are decoupled fully, so coupling them is actually hiding the fact that they are independent, and when someone looks at one file, he has to investigate the source code to see which of the two bots uses that file instead of seeing directly by the directory structure.
They should be combined only if there is a strong reason, for example a large base of shared code.
Having the two bots in two folders doesnt prevent a python module system, does it?

xpartamupp/echelon.py
167 ↗(On Diff #11424)

(I suppose its not so clean to have this hardcoded in the botcode and a second list of these attributes in another file, already present in the existing code so not the fault of this diff)

xpartamupp/stanzas.py
155 ↗(On Diff #11424)

(each property on an individual line ftw)

xpartamupp/utils.py
20 ↗(On Diff #11424)

(Perhaps LimitedSizeOrderedDict)

Stan added a subscriber: JoshuaJB.Feb 29 2020, 2:02 AM

I would suggest to rename "utils" to LimitedDict or similar, otherwise it invites developers to add many unrelated things. We can see in binaries/data/mods/public/gui/common/functions_utility.js an example of this, and then the difficulty to split it into logically cohesive components accumulates.

I actually envisioned it for housing different kinds of utility functions/classes for the lobby bots, as that's common for such Python projects. I believe that for the forseeable future there won't be much of such utility functionality, so the module won't grow too large. Once it grows too large, splitting it would be of course a reasonable approach and part of refactoring which should then happen. Naming the module differently now wouldn't provide a benefit from my point of view. If going with your proposed name, it'd also make it less explicit and more redundant regarding what's going to be imported (from xpartamupp.limitedDict import LimitedSizeDict instead of from xpartamupp.utils import LimitedSizeDict).

Having XPartaMupp.py in a separate folder allows the reader to instantly see that it has no connection to EcheLOn, and that the three files in EcheLOn have no connection to XPartaMuPP.

Fine for me. If we want to go this way, let's implement it properly with:

  • one common source code directory with one additional subdirectory per bot
  • parent directory containing all the stuff for Python packaging
  • directory/file names all lower case (as it's best practice for Python code layout)

That'd result in a layout like:

./0ad_lobby_bots/mod_ipstamp/…
./0ad_lobby_bots/README.md
./0ad_lobby_bots/lobby_bots/echelon/echelon.py
./0ad_lobby_bots/lobby_bots/echelon/elo.py
./0ad_lobby_bots/lobby_bots/echelon/lobby_ranking.py
./0ad_lobby_bots/lobby_bots/stanzas.py
./0ad_lobby_bots/lobby_bots/utils.py
./0ad_lobby_bots/lobby_bots/xpartamupp/xpartamupp.py
./0ad_lobby_bots/setup.py
./0ad_lobby_bots/tests/…

The proposed folder layout:

xpartamupp/elo.py

is wrong, correct?

To keep things simple I kept xpartamupp as name. The directory layout detailed above is much more reasonable. Currently also the Python package is named XpartaMuPP which is equally inappropriate and should be changed to 0ad_lobby_bots as well.

Splitting the bots later on into different Python packages would be an option, but I see no benefit to pursue this now.

user1 edited the summary of this revision. (Show Details)Mar 2 2020, 11:54 PM
user1 added a comment.Mar 3 2020, 12:00 AM

In D2630#111114, @Dunedan wrote:
Invalidates as in requires additional changes to this revision after merging?

That will need to be changed to be correct after this is applied.

Because to have systemd unit files seems still be to be desirable to me.

Yea I think so too.

I can't apply the patch to current SVN. Am I doing something wrong or is it incomplete?

Stan added a comment.Mar 7 2020, 11:20 AM

I can't apply the patch to current SVN. Am I doing something wrong or is it incomplete?

I believe the patch doesn't have the correct root folder. I assume those files are in source/tools/lobby?

In D2630#111282, @Stan wrote:

I believe the patch doesn't have the correct root folder. I assume those files are in source/tools/lobby?

That is one problem as well, but that's easy to circumvent when applying the patch. However it fails because some of the files it tries to patch aren't there.

Stan added a comment.Mar 7 2020, 12:42 PM

I guess then you have to display each file that fails in this patch as 'raw right' and apply them manually. Maybe @user1 can upload the patch to have the correct root.

user1 edited the summary of this revision. (Show Details)Mar 8 2020, 5:58 PM
user1 added a comment.Mar 8 2020, 6:08 PM

I can't apply the patch to current SVN. Am I doing something wrong or is it incomplete?

Ok. I'm working on this now. I can't apply the patch either.

Stan added a comment.Mar 17 2020, 6:51 PM
In D2630#111403, @user1 wrote:

I can't apply the patch to current SVN. Am I doing something wrong or is it incomplete?

Ok. I'm working on this now. I can't apply the patch either.

Any news?

user1 updated this revision to Diff 11499.Mar 18 2020, 2:17 PM
user1 edited the summary of this revision. (Show Details)

Per elexis' suggestion, this patch doesn't rename or move things anymore.

It should happen that a follow-up revision to this will restructure the directory tree and rename everything as they need to be.

Tests still pass.

Stan added a comment.Mar 18 2020, 2:27 PM
In D2630#111755, @user1 wrote:

Per elexis' suggestion, this patch doesn't rename or move things anymore.

It should happen that a follow-up revision to this will restructure the directory tree and rename everything as they need to be.

Tests still pass.

Why don't you generate the patch in the root https://code.wildfiregames.com/source/0ad/browse/ps/trunk/

instead of https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/tools/lobbybots/

user1 added a comment.EditedMar 18 2020, 3:05 PM

This patch still isn't working correctly. Please standby; I'm having to figure out why svn diff is producing something unusable. Anybody have any thoughts?

Why don't you generate the patch in the root https://code.wildfiregames.com/source/0ad/browse/ps/trunk/

instead of https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/tools/lobbybots/

Because I didn't download the entire source tree.
I can do that for the final if it's necessary. The patch should apply while using patch -p0 from the ../source/tools/lobbybots folder (after I fix it)

Maybe I can fake it as though the patch is from root.. Either I can sed -i s///g to change the paths in the patch or, as I think it could work, I can just create empty parent folders for lobbybots and then run svn diff from the practically empty fake root.. Has anybody tried anything like this?

Stan added a comment.Mar 18 2020, 3:15 PM

Might be line endings. Depending on your OS, it might or might not strip the special chars

user1 updated this revision to Diff 11504.Mar 18 2020, 5:27 PM

The patch was not working properly. I just recopied the files. I'm still not sure why the last attempt produced garbage.

user1 updated this revision to Diff 11631.Apr 4 2020, 7:40 PM
user1 edited the test plan for this revision. (Show Details)

Now based from ps/trunk/

@user1 : Did you verify the code now doesn't just apply but also works with this patch?

user1 added a comment.May 8 2020, 11:22 PM

Yes, it seems to work well. I tested everything I could think of. Unit tests pass. Ratings work. Spotted a couple white space problems in the bots' chat responses.

source/tools/lobbybots/XpartaMuPP/XpartaMuPP.py
231

space after you're

232

space after I'm

source/tools/lobbybots/setup.py
3

During one test there was a hitch where I had to copy stanzas.py and utils.py to my venv/lib/python-3.7/site-packagaes/XpartaMuPP-0.23-py3.7.egg/ folder.

During another test it worked fine. In the venv/lib/python-3.7/site-packagaes/ folder, there was a XpartaMuPP-link file instead of files in a folder. The -link file pointed to the folder it was installed from.

bb added a subscriber: bb.Mon, Oct 12, 8:22 PM

I suppose the lobbybot readme needs some updates too

Running XpartaMuPP.py yells at me for missing the stanzas (and probably utils too)

Running tests give me:

test_add_invalid_1_player1_domain_tld (tests.test_xpartamupp.TestGames)
Test trying to add games with invalid data [with jid='player1@domain.tld', game_data={}]. ... WARNING:root:Received invalid data for add game from 0ad: {}
ok
test_add_invalid_2_player1_domain_tld (tests.test_xpartamupp.TestGames)
Test trying to add games with invalid data [with jid='player1@domain.tld', game_data=None]. ... WARNING:root:Received invalid data for add game from 0ad: None
ok
test_add_invalid_3_player1_domain_tld (tests.test_xpartamupp.TestGames)
Test trying to add games with invalid data [with jid='player1@domain.tld', game_data='']. ... WARNING:root:Received invalid data for add game from 0ad: 
ok
test_change_state (tests.test_xpartamupp.TestGames)
Test state changes of a games. ... ok
test_remove (tests.test_xpartamupp.TestGames)
Test removal of games. ... ok
test_remove_unknown (tests.test_xpartamupp.TestGames)
Test removal of a game, which doesn't exist. ... WARNING:root:Game for jid foo@bar.tld didn't exist
ok
test_failing_connect (tests.test_xpartamupp.TestMain)
Test failing connect to XMPP server. ... ERROR:root:Unable to connect
ok
test_success (tests.test_xpartamupp.TestMain)
Test successful execution. ... ok

======================================================================
FAIL: test_valid_0 (tests.test_echelon.TestArgumentParsing)
Test valid parameter combinations [with cmd_args=[], expected_args=Namespace(database_url='sqlite:/...xdisabletls=False, xserver=None)].
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/.eggs/parameterized-0.7.4-py3.7.egg/parameterized/parameterized.py", line 530, in standalone_func
    return func(*(a + p.args), **p.kwargs)
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/tests/test_echelon.py", line 117, in test_valid
    self.assertEqual(parse_args(cmd_args), expected_args)
AssertionError: Names[59 chars]n='localhost', log_level=30, login='EcheLOn', [83 chars]None) != Names[59 chars]n='lobby.wildfiregames.com', log_level=30, log[97 chars]None)

======================================================================
FAIL: test_valid_1 (tests.test_echelon.TestArgumentParsing)
Test valid parameter combinations [with cmd_args=['--debug'], expected_args=Namespace(database_url='sqlite:/...xdisabletls=False, xserver=None)].
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/.eggs/parameterized-0.7.4-py3.7.egg/parameterized/parameterized.py", line 530, in standalone_func
    return func(*(a + p.args), **p.kwargs)
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/tests/test_echelon.py", line 117, in test_valid
    self.assertEqual(parse_args(cmd_args), expected_args)
AssertionError: Names[59 chars]n='localhost', log_level=10, login='EcheLOn', [83 chars]None) != Names[59 chars]n='lobby.wildfiregames.com', log_level=10, log[97 chars]None)

======================================================================
FAIL: test_valid_2 (tests.test_echelon.TestArgumentParsing)
Test valid parameter combinations [with cmd_args=['--quiet'], expected_args=Namespace(database_url='sqlite:/...xdisabletls=False, xserver=None)].
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/.eggs/parameterized-0.7.4-py3.7.egg/parameterized/parameterized.py", line 530, in standalone_func
    return func(*(a + p.args), **p.kwargs)
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/tests/test_echelon.py", line 117, in test_valid
    self.assertEqual(parse_args(cmd_args), expected_args)
AssertionError: Names[59 chars]n='localhost', log_level=40, login='EcheLOn', [83 chars]None) != Names[59 chars]n='lobby.wildfiregames.com', log_level=40, log[97 chars]None)

======================================================================
FAIL: test_valid_3 (tests.test_echelon.TestArgumentParsing)
Test valid parameter combinations [with cmd_args=['--verbose'], expected_args=Namespace(database_url='sqlite:/...xdisabletls=False, xserver=None)].
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/.eggs/parameterized-0.7.4-py3.7.egg/parameterized/parameterized.py", line 530, in standalone_func
    return func(*(a + p.args), **p.kwargs)
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/tests/test_echelon.py", line 117, in test_valid
    self.assertEqual(parse_args(cmd_args), expected_args)
AssertionError: Names[59 chars]n='localhost', log_level=20, login='EcheLOn', [83 chars]None) != Names[59 chars]n='lobby.wildfiregames.com', log_level=20, log[97 chars]None)

======================================================================
FAIL: test_valid_0 (tests.test_xpartamupp.TestArgumentParsing)
Test valid parameter combinations [with cmd_args=[], expected_args=Namespace(domain='lobby.wildfire...xdisabletls=False, xserver=None)].
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/.eggs/parameterized-0.7.4-py3.7.egg/parameterized/parameterized.py", line 530, in standalone_func
    return func(*(a + p.args), **p.kwargs)
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/tests/test_xpartamupp.py", line 126, in test_valid
    self.assertEqual(parse_args(cmd_args), expected_args)
AssertionError: Namespace(domain='localhost', log_level=30, login='xpartamupp[82 chars]None) != Namespace(domain='lobby.wildfiregames.com', log_level=30, log[96 chars]None)

======================================================================
FAIL: test_valid_1 (tests.test_xpartamupp.TestArgumentParsing)
Test valid parameter combinations [with cmd_args=['--debug'], expected_args=Namespace(domain='lobby.wildfire...xdisabletls=False, xserver=None)].
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/.eggs/parameterized-0.7.4-py3.7.egg/parameterized/parameterized.py", line 530, in standalone_func
    return func(*(a + p.args), **p.kwargs)
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/tests/test_xpartamupp.py", line 126, in test_valid
    self.assertEqual(parse_args(cmd_args), expected_args)
AssertionError: Namespace(domain='localhost', log_level=10, login='xpartamupp[82 chars]None) != Namespace(domain='lobby.wildfiregames.com', log_level=10, log[96 chars]None)

======================================================================
FAIL: test_valid_2 (tests.test_xpartamupp.TestArgumentParsing)
Test valid parameter combinations [with cmd_args=['--quiet'], expected_args=Namespace(domain='lobby.wildfire...xdisabletls=False, xserver=None)].
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/.eggs/parameterized-0.7.4-py3.7.egg/parameterized/parameterized.py", line 530, in standalone_func
    return func(*(a + p.args), **p.kwargs)
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/tests/test_xpartamupp.py", line 126, in test_valid
    self.assertEqual(parse_args(cmd_args), expected_args)
AssertionError: Namespace(domain='localhost', log_level=40, login='xpartamupp[82 chars]None) != Namespace(domain='lobby.wildfiregames.com', log_level=40, log[96 chars]None)

======================================================================
FAIL: test_valid_3 (tests.test_xpartamupp.TestArgumentParsing)
Test valid parameter combinations [with cmd_args=['--verbose'], expected_args=Namespace(domain='lobby.wildfire...xdisabletls=False, xserver=None)].
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/.eggs/parameterized-0.7.4-py3.7.egg/parameterized/parameterized.py", line 530, in standalone_func
    return func(*(a + p.args), **p.kwargs)
  File "/home/Bouke/0ad/SVNcom/0ad/source/tools/lobbybots/tests/test_xpartamupp.py", line 126, in test_valid
    self.assertEqual(parse_args(cmd_args), expected_args)
AssertionError: Namespace(domain='localhost', log_level=20, login='xpartamupp[82 chars]None) != Namespace(domain='lobby.wildfiregames.com', log_level=20, log[96 chars]None)

----------------------------------------------------------------------
Ran 86 tests in 1.157s

FAILED (failures=8)
Test failed: <unittest.runner.TextTestResult run=86 errors=0 failures=8>
error: Test failed: <unittest.runner.TextTestResult run=86 errors=0 failures=8>
source/tools/lobbybots/EcheLOn/ELO.py
1

#!/usr/bin/env python3

90

This test should find his way into the test scripts

source/tools/lobbybots/EcheLOn/EcheLOn.py
74

wondering whether we should set the highest_rating here too to -1 so we don't set it weirdly in the code below

170

Not sure if I agree with dropping the TODO's, anyone implementing this, should come across this code.

326

Appear to be using list(nicks) twice, might be worth storing it

332

must be something wrong here nicks[str(jid)] vs nicks[jid]

351

2**12=4096, so we allow at most 4096 players at the same time, we currently don't exceed 1000, so should be enough for a while. Maybe it should be a global and what happens if we exceed the limit? Probably should add an overflow_callback

542–548

Am I seeing this wrong, or are we not doing anything (but sending some debug message), so should we do this at all?

562
source/tools/lobbybots/XpartaMuPP/XpartaMuPP.py
40

2**7=128, that might be a bit few... Also should add some overflow_callback

174

this comment seems wrong, since we don't do it here

198

same

315–317

Did you ditch those defaults on purpose?

318–319

Seems some arguments changed. is that intended?

source/tools/lobbybots/setup.py
20–22

dupe of the txt above?

41
source/tools/lobbybots/stanzas.py
1

#!/usr/bin/env python3

same below

source/tools/lobbybots/utils.py
39

In case we overflow, we probably need to send some debug message, instead of silently continuing