Page MenuHomeWildfire Games

Convert Lobby README to markdown
AbandonedPublic

Authored by user1 on Mar 30 2017, 9:08 PM.

Details

Summary

Changes the file to markdown.

Test Plan

Verify steps work to setup lobby.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Vulcan added a subscriber: Vulcan.Mar 31 2017, 12:24 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/648/ for more details.

scythetwirler planned changes to this revision.Mar 31 2017, 7:11 PM
elexis edited edge metadata.

Adding user1 since he setup all the bots locally.

user1 added inline comments.Apr 14 2017, 2:39 AM
source/tools/XpartaMuPP/README.md
41

In more recent versions of ejabberd the config file is now /etc/ejabberd/ejabberd.yml

The syntax is different so many of the lines from here down need to be updated.

scythetwirler added inline comments.Apr 14 2017, 9:29 PM
source/tools/XpartaMuPP/README.md
41

Yup. Also, there's additional include statements that need to be added to the mod_ipstamp.erl file.

user1 requested changes to this revision.May 3 2017, 6:50 PM

Can we add to the title of this something like 'and convert to markdown'.
Then let's keep this revision focused on the conversion and feature some simple text cleanup if necessary. When we decide the markdown is good we can get this closed.
We can then address the other concerns about how it should read with newer versions of ejabberd config in a separate diff. Also note upcoming necessary change related to #4540 if that issue's latest proposal is adopted.

user1 retitled this revision from Cleanup Lobby README to Convert Lobby README to markdown.May 16 2017, 3:13 PM
user1 accepted this revision.May 16 2017, 4:33 PM

I tested the markdown output and it looks good. The README instructions are correct for ejabberd version 2.x but not later. I think we can start work on that in a trac ticket discussing changes (code and readme changes) needed for upgrading ejabberd.

Also I think any readme change we might need related to #4540 can be included in the revision corresponding to that issue.

user1 requested changes to this revision.May 16 2017, 7:32 PM

Now I've just looked closely at the textual changes made and found a couple issues.

There are places that had used # and $ to indicate invoking commands as root or not. These were all changed to $ and this is erroneous unless we indicate sudo on the necessary lines (which is probably preferable to the way it was before anyway).

Elexis had suggested that I could commandeer this revision and I don't mind doing that if you'd like.

source/tools/XpartaMuPP/README.md
14

sudo

18

sudo

32

No $ here; it's confusing. This is not being entered into a shell, it is telling the user what to change inside mod_ipstamp.erl

37

$ sudo make install

45

$ sudo

63

$ sudo

67

$ sudo

91

See comment on line 92.

93

I just noticed the rewording of this line and I wonder if it might be appropriate to keep the original line intact especially if we are going to add python3-sqlalchemy-utils.

The original line had

If you would like to run the leaderboard database,
    $ sudo apt-get install python3-sqlalchemy

Adding the additional dependency to the old line is very straight-forward and highly readable(copy-pasteable). And we already assume the user is on a debian flavour of linux on L13 and L17. But the alternative doesn't seem too bad either:

If you would like to run the leaderboard database, you will need to install SQLAlchemy and SQLAlchemy-Utils for Python 3.

elexis had suggested that I could commandeer this revision and I don't mind doing that if you'd like.

