Skip to content

Conversation

@erebion
Copy link
Contributor

@erebion erebion commented May 28, 2025

This PR adds mod_unified_push from https://codeberg.org/itd/mod_unified_push

The author plans to archive the original repo if this PR gets accepted: https://codeberg.org/itd/mod_unified_push/issues/1

I've been using this module for a couple weeks now and it seems to work just fine.

As I couldn't find any info on what's required for contributions, kindly let me know what might be missing for this PR to get accepted.

@badlop
Copy link
Member

badlop commented May 29, 2025

Hi! This is a first list of ideas:

xmpp_spec.patch

Maybe that patch could be merged into upstream xmpp library.

README.txt

I would rename the file to README.md

I see some interesting setup instructions in https://codeberg.org/itd/mod_unified_push/issues/1 that maybe should be added to the README file.

How much detailed should the explanation be? You decide, depending on your experience configuring the module, and the complexity of the task, if it's configured similarly to other modules...

Example of detailed configuration instructions:
https://github.com/processone/ejabberd-contrib/tree/master/mod_filter#readme

-include_lib("p1_xmpp/include/xmpp.hrl").

It seems the module is designed to use libraries installed from debian repository. That path is different when libraries are compiled from source code, see

-include_lib("xmpp/include/xmpp.hrl").

ejabberd-contrib has automatic tests to ensure the modules compile correctly and pass some static analysis (with tools xref and dialyzer). Right now those results show a lot of errors, but most are related to p1_xmpp, and will disappear when this gets fixed.

Solution: replace with

-include_lib("xmpp/include/xmpp.hrl").

If you want to support both paths ("p1_xmpp" and "xmpp"), I guess we can look for some conditional compilation directive...

url: "[email protected]:itd/mod_unified_push.git"

This should be the URL where the source code is published.

Notice that the URL for most modules is

url: "[email protected]:processone/ejabberd-contrib.git"

There is one exception: spec files located in the extra/ directory can contain URLs from other git repositories, and then ejabberd takes care to download from those remote locations.

Unfortunately, as you can see in https://github.com/processone/ejabberd-contrib/tree/master/extra right now all the modules mentioned in that directory are not maintained anymore, so the files were renamed to "broken" to prevent ejabberd using them.

In summary, if the source code is expected to be published in ejabberd-contrib, then replace with

url: "[email protected]:processone/ejabberd-contrib.git"

You can add a paragraph in README.md explaining that the module was previously published in url XXX.

@itd0
Copy link

itd0 commented May 29, 2025

Hi,

thanks for the review and the feedback! Hope you don't mind me adding my two cents.

README.txt

I would rename the file to README.md

.md sounds good to me. IIRC, ejabberdctl module_check asked for a .txt (maybe here?) and I tried to comply.

-include_lib("p1_xmpp/include/xmpp.hrl").

It seems the module is designed to use libraries installed from debian repository.

Indeed. The include is inspired / guided / ... by a Debian patch.

If you want to support both paths ("p1_xmpp" and "xmpp"), I guess we can look for some conditional compilation directive...

Sounds interesting. Only for this module or in general?

url: "[email protected]:itd/mod_unified_push.git"

In summary, if the source code is expected to be published in ejabberd-contrib

(I'd be happy to see the source move here.)

You can add a paragraph in README.md explaining that the module was previously published in url XXX.

(Either, adding or omitting something like this, would work for me.)

Regards
itd

PS: Thanks erebion for creating this pull request.

@erebion
Copy link
Contributor Author

erebion commented Jun 5, 2025

Hi! This is a first list of ideas:

xmpp_spec.patch

Maybe that patch could be merged into upstream xmpp library.

README.txt

I would rename the file to README.md

✔️

I see some interesting setup instructions in https://codeberg.org/itd/mod_unified_push/issues/1 that maybe should be added to the README file.

✔️

url: "[email protected]:itd/mod_unified_push.git"

This should be the URL where the source code is published.

✔️

Did what I feel competent to do.

The remaining two points will require someone else, as I have not looked at Erlang code before I opened this Merge Request, so I have no competence for this yet (but give me tip on learning Erlang and I might be able to contribute in the future, I'd like to understand more of how ejabberd works).

xmpp_spec.patch

Maybe that patch could be merged into upstream xmpp library.

and

-include_lib("p1_xmpp/include/xmpp.hrl").

for which I'll not change the path until you both have finished talking about how this should done.

Maybe @itd0 would be fine looking at this change, it seems to be a small change after all. :)

@itd0
Copy link

itd0 commented Jun 7, 2025

xmpp_spec.patch

Maybe that patch could be merged into upstream xmpp library.

See processone/xmpp#101

-include_lib("p1_xmpp/include/xmpp.hrl").

for which I'll not change the path until you both have finished talking about how this should done.

To keep things simple (and consistent), I'd suggest to use xmpp/include/xmpp.hrl here and worry about Debian later. (I remain curious about the conditional compilation directive but that's not the point of this PR.)

