openwrt / telephony

The telephony packages feed
105 stars 247 forks source link

asterisk: bump to 18.7.1 and adjust for musl 64bit time_t #692

Closed dhewg closed 2 years ago

dhewg commented 2 years ago

This fixes #690. Runtime tested on lantiq/xrx200.

I fixed all time_t related compiler warnings while enabling almost all asterisk modules. kmod-dahdi doesn't build, so I left out all dahdi related modules.

micmac1 commented 2 years ago

Hello Andre,

Does this still work with glibc? Can you send it upstream?

Kind regards, Seb

dhewg commented 2 years ago

It builds warning free on glibc, and there're other uses of PRId64 already in there. So it should be fine.

My last interactions with upstream have been meh, but: https://issues.asterisk.org/jira/browse/ASTERISK-29674

And already: You dont appear to have a current, signed submission license agreement on file. Please sign one before attempting to upload a code or documentation contribution. Nope, not doing that

dhewg commented 2 years ago

Bumped the bump from 18.6.0 to 18.7.0

micmac1 commented 2 years ago

It builds warning free on glibc, and there're other uses of PRId64 already in there. So it should be fine.

My last interactions with upstream have been meh, but: https://issues.asterisk.org/jira/browse/ASTERISK-29674

And already: You dont appear to have a current, signed submission license agreement on file. Please sign one before attempting to upload a code or documentation contribution. Nope, not doing that

Hello @dhewg

Yes, I know sending something upstream can be difficult/entail a certain amount of work. But the flip side of not sending it upstream is that we need to maintain the patch here, which also potentially means an amount of work (maybe not for you, but for other people).

One other good reason to send something upstream is that you'll get a proper review of the changes you made.

Kind regards, Seb

dhewg commented 2 years ago

Right, but that's not what happened. I'm not objecting to sending stuff upstream. Like at all, that's how it should be. I took the time to report the issue and even attached a patch fixing the issue. It's upstream's choice to not accept it, it's not my fault they reject it because of some bureaucratic BS.

I don't even care what license to put this trivial patch under. If you signed their stuff feel free to slap your name on it and send it their way.

If this makes it unacceptable for OpenWrt I don't have have a problem with carrying the patch locally, but note that asterisk is broken as-is on master since the musl bump.

micmac1 commented 2 years ago

I think your time64 patch may be incorrect. I hope I'm not leaning my head too far our the window here, not being a programmer and all :)

The PRId64 macro is meant to be used for conversion of type int64_t. I don't think it can serve properly here in this situation, where libcs go to use time64.

Your patch seems to work when using libc like musl with time64, on a 32 bit host. Probably it'll also work on a 64 bit host. But when using a libc without time64 it introduces the same warnings (reversed) when compiling for 32 bit targets (64 bit targets may be OK, I didn't try).

res_pjsip/pjsip_options.c: In function 'ast_sip_format_contact_ami':
res_pjsip/pjsip_options.c:2736:26: warning: format '%lld' expects argument of type 'long long int', but argument 4 has type 'time_t' {aka 'long int'} [-Wformat=]
  ast_str_append(&buf, 0, "RegExpire: %" PRId64 "\r\n", contact->expiration_time.tv_sec);
                          ^~~~~~~~~~~~~~                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/sk/tmp/sdk/openwrt-sdk-21.02.0-ath79-generic_gcc-8.4.0_musl.Linux-x86_64/build_dir/target-mips_24kc_musl/asterisk-18.5.1/include/asterisk/compat.h:30,
                 from /home/sk/tmp/sdk/openwrt-sdk-21.02.0-ath79-generic_gcc-8.4.0_musl.Linux-x86_64/build_dir/target-mips_24kc_musl/asterisk-18.5.1/include/asterisk.h:22,
                 from res_pjsip/pjsip_options.c:20:
/home/sk/tmp/sdk/openwrt-sdk-21.02.0-ath79-generic_gcc-8.4.0_musl.Linux-x86_64/staging_dir/toolchain-mips_24kc_gcc-8.4.0_musl/include/inttypes.h:36:25: note: format string is defined here
 #define PRId64 __PRI64 "d"

glibc's /usr/include/inttypes.h defines it like this:


# if __WORDSIZE == 64
#  define __PRI64_PREFIX        "l"
#  define __PRIPTR_PREFIX       "l"
# else
#  define __PRI64_PREFIX        "ll"
#  define __PRIPTR_PREFIX
# endif
...
# define PRId64         __PRI64_PREFIX "d"

So this depends solely on __WORDSIZE. It doesn't have anything to do with time.

You say the patch is trivial, but I think the devil is in the details :)

dhewg commented 2 years ago

Hehe, apparently it is! This PRId64 solution comes from another thread that talked about switching glibc to 64bit time for OpenWrt too. Your're right that this won't work at this time, as glibc apparently hasn't switched yet. I'm not sure if there's a time_t related printf specifier at all.

micmac1 commented 2 years ago

I linked a pull request to your upstream report. Please check if you have some time.

dhewg commented 2 years ago

Nice, will do!

dhewg commented 2 years ago

Seems to work just fine, thanks! Updated the PR, bumped the bump again to 18.7.1 and replaced my patch with yours. Now 32bit glibc builds should work again (which is the only version I did not test compile...)

micmac1 commented 2 years ago

Thank you very much!

dhewg commented 2 years ago

Nope, thank you ;)

And I probably shouldn't have invested in a project where I can't, won't or am not willing to contribute for whatever reason, because I actually enjoy contributing when I run into problems or limitations.

Edit: That sounded vague, of course I meant asterisk and not OpenWrt :)