| Message ID | 20190718100636.27290-2-titouan.christophe@railnova.eu |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Update package mosquitto | expand |
>>>>> "Titouan" == Titouan Christophe <titouan.christophe@railnova.eu> writes: > Signed-off-by: Titouan Christophe <titouan.christophe@railnova.eu> > --- > package/mosquitto/Config.in | 4 ---- > package/mosquitto/mosquitto.mk | 6 ++++++ > 2 files changed, 6 insertions(+), 4 deletions(-) > diff --git a/package/mosquitto/Config.in b/package/mosquitto/Config.in > index 11b6d7891b..7135e86e69 100644 > --- a/package/mosquitto/Config.in > +++ b/package/mosquitto/Config.in > @@ -1,6 +1,5 @@ > config BR2_PACKAGE_MOSQUITTO > bool "mosquitto" > - depends on !BR2_STATIC_LIBS # builds .so > help > Mosquitto is an open source message broker that implements > the MQ Telemetry Transport protocol versions 3.1 and > @@ -22,6 +21,3 @@ config BR2_PACKAGE_MOSQUITTO_BROKER > comment "mosquitto broker needs a system with MMU" > depends on BR2_PACKAGE_MOSQUTTO && !BR2_USE_MMU > - > -comment "mosquitto needs a toolchain w/ dynamic library" > - depends on BR2_STATIC_LIBS > diff --git a/package/mosquitto/mosquitto.mk b/package/mosquitto/mosquitto.mk > index 51c0abd0ba..ed72af754a 100644 > --- a/package/mosquitto/mosquitto.mk > +++ b/package/mosquitto/mosquitto.mk > @@ -17,6 +17,12 @@ MOSQUITTO_MAKE_OPTS = \ > WITH_WRAP=no \ > WITH_DOCS=no > +ifeq ($(BR2_STATIC_LIBS),y) > +MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=yes WITH_SHARED_LIBRARIES=no > +else > +MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=no WITH_SHARED_LIBRARIES=yes > +endif We actually have 3 variants: - static only (BR2_STATIC_LIBS) - shared only (BR2_SHARED_LIBS) - shared and static (BR2_SHARED_STATIC_LIBS) So I reworked it to take that into consideration and committed, thanks.
>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes: Hi, >> +ifeq ($(BR2_STATIC_LIBS),y) >> +MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=yes WITH_SHARED_LIBRARIES=no >> +else >> +MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=no WITH_SHARED_LIBRARIES=yes >> +endif > We actually have 3 variants: > - static only (BR2_STATIC_LIBS) > - shared only (BR2_SHARED_LIBS) > - shared and static (BR2_SHARED_STATIC_LIBS) > So I reworked it to take that into consideration and committed, thanks. Sorry, I will have to take that back. I did a test compilation with a completely static (BR2_STATIC_LIBS) setup, which failed: In file included from security.c:25: lib_load.h:23:11: fatal error: dlfcn.h: No such file or directory # include <dlfcn.h> ^~~~~~~~~ compilation terminated. The loadable plugin logic in security.c seems to be built unconditionally, so that cannot work in a completely static setup. Care to bring up this issue upstream? In the mean time I have marked your patch as changes requested. For reference, the changes I did in mosquitto.mk look like this: feq ($(BR2_SHARED_LIBS),y) MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=no else MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=yes endif ifeq ($(BR2_STATIC_LIBS),y) MOSQUITTO_MAKE_OPTS += WITH_SHARED_LIBRARIES=no else MOSQUITTO_MAKE_OPTS += WITH_SHARED_LIBRARIES=yes endif
Hello Peter, Thank you for reviewing this series ! On 8/1/19 12:41 PM, Peter Korsgaard wrote: >>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes: > > Hi, > > >> +ifeq ($(BR2_STATIC_LIBS),y) > >> +MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=yes WITH_SHARED_LIBRARIES=no > >> +else > >> +MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=no WITH_SHARED_LIBRARIES=yes > >> +endif > > > We actually have 3 variants: > > > - static only (BR2_STATIC_LIBS) > > - shared only (BR2_SHARED_LIBS) > > - shared and static (BR2_SHARED_STATIC_LIBS) > > > So I reworked it to take that into consideration and committed, thanks. > > Sorry, I will have to take that back. I did a test compilation with a > completely static (BR2_STATIC_LIBS) setup, which failed: Could you share the defconfig you have used to obtain that result ? When I run test-pkg with BR2_PACKAGE_MOSQUITTO=y (and BR2_PACKAGE_MOSQUITTO_BROKER defaults to y), I see: """br-arm-full-static [5/6]: OK""". > > In file included from security.c:25: > lib_load.h:23:11: fatal error: dlfcn.h: No such file or directory > # include <dlfcn.h> > ^~~~~~~~~ > compilation terminated. > > The loadable plugin logic in security.c seems to be built > unconditionally, so that cannot work in a completely static setup. > > Care to bring up this issue upstream? As soon as I understand the conditions to reproduce, I will do so. > > In the mean time I have marked your patch as changes requested. For > reference, the changes I did in mosquitto.mk look like this: > > feq ($(BR2_SHARED_LIBS),y) > MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=no > else > MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=yes > endif > > ifeq ($(BR2_STATIC_LIBS),y) > MOSQUITTO_MAKE_OPTS += WITH_SHARED_LIBRARIES=no > else > MOSQUITTO_MAKE_OPTS += WITH_SHARED_LIBRARIES=yes > endif > Thanks for sharing ! Best regards, Titouan
>>>>> "Titouan" == Titouan Christophe <titouan.christophe@railnova.eu> writes: > Hello Peter, > Thank you for reviewing this series ! > On 8/1/19 12:41 PM, Peter Korsgaard wrote: >>>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes: >> >> Hi, >> >> >> +ifeq ($(BR2_STATIC_LIBS),y) >> >> +MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=yes WITH_SHARED_LIBRARIES=no >> >> +else >> >> +MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=no WITH_SHARED_LIBRARIES=yes >> >> +endif >> >> > We actually have 3 variants: >> >> > - static only (BR2_STATIC_LIBS) >> > - shared only (BR2_SHARED_LIBS) >> > - shared and static (BR2_SHARED_STATIC_LIBS) >> >> > So I reworked it to take that into consideration and committed, thanks. >> >> Sorry, I will have to take that back. I did a test compilation with a >> completely static (BR2_STATIC_LIBS) setup, which failed: > Could you share the defconfig you have used to obtain that result ? It was afaik just the default arm variant with BR2_STATIC_LIBS enabled. > When I run test-pkg with BR2_PACKAGE_MOSQUITTO=y (and > BR2_PACKAGE_MOSQUITTO_BROKER defaults to y), I see: > """br-arm-full-static [5/6]: OK""". Hmm, very odd. With current git HEAD and this patch 1/3 applied and the following config: BR2_arm=y BR2_STATIC_LIBS=y BR2_TOOLCHAIN_EXTERNAL=y BR2_TOOLCHAIN_EXTERNAL_DOWNLOAD=y BR2_TOOLCHAIN_EXTERNAL_URL="http://autobuild.buildroot.org/toolchains/tarballs/br-arm-full-static-2019.02-rc1.tar.bz2" BR2_TOOLCHAIN_EXTERNAL_GCC_7=y BR2_TOOLCHAIN_EXTERNAL_HEADERS_4_4=y BR2_TOOLCHAIN_EXTERNAL_LOCALE=y # BR2_TOOLCHAIN_EXTERNAL_HAS_THREADS_DEBUG is not set BR2_TOOLCHAIN_EXTERNAL_CXX=y BR2_PACKAGE_MOSQUITTO=y (E.G. support/config-fragments/autobuild/br-arm-full-static.config + mosquitto) I get the expected failure: In file included from security.c:25:0: lib_load.h:23:11: fatal error: dlfcn.h: No such file or directory # include <dlfcn.h> ^~~~~~~~~ compilation terminated. Makefile:189: recipe for target 'security.o' failed br-arm-full-static-2019.02-rc1.tar.bz2 does not contain a dlfcn.h file, so that makes sense.
Hello, On 8/1/19 7:25 PM, Peter Korsgaard wrote: > > It was afaik just the default arm variant with BR2_STATIC_LIBS enabled. > > > When I run test-pkg with BR2_PACKAGE_MOSQUITTO=y (and > > BR2_PACKAGE_MOSQUITTO_BROKER defaults to y), I see: > > """br-arm-full-static [5/6]: OK""". > > Hmm, very odd. With current git HEAD and this patch 1/3 applied and the > following config: > > BR2_arm=y > BR2_STATIC_LIBS=y > BR2_TOOLCHAIN_EXTERNAL=y > BR2_TOOLCHAIN_EXTERNAL_DOWNLOAD=y > BR2_TOOLCHAIN_EXTERNAL_URL="http://autobuild.buildroot.org/toolchains/tarballs/br-arm-full-static-2019.02-rc1.tar.bz2" > BR2_TOOLCHAIN_EXTERNAL_GCC_7=y > BR2_TOOLCHAIN_EXTERNAL_HEADERS_4_4=y > BR2_TOOLCHAIN_EXTERNAL_LOCALE=y > # BR2_TOOLCHAIN_EXTERNAL_HAS_THREADS_DEBUG is not set > BR2_TOOLCHAIN_EXTERNAL_CXX=y > BR2_PACKAGE_MOSQUITTO=y > Indeed, when doing a regular make (and not test-pkg) with this defconfig I can reproduce the aforementioned failure. > (E.G. support/config-fragments/autobuild/br-arm-full-static.config + mosquitto) > > I get the expected failure: > > In file included from security.c:25:0: > lib_load.h:23:11: fatal error: dlfcn.h: No such file or directory > # include <dlfcn.h> > ^~~~~~~~~ > compilation terminated. > Makefile:189: recipe for target 'security.o' failed > > br-arm-full-static-2019.02-rc1.tar.bz2 does not contain a dlfcn.h file, > so that makes sense. > Now, when looking at mosquitto, I understand that WITH_SHARED_LIBRARY=no implies that libmosquitto.so won't be created (and linked against). However, the failure over here is due to some code in the broker which is intended to dynamically load modules at runtime, no matter if mosquitto is running inside a statically linked executable or from a dylib. As such, I suggest then to only make the broker depend on BR2_SHARED_LIBS, while still allowing the client lib to be built statically, which in my opinion makes sense for embedded targets. What do you think ? Kind regards, Titouan
>>>>> "Titouan" == Titouan Christophe <titouan.christophe@railnova.eu> writes: Hi, > Indeed, when doing a regular make (and not test-pkg) with this > defconfig I can reproduce the aforementioned failure. Funky, but OK. > Now, when looking at mosquitto, I understand that > WITH_SHARED_LIBRARY=no implies that libmosquitto.so won't be created > (and linked against). > However, the failure over here is due to some code in the broker which > is intended to dynamically load modules at runtime, no matter if > mosquitto is running inside a statically linked executable or from a > dylib. > As such, I suggest then to only make the broker depend on > BR2_SHARED_LIBS, while still allowing the client lib to be built > statically, which in my opinion makes sense for embedded targets. > What do you think ? Yes, that could work. Will you send a patch for that?
diff --git a/package/mosquitto/Config.in b/package/mosquitto/Config.in index 11b6d7891b..7135e86e69 100644 --- a/package/mosquitto/Config.in +++ b/package/mosquitto/Config.in @@ -1,6 +1,5 @@ config BR2_PACKAGE_MOSQUITTO bool "mosquitto" - depends on !BR2_STATIC_LIBS # builds .so help Mosquitto is an open source message broker that implements the MQ Telemetry Transport protocol versions 3.1 and @@ -22,6 +21,3 @@ config BR2_PACKAGE_MOSQUITTO_BROKER comment "mosquitto broker needs a system with MMU" depends on BR2_PACKAGE_MOSQUTTO && !BR2_USE_MMU - -comment "mosquitto needs a toolchain w/ dynamic library" - depends on BR2_STATIC_LIBS diff --git a/package/mosquitto/mosquitto.mk b/package/mosquitto/mosquitto.mk index 51c0abd0ba..ed72af754a 100644 --- a/package/mosquitto/mosquitto.mk +++ b/package/mosquitto/mosquitto.mk @@ -17,6 +17,12 @@ MOSQUITTO_MAKE_OPTS = \ WITH_WRAP=no \ WITH_DOCS=no +ifeq ($(BR2_STATIC_LIBS),y) +MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=yes WITH_SHARED_LIBRARIES=no +else +MOSQUITTO_MAKE_OPTS += WITH_STATIC_LIBRARIES=no WITH_SHARED_LIBRARIES=yes +endif + # adns uses getaddrinfo_a ifeq ($(BR2_TOOLCHAIN_USES_GLIBC),y) MOSQUITTO_MAKE_OPTS += WITH_ADNS=yes
Signed-off-by: Titouan Christophe <titouan.christophe@railnova.eu> --- package/mosquitto/Config.in | 4 ---- package/mosquitto/mosquitto.mk | 6 ++++++ 2 files changed, 6 insertions(+), 4 deletions(-)