(should be done this release IMO since we plan to add 2 new ejabberd plugins in D364 and #4540)

leper resigned from this revision.May 17 2017, 6:38 PM

Why assume that sudo is installed or configured if there is quite standard syntax to denote that?

That said most of the previous contents should have been valid markdown already (though using the slightly more verbose syntax. Also I thought it was by now obvious that I don't care about the lobby anymore.

user1 commandeered this revision.May 18 2017, 1:37 AM
user1 edited reviewers, added: scythetwirler; removed: user1.

I guess it's ok to assume sudo is installed if it's installed by default on the most popular Debian-based distributions.

user1 updated this revision to Diff 2009.May 18 2017, 1:52 AM

Added sudo to several lines and rephrased lines 90-96.

Note that sudo is already assumed in the previous version of the README.

user1 updated this revision to Diff 2123.May 22 2017, 3:30 PM

This version uses pound sign (#) instead of sudo.

scythetwirler edited edge metadata.May 24 2017, 6:56 PM

Have you been able to successfully run newer versions of Ejabberd?

user1 added a comment.May 25 2017, 3:39 PM

Yes I'm running ejabberd 16.01.

@Itms I don't plan any further changes to this. I think any other changes, like for example supporting recent versions of ejabberd, can be included in a future revision.

Itms added a comment.Jun 11 2017, 10:10 PM

The Mardown changes look good to me (except some nitpicking about localhost).

However, I can't manage to build the erlang module on a fresh Debian Stretch install. The ejabberd files are put into /usr/lib/x86_64-linux-gnu/ejabberd-16.09/ (see https://packages.debian.org/stretch/amd64/ejabberd/filelist) so I created a symlink to that at /usr/lib/ejabberd. After that I get

undefined macro 'INFO_MSG/2'
undefined macro 'INFO_MSG/2'
undefined macro 'INFO_MSG/2'
function start/2 undefined
function stop/1 undefined

I don't really know how to fix that (I also tried to put the complete path into the Makefile but I get the same build errors.

I think it would be nice to give information about that issue, especially when we look into migrating the lobby server to a more recent Debian version. In the meantime I couldn't test the subsequent instructions but gameboy manages to make the thing work so I suppose it's not too broken.

source/tools/XpartaMuPP/README.md
5

Make localhost verbatim.

30

Same here.

user1 updated this revision to Diff 2531.Jun 12 2017, 6:36 PM

We only support ejabberd 2.x for the time being. I've indicated that near the beginning of the document and I inform the user that it's available in Debian wheezy.

@Itms INFO_MSG is defined in ejabberd.hrl in ejabberd 2.x. Is it possible for you to test with wheezy?

user1 marked 2 inline comments as done.Jun 12 2017, 6:36 PM
Itms requested changes to this revision.Jun 12 2017, 8:11 PM
In D280#25781, @user1 wrote:

We only support ejabberd 2.x for the time being. I've indicated that near the beginning of the document and I inform the user that it's available in Debian wheezy.

Oh, alright, in that case I would rephrase the sentence so it's clearer (right now it sounds like 2.x or higher, and the mention to wheezy sounds like 'we even support oldstable' ?). I could test in wheezy, but I'm too lazy to fire up a VM with such an old release. Hopefully we will support recent versions of ejabberd soon and we will update the build steps if something changes.

I'll just review the text once you make the notice clearer.

This revision now requires changes to proceed.Jun 12 2017, 8:11 PM
user1 updated this revision to Diff 2540.Jun 12 2017, 9:52 PM
user1 edited edge metadata.

Now it's clear we only support the old version of ejabberd but it's also kind of implied that we will soon support newer versions.

I guess I've done most of the work needed to support ejabberd 16. Unfortunately I don't really know anything about erlang and little about python. I just followed compiler/runtime errors and debugged until I had a working setup. I'm guessing it is ugly to the eyes of someone who really knows what they're looking at.

I have been working on preparing a suitable diff for those changes so perhaps soon I can post it to phab and let the critique pour in. Maybe post-release when things die down a bit.

elexis added a comment.Jul 7 2017, 5:54 PM

After the readme was converted, we should add some more entries mentioned in #4671

The README instructions are correct for ejabberd version 2.x but not later. I think we can start work on that in a trac ticket discussing changes (code and readme changes) needed for upgrading ejabberd.

@user1: Ping me, if you have concrete plans to upgrade ejabberd to a recent version. I already have a local working setup with adjusted mod_ipstamp and so (I just haven't written all details down yet).

Dunedan if you have any code updates, please upload them.

(user1 does not have access to the lobby server.)
(Also we were wondering if it would be easier to upgrade or do a new installation from scratch.)

In D280#47719, @elexis wrote:

Dunedan if you have any code updates, please upload them.

Having this patch applied would be nice before doing that, as it'll introduce some changes to the README as well.

I feel like this patch is in some kind of dead-lock right now.

@user1 : Do you mind adding the clarification requested by @Itms, that the bots currently don't work with recent ejabberd version, so we can get this patch merged?

I'd like to open a follow-up patch, with everything necessary (updated mod_ipstamp and documentation) to run the bots with recent ejabberd versions, but I'm waiting for the README being Markdown to do that, so I don't have to change the format of the documentation all over again.

Dunedan accepted this revision.Jan 7 2018, 8:51 PM

@Itms: Apparently there is already a note in the README about the supported ejabberd versions. Enough for you to continue to review or even approve this patch?

Given that these are mainly formatting changes, it's a no brainer for me, so I'll approve it for now already.

Itms abandoned this revision.Feb 12 2018, 7:19 PM

This is superseded by D1208. Thanks for bootstrapping the update/cleanup work!