Page MenuHomeWildfire Games

Correct and complete lobby server readme
ClosedPublic

Authored by elexis on Oct 30 2018, 1:19 PM.

Details

Summary

While following the lobby README.md I noticed a number of things missing that are required for running the service or good practice.

  • Giving bots administrator rights seems unnecessary, as they only need to know the real JID, which can be accomplished by making the room non-anonymous, see rP21720. STUN also seems to imply non-public rooms.
  • Use-policy ejabberd configuration chapter explaining wfg ejabberd configuration practices, so that people don't have to rediscover how to do that and what is needed.
  • Reordered, introduction, more chapters:
    • STUN config missing following rP19703.
    • IPv6 disabling missing following recent ejabberd update and enet still not supporting that, refs #4301.
    • TLS encryption chapter and how to run ejabberd without encryption, refs rP21875, rP21908.
    • Rename wfgbot to xpartamupp23. Explain room version reasoning and default to versioned naming patterns for bot and room names.
    • Support running bots without TLS, refs rP14098.
    • Explain how to configure 0 A.D. with, test the new lobby and distribute the settings.
    • Explain a bit more on the rating bot, refs rP18609.
    • Explain that there should be Terms and Conditions, refs rP15019, rP21908.
    • Restrict registrable usernames serverside, D715 / rP19881
  • Remove COPYING file redundant with license_gpl-2.0.txt, refs rP21720.
Test Plan

Ideally one should purge any local ejabberd config and follow along, reinstall ejabberd and the bots cleanly.

https://dillinger.io/ displays the markup in a rendered version.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 397| 397| 				// Players see colors depending on diplomacy
| 398| 398| 				g_DisplayedPlayerColors[i] =
| 399| 399| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 400|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
|    | 400|+						g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 401| 401| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 402| 402| 					getDiplomacyColor("enemy");
| 403| 403| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 398| 398| 				g_DisplayedPlayerColors[i] =
| 399| 399| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 400| 400| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 401|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
|    | 401|+							g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 402| 402| 					getDiplomacyColor("enemy");
| 403| 403| 
| 404| 404| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 399| 399| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 400| 400| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 401| 401| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 402|    |-					getDiplomacyColor("enemy");
|    | 402|+								getDiplomacyColor("enemy");
| 403| 403| 
| 404| 404| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
| 405| 405| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 655| 655| 					"civ": setStringTags(g_CivData[g_Players[g_ViewedPlayer].civ].Name, { "font": "sans-bold-stroke-14" }),
| 656| 656| 					"hotkey_civinfo": colorizeHotkey("%(hotkey)s", "civinfo"),
| 657| 657| 					"hotkey_structree": colorizeHotkey("%(hotkey)s", "structree")
| 658|    |-			});
|    | 658|+				});
| 659| 659| 	}
| 660| 660| 
| 661| 661| 	// Following gaia can be interesting on scripted maps
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1239|1239| 
|1240|1240| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|    |-		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|    |1242|+			"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|1243| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|1244| 	});
|1245|1245| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1240|1240| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|1242| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|    |-		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|    |1243|+			"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|1244| 	});
|1245|1245| 
|1246|1246| 	let resCodes = g_ResourceData.GetCodes();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|1242| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|1243| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|    |-	});
|    |1244|+		});
|1245|1245| 
|1246|1246| 	let resCodes = g_ResourceData.GetCodes();
|1247|1247| 	for (let r = 0; r < resCodes.length; ++r)
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1740|1740| 	for (let rct of resourcesCounterTypes)
|1741|1741| 		for (let rt of resourcesTypes)
|1742|1742| 			reportObject[rt + rct.substr(9)] = playerStatistics[rct][rt];
|1743|    |-			// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|    |1743|+	// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|1744|1744| 
|1745|1745| 	reportObject.vegetarianFoodGathered = playerStatistics.resourcesGathered.vegetarianFood;
|1746|1746| 	for (let type of unitsClasses)

binaries/data/mods/public/gui/session/session.js
|1067| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1142| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1143| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1144| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

Link to build: https://jenkins.wildfiregames.com/job/differential/757/display/redirect