@erebion
Copy link
Contributor Author

erebion commented Jun 9, 2025

To keep things simple (and consistent), I'd suggest to use xmpp/include/xmpp.hrl here and worry about Debian later. (I remain curious about the conditional compilation directive but that's not the point of this PR.)

I've amended the Merge Request as suggested.

Then it is just one point left:

xmpp_spec.patch

Maybe that patch could be merged into upstream xmpp library.
See processone/xmpp#101

badlop added a commit to processone/ejabberd that referenced this pull request Jun 9, 2025
badlop added a commit to processone/ejabberd that referenced this pull request Jun 9, 2025
@badlop
Copy link
Member

badlop commented Jun 9, 2025

ejabberdctl module_check asked for a .txt (maybe here?) and I tried to comply.

Ah sorry, you're right: both the source code and the documentation instruct to use README.txt in plaintext.

As ejabberd's README is in markdown, and some modules already included markdown, three years ago I converted all working modules to markdown too:

Convert modules README.txt to markdown syntax 04f4489

I've now updated the source code and documentation ;)


(I remain curious about the conditional compilation directive but that's not the point of this PR.)

I was also curious about how exactly could it be implemented, and wrote a quick and dirty patch...

Example usage:

$ ./configure --with-rebar=rebar3
$ make
===> Compiling src/ext_mod.erl failed
src/ext_mod.erl:{54,14}: can't find include lib "path_in_non_debian_system.hrl"

$ ./configure --enable-debian --with-rebar=rebar3
$ make
===> Compiling src/ext_mod.erl failed
src/ext_mod.erl:{58,14}: can't find include lib "path_in_debian_system.hrl"

Patch to ejabberd (and dummy test in a random file, for example ext_mod.erl):

diff --git a/configure.ac b/configure.ac
index 12e3ae463..1b64a7f8f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -301,6 +301,14 @@ AC_ARG_ENABLE(zlib,
   *) AC_MSG_ERROR(bad value ${enableval} for --enable-zlib) ;;
 esac],[if test "x$zlib" = "x"; then zlib=true; fi])
 
+AC_ARG_ENABLE(debian,
+[AS_HELP_STRING([--enable-debian],[use Debian specific include paths (default: no)])],
+[case "${enableval}" in
+  yes) debian=true ;;
+  no)  debian=false ;;
+  *) AC_MSG_ERROR(bad value ${enableval} for --enable-debian) ;;
+esac],[if test "x$debian" = "x"; then debian=false; fi])
+
 case "`uname`" in
   "Darwin")
     # Darwin (macos) erlang-sqlite is built using amalgamated lib, so no external dependency
@@ -334,6 +342,7 @@ AC_SUBST(stun)
 AC_SUBST(sip)
 AC_SUBST(debug)
 AC_SUBST(lua)
+AC_SUBST(debian)
 AC_SUBST(tools)
 AC_SUBST(latest_deps)
 AC_SUBST(system_deps)
diff --git a/rebar.config b/rebar.config
index 97b2f2805..b0a5f9de9 100644
--- a/rebar.config
+++ b/rebar.config
@@ -141,6 +141,7 @@
             {if_version_below, "26", {d, 'OTP_BELOW_26'}},
             {if_version_below, "27", {d, 'OTP_BELOW_27'}},
             {if_version_below, "28", {d, 'OTP_BELOW_28'}},
+            {if_var_true, debian, {d, 'DEBIAN_SYSTEM'}},
             {if_var_false, debug, no_debug_info},
             {if_var_true, debug, debug_info},
             {if_var_true, elixir, {d, 'ELIXIR_ENABLED'}},
diff --git a/src/ext_mod.erl b/src/ext_mod.erl
index 2d34a91f9..0b4d111f8 100644
--- a/src/ext_mod.erl
+++ b/src/ext_mod.erl
@@ -50,6 +50,14 @@
 -include("translate.hrl").
 -include_lib("xmpp/include/xmpp.hrl").
 
