Changes the file to markdown.
Details
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
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.
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. |
source/tools/XpartaMuPP/README.md | ||
---|---|---|
41 | Yup. Also, there's additional include statements that need to be added to the mod_ipstamp.erl file. |
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.
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.
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. |
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.
I guess it's ok to assume sudo is installed if it's installed by default on the most popular Debian-based distributions.
Added sudo to several lines and rephrased lines 90-96.
Note that sudo is already assumed in the previous version of the README.
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.
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. |
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?
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.
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.
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.)
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.
@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.