smiley added a subscriber: smiley.Oct 30 2018, 6:17 PM
smiley added inline comments.
source/tools/XpartaMuPP/EcheLOn.py
792 ↗(On Diff #6953)

No lowercase false?

source/tools/XpartaMuPP/README.md
80 ↗(On Diff #6953)

ejabberd should be already installed

Giving bots administrator rights seems unnecessary, as they only need to know the real JID, which can be accomplished by making the room non-anonymous, see rP21720. STUN also seems to imply non-public rooms.

(That's the only part that introduces a behavior change from what we have configured on the server.)

source/tools/XpartaMuPP/EcheLOn.py
792 ↗(On Diff #6953)

For consistency with python?

source/tools/XpartaMuPP/README.md
80 ↗(On Diff #6953)

thx

86 ↗(On Diff #6953)

(SleekXMPP sounds like it requires pip https://github.com/fritzy/SleekXMPP)

elexis added inline comments.Oct 31 2018, 11:56 AM
source/tools/XpartaMuPP/README.md
1 ↗(On Diff #6953)

I guess we can make that Pyrogenesis Multiplayer Lobby Setup

smiley added inline comments.Oct 31 2018, 2:11 PM
source/tools/XpartaMuPP/EcheLOn.py
792 ↗(On Diff #6953)

Python doesn't allow false. Unlike most languages. I mentioned it because most cmd-line applications are case insensitive for inputs like these. Y/N commonly. Or am I wrong? TBH this is pretty rare.

source/tools/XpartaMuPP/README.md
48 ↗(On Diff #6953)

Package*
Also, debian-based is more inclusive.

smiley added inline comments.Oct 31 2018, 5:46 PM
source/tools/XpartaMuPP/README.md
86 ↗(On Diff #6953)

1.3.4 is available in 16.4 repos. Not necessary probably.

user1 added a reviewer: user1.Nov 1 2018, 12:01 AM

Links are not being rendered because you're putting them in codeblocks instead. Do you want me to mark them?

There are alot of newlines and paragraph breaks that don't need to be there. Do you want me to mark them?

source/tools/XpartaMuPP/EcheLOn.py
775 ↗(On Diff #6953)

Maybe you can use boolean handling here.

from: https://docs.python.org/3/library/optparse.html#handling-boolean-flag-options

Flag options—set a variable to true or false when a particular option is seen —are quite common. optparse supports them with two separate actions, store_true and store_false.

You could use action='store_false' with an option called something like --disable-tls. You can set the default value to True instead of "True" . This way we avoid the ambiguity issues that smiley mentioned.

-edit-
Now I'm thinking you might want to change the name of the variable to xdisabletls instead of xusetls and use action='store_true' and default=False

792 ↗(On Diff #6953)

if xmpp.connect((opts.xserver, 5222), True, not opts.xdisabletls):

source/tools/XpartaMuPP/README.md
11 ↗(On Diff #6953)

Which goes in the parentheses, the proper name of the thing or the description?

Like this:

XMPP server (ejabberd)
Gamelist bot (XPartaMuPP)
Ratings bot (EcheLOn)

20 ↗(On Diff #6953)

you have to =make= some decisions

55 ↗(On Diff #6953)

You could use MD link syntax for URLs instead of a code block.

from: https://daringfireball.net/projects/markdown/syntax#link

To create an inline link, use a set of regular parentheses immediately after the link text’s closing square bracket. Inside the parentheses, put the URL where you want the link to point, along with an optional title for the link, surrounded in quotes.

The line could be like this:

* Install `ejabberd` using the following command. Alternatively see [the ejabberd installation instructions](https://docs.ejabberd.im/admin/installation/).

Rendered it would look like this:

122 ↗(On Diff #6953)

Considering the paragraph that follows this line, I thought it makes sense to put all ejabberd.yml editing into one section.

## 2. Configure ejabberd
The ejabberd configuration in the remainder of this document is performed by editing `/etc/ejabberd/ejabberd.yml`.

Then make a sub-section for the different domains of configuration there are to address..

### 2.1. Configure mod_ipstamp
...
### 2.2. Configure ejabberd connectivity
...
### 2.3. Configure ejabberd use policy
...
### 2.4. Lobby bot configuration
127 ↗(On Diff #6953)

### 2.1. Configure mod_ipstamp

142 ↗(On Diff #6953)

### 2.2. Configure ejabberd connectivity

146 ↗(On Diff #6953)

#### 2.2.1. Disable IPv6

154 ↗(On Diff #6953)

#### 2.2.2. Enable STUN

155 ↗(On Diff #6953)

others' games (plural possessive)

Commas aren't needed here. If you think the sentence is too long you can split it something like this:

ejabberd and 0 A.D. support the STUN protocol. This allows players to connect to each others' games even if the host did not configure...

362 ↗(On Diff #6953)

## 3. Setup lobby bots

364 ↗(On Diff #6953)

### 3.1. Register lobby bot accounts

380 ↗(On Diff #6953)

This entire section might be moved to the ## 2. Configuring ejabberd section.
### 2.4. Lobby bot configurations

430 ↗(On Diff #6953)

### 3.2. Running XpartaMuPP - XMPP Multiplayer Game Manager

449 ↗(On Diff #6953)

### 3.3.

485 ↗(On Diff #6953)

## 4.

487 ↗(On Diff #6953)

This sentence sounds a little weird to me.
I'm reading it as the 0 A.D. client is already configured to connect to the new lobby. Instead it should sound like the 0 A.D. client is now going to be configured.

The 0 A.D. client is now going to be configured to connect to the new Multiplayer Lobby.

492 ↗(On Diff #6953)

### 4.1.

494 ↗(On Diff #6953)

the local =user's= 0 A.D configuration path

498 ↗(On Diff #6953)

Add =the= following settings

513 ↗(On Diff #6953)

### 4.2.

521 ↗(On Diff #6953)

### 4.3.

540 ↗(On Diff #6953)

The terms should =be= published online, so users can save and print them.

547 ↗(On Diff #6953)

### 4.4.

553 ↗(On Diff #6953)

missing the closing code block ` ?

This could use MD link syntax instead.

source/tools/XpartaMuPP/XpartaMuPP.py
643 ↗(On Diff #6953)

Boolean handling could be used to avoid ambiguity.

660 ↗(On Diff #6953)

if xmpp.connect((opts.xserver, 5222), True, not opts.xdisabletls):

smiley added inline comments.Nov 1 2018, 10:28 AM
source/tools/XpartaMuPP/README.md
55 ↗(On Diff #6953)

IMO, just putting the raw link is better. It would be parsed as a link and users would know what they are clicking.

elexis added a comment.Nov 1 2018, 7:33 PM
In D1659#65685, @user1 wrote:

Links

Thanks, I didn't find the`https://...` format until now.

There are alot of newlines and paragraph breaks that don't need to be there. Do you want me to mark them?

I found 2 occurrences of double-empty-lines. The other newlines were intended and polished so it looks good in the way eclipse displays it. So yes, if there is a specific whitespace issue, pointing it out would help.

I would appreciate explicit feedback on the stripping of bot muc admin access rights.

source/tools/XpartaMuPP/EcheLOn.py
775 ↗(On Diff #6953)

Thanks, that's better!

source/tools/XpartaMuPP/README.md
1 ↗(On Diff #6953)

Perhaps "0 A.D. / Pyrogenesis" in for human-readable labels, "pyrogenesis" in identifiers used in services and files

11 ↗(On Diff #6953)

The proper name identifies subject, the description is secondary, so should be read afterwards.
I guess the ejabberd entry must be switched then.

Or if we want the description first, then it should be a colon instead of parentheses.

11 ↗(On Diff #6953)

(Also to be as picky as programs using filenames, XpartaMuPP instead of XPartaMuPP)

55 ↗(On Diff #6953)

The disadvantage of [the ejabberd installation instructions](https://docs.ejabberd.im/admin/installation/) is that the readflow is obstructed if people look at the code; and people who look at the rendered version don't see the link until hovering it. So IMO <link> is preferable.

122 ↗(On Diff #6953)

While it is true that the chapters share the same file to be edited, and that it would be better to say "edit all settings of this chapter in this file" rather than "edit all settings of the rest of the document in this file".
However the purposes for which the file is edited differ significantly, and the chapters are very long, so (for my taste) I'd prefer to have them single chapters. (It will also feel a bit more like progress to the reader if it's divided into 5 steps that are about equal of work, rather than one step that is very long fwiw.) (Notice the connectivity chapter even goes to ####level.)

# 0 A.D. / Pyrogenesis Multiplayer Lobby Setup
## Service description
## Service choices
#### Choice: Domain Name
#### Choice: Rating service
#### Choice: Pyrogenesis version compatibility
## 1. Install dependencies
### 1.1 Install ejabberd
### 1.2 Install python3 and SleekXmpp
### 1.3 Install ejabberd ipstamp module
## 2. Configure ejabberd mod_ipstamp
## 3. Configure ejabberd connectivity
### 3.1 Disable IPv6
### 3.2 Enable STUN
### 3.3 Enable keep-alive
### 3.3 Disable unused services
### 3.4 Enable TLS encryption
#### Choice A: No encryption
#### Choice B: Self-signed certificate
#### Choice C: Let's Encrypt certificate
## 3. Configure ejabberd use policy
### Optional use policies
## 4. Setup lobby bots
### 4.1 Register lobby bot accounts
### 4.2 Authorize lobby bots with ejabberd
### 4.3 Running XpartaMuPP - XMPP Multiplayer Game Manager
### 4.4 Running EcheLOn - XMPP Multiplayer Rating Manager
## 5. Configure Pyrogenesis for the new Multiplayer Lobby
### 5.1 Local Configuration
### 5.2 Test the Multiplayer Lobby
### 5.3 Terms and Conditions
### 5.4 Distribute the configuration
487 ↗(On Diff #6953)

Yes

elexis added inline comments.Nov 1 2018, 7:36 PM
source/tools/XpartaMuPP/mod_ipstamp/COPYING
1 ↗(On Diff #6953)

@Dunedan you agree that your copy can be nuked or should we add it to all the other folders too?

elexis updated this revision to Diff 6959.Nov 1 2018, 7:37 PM

Some phrasing updates, link syntax.

Vulcan added a comment.Nov 1 2018, 7:54 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 397| 397| 				// Players see colors depending on diplomacy
| 398| 398| 				g_DisplayedPlayerColors[i] =
| 399| 399| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 400|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
|    | 400|+						g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 401| 401| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 402| 402| 					getDiplomacyColor("enemy");
| 403| 403| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 398| 398| 				g_DisplayedPlayerColors[i] =
| 399| 399| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 400| 400| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 401|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
|    | 401|+							g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 402| 402| 					getDiplomacyColor("enemy");
| 403| 403| 
| 404| 404| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 399| 399| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 400| 400| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 401| 401| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 402|    |-					getDiplomacyColor("enemy");
|    | 402|+								getDiplomacyColor("enemy");
| 403| 403| 
| 404| 404| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
| 405| 405| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 655| 655| 					"civ": setStringTags(g_CivData[g_Players[g_ViewedPlayer].civ].Name, { "font": "sans-bold-stroke-14" }),
| 656| 656| 					"hotkey_civinfo": colorizeHotkey("%(hotkey)s", "civinfo"),
| 657| 657| 					"hotkey_structree": colorizeHotkey("%(hotkey)s", "structree")
| 658|    |-			});
|    | 658|+				});
| 659| 659| 	}
| 660| 660| 
| 661| 661| 	// Following gaia can be interesting on scripted maps
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1239|1239| 
|1240|1240| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|    |-		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|    |1242|+			"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|1243| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|1244| 	});
|1245|1245| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1240|1240| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|1242| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|    |-		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|    |1243|+			"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|1244| 	});
|1245|1245| 
|1246|1246| 	let resCodes = g_ResourceData.GetCodes();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|1242| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|1243| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|    |-	});
|    |1244|+		});
|1245|1245| 
|1246|1246| 	let resCodes = g_ResourceData.GetCodes();
|1247|1247| 	for (let r = 0; r < resCodes.length; ++r)
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1740|1740| 	for (let rct of resourcesCounterTypes)
|1741|1741| 		for (let rt of resourcesTypes)
|1742|1742| 			reportObject[rt + rct.substr(9)] = playerStatistics[rct][rt];
|1743|    |-			// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|    |1743|+	// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|1744|1744| 
|1745|1745| 	reportObject.vegetarianFoodGathered = playerStatistics.resourcesGathered.vegetarianFood;
|1746|1746| 	for (let type of unitsClasses)

binaries/data/mods/public/gui/session/session.js
|1067| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1142| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1143| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1144| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

Link to build: https://jenkins.wildfiregames.com/job/differential/761/display/redirect

smiley added a comment.Nov 1 2018, 8:35 PM

Apart from these two things and also two point 3 being present, it rendered without anything weird in VS Code.

source/tools/XpartaMuPP/README.md
16 ↗(On Diff #6959)

*

280 ↗(On Diff #6959)

No need to say (optional), title already did.

332 ↗(On Diff #6959)

Missed link.

user1 added inline comments.Nov 1 2018, 8:40 PM
source/tools/XpartaMuPP/README.md
55 ↗(On Diff #6953)

I like that, too.
Best to enclose the automatic links in < > .

elexis added inline comments.Nov 3 2018, 12:39 AM
source/tools/XpartaMuPP/README.md
329 ↗(On Diff #6959)

Stupid f'ing ejabberd specification doesn't tell one that /lib/systemd/system/ejabberd.service by default sets ProtectHome=true which finally explained the file permission errors for chatlogs I was chasing in circles for hours..

A folder named XpartaMuPP should not contain EcheLOn.
I propose to use a separate folder for XPartaMupp and EcheLOn. The directory is becoming quite messy. Not going to upload a diff with moved files, but I do propose:

source/tools/lobbybots/XpartaMupp/XpartaMuPP.py
source/tools/lobbybots/XpartaMupp/XpartaMuPP.log
source/tools/lobbybots/XpartaMupp/XpartaMuPP.pid
source/tools/lobbybots/EcheLOn/EcheLOn.py
source/tools/lobbybots/EcheLOn/EcheLOn.log
source/tools/lobbybots/EcheLOn/EcheLOn.pid
source/tools/lobbybots/EcheLOn/ELO.py
source/tools/lobbybots/EcheLOn/LobbyRanking.py
source/tools/lobbybots/EcheLOn/lobby_rankings.sqlite3
source/tools/lobbybots/mod_ipstamp/mod_ipstamp.spec
source/tools/lobbybots/mod_ipstamp/src/mod_ipstamp.erl
source/tools/lobbybots/systemd/pyrogenesis-lobbybots.service
source/tools/lobbybots/systemd/pyrogenesis-lobbybots.sh
source/tools/lobbybots/README.md

It would save the users a significant amount of time if we provided a sample ejabberd configuration, examples/ejabberd.yml. I suspect that wouldnt be maintained by us, but perhaps it's still the right thing to do or worth it.

In D1659#65858, @elexis wrote:

A folder named XpartaMuPP should not contain EcheLOn.
I propose to use a separate folder for XPartaMupp and EcheLOn. The directory is becoming quite messy. Not going to upload a diff with moved files, but I do propose:

My vision was always to have them as a proper Python package, eventually simply installable via pip (therefore hosted on Pypi as well). As they share a lot of code I believe it makes a lot of sense to keep them as a single package. Actually in my fork (https://github.com/Dunedan/XpartaMuPP) I already have them as Python package (though without systemd unit files).

Aside from that I'd argue that this is out of scope for this diff.

elexis added a comment.Nov 4 2018, 5:53 PM

My vision was always to have them as a proper Python package, eventually simply installable via pip (therefore hosted on Pypi as well).
As they share a lot of code I believe it makes a lot of sense to keep them as a single package. Actually in my fork (https://github.com/Dunedan/XpartaMuPP) I already have them as Python package (though without systemd unit files).

I'd argue that this is out of scope for this diff.

Sure, but the way the file structure is going to be will influence the way the systemd script will be writte, which will influence the way this README will be written, which may influence the way the README should be written now.

Perhaps it makes sense to have the bots in the same python installation package.
But am I mistaken or didn't you mention once to unite the two bots in a single program, i.e. to to revert #3022 / rP18609 / P18610?
It's quite benefitial to the admins that bots can run and be stopped or restarted independently, it's more fault tolerant (one process dying/lagging/being exploited doesn't (or doesn't as much) kill/lag/exploit the other bot.
There is a moderation bot in place and we have a use case for at least one more bot. These bots should also run independently and be located in separate directories.
Right now, XPartaMuPP and EcheLOn don't share any code, but write a database, PID, LOG (in the future possibly cache) files to the current dir, so it only makes sense to separate the two independent programs into two distinct directories, right?

The only "behavior change" in this diff may also be considered unrelated from the rest of the diff:
I hope we all agree that one should give an xmpp user account only muc administrative access if there is a purpose for administrative access.

From https://xmpp.org/extensions/xep-0045.html#admin:

Admin Use Cases:

  1. ban a user from the room
  2. modify the list of users who are banned from the room
  3. grant or revoke membership
  4. modify the member list
  5. grant or revoke moderator status
  6. modify the list of moderators

The gamelist bot nor the rating bot serves any of these purposes (the moderation bot does).

Solely making them muc administrators only to show an @ in front of the nickname in the 0ad gui seems a bit irresponsible,
because if the problem is that users should be informed of these things being bots, then it should be displayed with a proper description D1663 (for which administrative access is also not needed).
Since the gamelist bot needs to see the real JID, there is only the choice between giving that bot administrative muc access or making the room non-anonymous.

So one has to compare the effects of giving one of the bots administrative access or revealing the real JID to everyone:
Practically the real JIDs are already known to everyone on the WFG lobby, because server-to-server communication is disabled, since it is not WFGs policy to make usernames anonymous and since STUN reveals the real JID of hosts (rP19703#inline-2513) without warning the client that that is the case (https://xmpp.org/extensions/xep-0045.html#enter-nonanon).
Since none of the use cases are covered, that's a negative for giving the administrative access.
The only use case for hiding real JIDs can be to allow a user to connect anonymously.
So I can only conclude that EcheLOn should never have administrative access, that XPartaMuPP should not have administrative access on Wildfire Games server, and that README.md can explain both options and recommend the administratir to chose dependong on the anonymity policy and STUN support. (I shouldn't have to lay this out for the N'th time here or in the dedicated room until the addresses respond, articulate their point and refute the arguments)

source/tools/XpartaMuPP/mod_ipstamp/COPYING
1 ↗(On Diff #6953)

@Dunedan you agree that your copy can be nuked or should we add it to all the other folders too?

elexis updated this revision to Diff 6967.Nov 6 2018, 4:16 PM

Rewrite realJID part to be a choice.
Fix few format issues (missing *)
Keep COPYING because whatever.

elexis edited the test plan for this revision. (Show Details)Nov 6 2018, 4:21 PM
user1 accepted this revision.Nov 6 2018, 4:49 PM

The markdown renders well and the document reads well. Unfortunately I did not get a chance to go through all the steps on a fresh install of ejabberd.
One area that I tested on ejabberd was the mod_ipstamp installation instructions and those worked out very well and saved me some time. TY elexis. :)
It seems obviously a big improvement over what we had before.

This revision is now accepted and ready to land.Nov 6 2018, 4:49 PM
In D1659#65867, @elexis wrote:

Perhaps it makes sense to have the bots in the same python installation package.

Yes, that's what I think as well.

But am I mistaken or didn't you mention once to unite the two bots in a single program, i.e. to to revert #3022 / rP18609 / P18610?

That was my idea early on, but as there was quite some resistance against that I didn't pursue it any further.

It's quite benefitial to the admins that bots can run and be stopped or restarted independently, it's more fault tolerant (one process dying/lagging/being exploited doesn't (or doesn't as much) kill/lag/exploit the other bot.

Agreed.

Right now, XPartaMuPP and EcheLOn don't share any code, but write a database, PID, LOG (in the future possibly cache) files to the current dir, so it only makes sense to separate the two independent programs into two distinct directories, right?

Why are pids and logs in the directory with the code and not handled by systemd?

So I can only conclude that EcheLOn should never have administrative access, that XPartaMuPP should not have administrative access on Wildfire Games server, and that README.md can explain both options and recommend the administratir to chose dependong on the anonymity policy and STUN support. (I shouldn't have to lay this out for the N'th time here or in the dedicated room until the addresses respond, articulate their point and refute the arguments)

Nothin against that from my side. That's something which could change with pubsub though.

Why are pids and logs in the directory with the code and not handled by systemd?

The pid/logfile is handled by systemd.
There is one logfile and pidfile per bot, at least one bot per room, but one systemd service handling all bots.
I don't see a better place to put them rather than the directory for that room for that bot, do you know one?
(Every bot instance needs an own code copy to provide the correct version for everyroom).

So I can only conclude that EcheLOn should never have administrative access, that XPartaMuPP should not have administrative access on Wildfire Games server, and that README.md can explain both options and recommend the administratir to chose dependong on the anonymity policy and STUN support. (I shouldn't have to lay this out for the N'th time here or in the dedicated room until the addresses respond, articulate their point and refute the arguments)

Nothin against that from my side. That's something which could change with pubsub though.

Are you sure? As far as I see it only asks users not connect with anonymous SASL, but may even work with non-real JIDs.

Thanks for the answers Dunedan and the review user1!

elexis updated the Trac tickets for this revision.Nov 7 2018, 12:21 PM
elexis edited the summary of this revision. (Show Details)
elexis added a comment.Nov 7 2018, 1:12 PM
  • max_stanza_size to prevent disconnects was added (present in our ejabberd.yml, but marked by scythetwirler as 'experimental'. Given that gamelists carry a lot of data, including a playerlist, lobby admins should indeed be prevented from setting a too low value, if they reduce the default Infinity)
  • user_regexp (from rP19881) was finally added. Can be tested easily by calling setFeedback(""); in updateFeedback() of register.js and setting the registration_timeout: infinity in ejabberd.yml. user1 tested it a bit too. The reexp is the same as used in that commit clientside. if the clientside nickname check is disabled, it returns "forbidden" after attempting to register an account with an invalid nickname.
  • From whatI gather here and in the mod room, user1 and Dunedan don't object the directory split (that IMO should be considered a consequence of the botsplit commit in rP18609)
Vulcan added a comment.Nov 7 2018, 4:20 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/767/

Itms added a comment.Nov 7 2018, 4:31 PM
In D1659#65921, @Vulcan wrote:

Build failure - The Moirai have given mortals hearts that can endure.

Sorry, it tried to build it after pulling in rP21924.

This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Nov 7 2018, 4:59 PM
elexis marked an inline comment as done.Nov 7 2018, 5:05 PM
elexis added inline comments.
source/tools/XpartaMuPP/mod_ipstamp/COPYING
1 ↗(On Diff #6953)

Then again why is this file in mod_ipstamp and not in XpartaMupp...

elexis marked an inline comment as done.Nov 7 2018, 5:21 PM
elexis added inline comments.
source/tools/XpartaMuPP/mod_ipstamp/COPYING
1 ↗(On Diff #6953)

Thought I had read D1208#49251, usually != mandatory, I guess the COPYING file is typical for modules distributed separately. whatever, I'll leave it.

In D1659#65912, @elexis wrote:

The pid/logfile is handled by systemd.
There is one logfile and pidfile per bot, at least one bot per room, but one systemd service handling all bots.
I don't see a better place to put them rather than the directory for that room for that bot, do you know one?
(Every bot instance needs an own code copy to provide the correct version for everyroom).

That sounds just wrong:

  • By default systemd handles the location of logs and pids on its own. No need to pollute the directory containing the code with it.
  • There should be two different systemd service files, one for XpartaMuPP and one for EcheLOn and as many instances of each service, as there are lobbies (different code location should also be no issue with this approach). See e.g. https://coreos.com/os/docs/latest/getting-started-with-systemd.html#instantiated-units

Btw: Where are these systemd service files? I have neither found them in here nor in SVN.

We need two bots per room, not two bots per server.
The bots host different game lists, run different code and use different logins.
So either we have systemd service that handles all bots, or we add a lot of systemd service.
The latter has the disadvantage of being a bit more cumbersome to configure, creating a new *.service file and *.sh file for each bot of each lobby. Restarting the bots would require one to run one command per bot per room instead of only one command.

Btw: Where are these systemd service files? I have neither found them in here nor in SVN.

leper never committed his init.d script and doesn't grant to redistribute it as GPL v2+, that's the whole point. D1661.

In D1659#65939, @elexis wrote:

We need two bots per room, not two bots per server.

Please re-read my comment. That's what I wrote as well. I just used the term lobby instead of room.

Btw: Where are these systemd service files? I have neither found them in here nor in SVN.

leper never committed his init.d script and doesn't grant to redistribute it as GPL v2+, that's the whole point. D1661.

Ah ok, I wasn't aware of D1661.