+-ifndef(DEBIAN_SYSTEM).
+-include_lib("path_in_non_debian_system.hrl").
+-endif.
+
+-ifdef(DEBIAN_SYSTEM).
+-include_lib("path_in_debian_system.hrl").
+-endif.
+
 -define(REPOS, "[email protected]:processone/ejabberd-contrib.git").
 
 -record(state, {}).
diff --git a/vars.config.in b/vars.config.in
index ded059b3e..1901aed24 100644
--- a/vars.config.in
+++ b/vars.config.in
@@ -24,6 +24,7 @@
 {debug, @debug@}.
 {new_sql_schema, @new_sql_schema@}.
 
+{debian, @debian@}.
 {tools, @tools@}.
 
 %% Dependencies

But later, thinking more carefully, I've found a better solution: this can be implemented in ext_mod.erl more easily, and it is now committed in processone/ejabberd@8855a30

If anybody is able to test it in a real debian system, please comment your results.

@badlop badlop self-assigned this Jun 9, 2025
@badlop
Copy link
Member

badlop commented Jun 9, 2025

The test actions are failing with different errors, some of them are probably related to ejabberd using an xmpp library that doesn't yet include the required improvements.

Let's wait until xmpp gets the patch, then ejabberd gets updated to use it, then those tests use it, and the new test results will be the ones to really review.

@erebion
Copy link
Contributor Author

erebion commented Jun 10, 2025

If anybody is able to test it in a real debian system, please comment your results.

Could try the build from CI. Can I go back to 25.04 if anything breaks?

EDIT: Once the patch is there and the CI no longer fails and so on

@badlop
Copy link
Member

badlop commented Jun 16, 2025

Could try the build from CI. Can I go back to 25.04 if anything breaks?

I think yes, because looking at the git commits in ejabberd master branch since 25.04, I see no changes in the mnesia or SQL database schemas, so I see no problem in switching from 25.04 to and from git master.


Regarding the overall tasks:

The three records unified_push_* are now defined in xmpp/include/xmpp_codec.hrl, which is included by
xmpp/include/xmpp.hrl, which mod_unified_push.erl already includes with the line

-include_lib("xmpp/include/xmpp.hrl").

Solution: you can remove those records definitions in lines 86-101.

@itd0
Copy link

itd0 commented Jun 16, 2025

The three records unified_push_* are now defined in xmpp/include/xmpp_codec.hrl, which is included by xmpp/include/xmpp.hrl, which mod_unified_push.erl already includes with the line

-include_lib("xmpp/include/xmpp.hrl").

Solution: you can remove those records definitions in lines 86-101.

It should also be safe to remove lines 354-868 (see FIXME in line 355).
(One could also remove the patch file and references to it from the README.md.)

Edit (2025-06-23): This (untested) commit should do the trick.

@erebion
Copy link
Contributor Author

erebion commented Jun 26, 2025

Solution: you can remove those records definitions in lines 86-101.

done

It should also be safe to remove lines 354-868 (see FIXME in line 355).

done

@erebion
Copy link
Contributor Author

erebion commented Jun 26, 2025

(One could also remove the patch file and references to it from the README.md.)

Edit (2025-06-23): This (untested) commit should do the trick.

Remove it or include the changed version?

@badlop
Copy link
Member

badlop commented Jun 30, 2025

With your latest improvements, I noticed there's a problem in ejabberd-contrib testing CI; and I've solved it with commit cf0c7a8. Please rebase your branch to master, to benefit from that small commit.

Additionally, I've solved other problems that were detected by CI testing, and have submitted a PR to your branch: erebion#1

@erebion erebion force-pushed the add_unified_push_module branch from d194bc2 to 2e0594c Compare June 30, 2025 14:45
@erebion
Copy link
Contributor Author

erebion commented Jun 30, 2025

With your latest improvements, I noticed there's a problem in ejabberd-contrib testing CI; and I've solved it with commit cf0c7a8. Please rebase your branch to master, to benefit from that small commit.

done

Additionally, I've solved other problems that were detected by CI testing, and have submitted a PR to your branch: erebion#1

Re-basing the branch on latest master created merge conflicts over there.

Several fixes for compilation, dialyzer, documentation and "make options"
@badlop badlop merged commit fed2b5a into processone:master Jul 4, 2025
5 checks passed
@badlop
Copy link
Member

badlop commented Jul 4, 2025

It's finally merged 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants