Bug 241619 - net-im/prosody: Refactor + Lua 5.2
Summary: net-im/prosody: Refactor + Lua 5.2
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Kurt Jaeger
URL:
Keywords:
Depends on: 241488 243460
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-31 17:29 UTC by Thomas Morper
Modified: 2020-03-04 19:48 UTC (History)
2 users (show)

See Also:
thomas: maintainer-feedback-


Attachments
svn diff to update net-im/prosody to 0.11.3 + lua 5.2 (6.67 KB, patch)
2019-10-31 17:29 UTC, Thomas Morper
no flags Details | Diff
Poudriere build log (25.68 KB, text/plain)
2019-11-01 06:37 UTC, David Thiel
no flags Details
svn diff, corrected version (8.62 KB, patch)
2019-11-04 02:01 UTC, Thomas Morper
no flags Details | Diff
svn diff, including new pid file location (9.10 KB, patch)
2019-11-20 23:38 UTC, Thomas Morper
no flags Details | Diff
net-im/prosody 0.11.2 to 0.11.3 diff (11.62 KB, patch)
2019-12-22 14:30 UTC, Sascha Biberhofer
no flags Details | Diff
Prosody 0.11.3 refactoring patch (9.32 KB, patch)
2020-01-17 16:29 UTC, Thomas Morper
no flags Details | Diff
UPDATING info for the refactoring patch (723 bytes, text/plain)
2020-01-17 16:37 UTC, Thomas Morper
no flags Details
Prosody 0.11.4 refactoring patch (10.03 KB, patch)
2020-02-11 00:03 UTC, Thomas Morper
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Morper 2019-10-31 17:29:49 UTC
Created attachment 208743 [details]
svn diff to update net-im/prosody to 0.11.3 + lua 5.2

I'd like to propose some changes to the prosody port, all contained in the attached patch:

1) Update Prosody to 0.11.3.

2) Drop LuaJIT and switch to Lua 5.2. LuaJIT is effectively dead, and though still working with Lua 5.1 Prosody's official-as-documented requirement is Lua 5.2.

3) Use arc4random.

4) Do not install the example certs because absolutely noone should use them.

5) Store user data in /var/db/prosody rather than /usr/local/var/lib/prosody. This requires moving an existing folder and I'm not sure about the right way to do this... is there a reliable way to do this automatically or would a pkg-install message be sufficient?

6) Install prosody-migrator (PR#235189). Do not add luadbi to RUN_DEPENDS though because it would pull in about 150MB of dependencies for this optional feature.

7) Update the configure-script switches. Explicitly state all required settings and do not use the "ostype" switch because some settings will be wrong then.

The patched port will build without warnings in poudriere and the resulting package has been working without any problems on my little Prosody server.
Comment 1 David Thiel freebsd_committer 2019-11-01 06:36:15 UTC
Thanks! Couple notes:

1) What's the reasoning behind arc4random change?

2) Is there a reason to use /var/db/ for the pid instead of /var/run?

3) There are some errors when building via poudriere. Log attached.
Comment 2 David Thiel freebsd_committer 2019-11-01 06:37:04 UTC
Created attachment 208756 [details]
Poudriere build log
Comment 3 Thomas Morper 2019-11-04 01:59:56 UTC
1) I had no intention for switching to arc4random, but I discovered this option while looking for something else, and it seems to be a sensible choice. arc4random is supposed to give us secure, performant, non-blocking randomness, and since we're on BSD we get it for free - not even a linker flag needed. So I guess there's mostly no reason for not using arc4random.

2) The default location for the pid file is the data directory which got moved from /usr/local/var/lib/prosody to the more fitting /var/db/prosody meaning the pid file ended up there as well. I agree that /var/run would be a better location for the pid file, but this needs more changes and checks and I didn't want to adress this issue in this round.

3) Sorry, I forgot to "svn add" a file and messed up the diff. Corrected version attached. Thanks for checking!
Comment 4 Thomas Morper 2019-11-04 02:01:07 UTC
Created attachment 208837 [details]
svn diff, corrected version
Comment 5 David Thiel freebsd_committer 2019-11-12 06:16:23 UTC
Ok, looks like that works. For the pid, I'm fine with it in either location, but I think it's best we decide now, otherwise people might have to change their configs twice. For whatever reason, in my production server it's specified in the config file.

I'm proposing the upgrade steps as:

1) service prosody stop
2) pkg upgrade prosody (or cd /usr/ports/net-im/prosody && make deinstall && make install)
3) mv /usr/local/var/lib/prosody/* /var/db/prosody/
4) If "pidfile" is specified in prosody.cfg.lua, change its new location to /var/db/prosody/prosody.pid (or remove the line)
5) service prosody start


We could technically do step 3 automatically on install, but I'm worried about yanking a db out from under a running process.
Comment 6 Thomas Morper 2019-11-20 23:37:18 UTC
Finally I had a look at the pid file...

By default Prosody doesn't write a pid file. However, a pid file needs to be defined via the "pidfile" config option or prosodyctl won't work. Without a leading path the pid file will be placed in the data directory (aka /var/db/prosody), otherwise in the defined location which needs write permissions for the prosody user.

So, I've updated the patch to create /var/run/prosody with the required permissions and use /var/run/prosody/prosody.pid as the default location in the rc script. This should match the users' expectations.

While I was at it I simplified the rc script. Grepping for an explicit "daemonize=false" in the config, then daemonizing via "daemon" rather than "prosodyctl" just isn't right, and without the "-p" option can't possibly work correctly, either. So this is gone. We stick to prosodyctl. It's up to the user to choose a different startup method when required (e.g. debugging).

With these changes #4 in your proposed upgrade steps would be

4) Set 'pidfile = "/var/run/prosody/prosody.pid"' in prosody.cfg.lua

What's the best way to communicate the upgrade steps to the user? Should the 5-step-guide be placed in pkg-message or is there a better way?
Comment 7 Thomas Morper 2019-11-20 23:38:25 UTC
Created attachment 209298 [details]
svn diff, including new pid file location
Comment 8 Sascha Biberhofer 2019-12-22 14:30:39 UTC
Created attachment 210139 [details]
net-im/prosody 0.11.2 to 0.11.3 diff

Hi! I've just taken a look at the patch. Package builds fine and runs fine with the data moved to the new directory. I've added a pkg-message file which unifies the other pkg-messages and notifies users upgrading from a previous release about the changes and the steps needed as suggested by lx@. 

It would also be a good idea to mention this in UPDATING, but other than that things look fine here and the upgrade was fairly painless on my end. :)
Comment 9 Kubilay Kocak freebsd_committer freebsd_triage 2020-01-13 03:11:24 UTC
I'll be landing (and MFH'ing) bug 241488 shortly

^Triage: 

 - Depend on bug 241488 to ensure it gets done first
 - Patch here needs updating to be rebased after bug 241488 commit

@Thomas Any older patches in this issue should be obsoleted. A single patch per issue is ideal, unless patches *must* be to be committed separately, say for instance in order to separate mergeable (port fixes) patches from non-mergeable (version updates)
Comment 10 Thomas Morper 2020-01-17 16:29:28 UTC
Created attachment 210822 [details]
Prosody 0.11.3 refactoring patch

With the update for Prosody 0.11.3 finally committed, here's an updated patch for the refactoring parts only, that is:

- switch to Lua 5.2
- remove support for the dead LuaJIT
- fix configure flags
- fix rc script
- move data directory and pid file (see separate comment for UPDATING)
- add the prosody-migrator
- remove cert examples
- use arc4random for randomness

Minor change: the cert examples are now removed completely as a self signed cert as well as real cert requests can be generated with prosodyctl.

The new patch has no major complaints from portlint and builds without warning in poudriere with 11.3 and 12.1 on AMD64. I've been running the refactored Prosody since October 2019 (and the re-created package since yesterday).
Comment 11 Thomas Morper 2020-01-17 16:37:42 UTC
Created attachment 210823 [details]
UPDATING info for the refactoring patch

Finally, here's a short text that should be added to the UPDATING file and that will inform Prosody users about the changes regarding the pid file and data directory and the required steps for an upgrade.

I did not include this in the refactoring patch so that the refactoring patch applies cleanly, regardless of changes in UPDATING by other ports.
Comment 12 Kubilay Kocak freebsd_committer freebsd_triage 2020-02-10 04:05:22 UTC
Comment on attachment 210139 [details]
net-im/prosody 0.11.2 to 0.11.3 diff

Already updated to 0.11.3
Comment 13 Kubilay Kocak freebsd_committer freebsd_triage 2020-02-10 04:06:23 UTC
Comment on attachment 208756 [details]
Poudriere build log

0.11.3 has landed already
Comment 14 Thomas Morper 2020-02-11 00:03:03 UTC
Created attachment 211547 [details]
Prosody 0.11.4 refactoring patch

net-im/prosody: refactored

 * Use Lua 5.2 and drop support for the dead LuaJIT.
 * Explicitly state all required configure options
   as the "freebsd" preset has some of them wrong.
 * Do not install the example certs as they clutter the
   config directory and nobody should ever use them.
   They can be recreated with prosodyctl if needed.
 * Remove the broken "daemonize" option from the rc script,
   handle all actions with "prosodyctl" instead.
 * Change data directory from /usr/local/var/lib/prosody to
   /var/db/prosody and include instructions in UPDATING.
 * Install the prosody migrator.
 * Use arc4random.

QA:

 * portlint: OK (looks fine, some warnings)
 * testport: OK (poudriere: 11.3 and 12.1, both i386 and amd64)
Comment 15 Kurt Jaeger freebsd_committer 2020-03-04 19:48:01 UTC
Committed, thanks!
Comment 16 commit-hook freebsd_committer 2020-03-04 19:48:06 UTC
A commit references this bug:

Author: pi
Date: Wed Mar  4 19:47:55 UTC 2020
New revision: 527796
URL: https://svnweb.freebsd.org/changeset/ports/527796

Log:
  net-im/prosody: refactor and move to lua 5.2

  - Use Lua 5.2 and drop support for the dead LuaJIT.
  - Explicitly state all required configure options
    as the "freebsd" preset has some of them wrong.
  - Do not install the example certs as they clutter the
    config directory and nobody should ever use them.
    They can be recreated with prosodyctl if needed.
  - Remove the broken "daemonize" option from the rc script,
    handle all actions with "prosodyctl" instead.
  - Change data directory from /usr/local/var/lib/prosody to
    /var/db/prosody and include instructions in UPDATING
  - Install the prosody migrator
  - Use arc4random

  PR:		241619
  Submitted by:	thomas@beingboiled.info
  Reviewed by:	lx (maintainer), Sascha Biberhofer <ports@skyforge.at>
  Approved by:	lx (maintainer timeout)

Changes:
  head/UPDATING
  head/net-im/prosody/Makefile
  head/net-im/prosody/files/patch-GNUmakefile
  head/net-im/prosody/files/patch-tools_migration_Makefile
  head/net-im/prosody/files/patch-util-src_time.c
  head/net-im/prosody/files/pkg-deinstall.in
  head/net-im/prosody/files/prosody.in
  head/net-im/prosody/pkg-plist