| Message ID | 20231227181756.156827-1-bernd@kuhls.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] package/exim: fix build with libexecinfo | expand |
Hello Bernd, On Wed, 27 Dec 2023 19:17:55 +0100 Bernd Kuhls <bernd@kuhls.net> wrote: > Upstream added optional support for execinfo > > https://git.exim.org/exim.git/commitdiff/204a7a2c2e8601558905dc34c576a627045a9f21 > https://git.exim.org/exim.git/commitdiff/48ea675fee2d5fee8d33c525e28727b69114cfce > > in version 4.97 which was added to buildroot with commit > faec3ca30e358575f70a036879029f63f7da9b29 > > Fixes: > http://autobuild.buildroot.net/results/282/282882371e1d8c224c457bf65016f8abd11f8c45/ Thanks for taking care of the build failures! > @@ -126,6 +127,15 @@ ifeq ($(BR2_STATIC_LIBS),y) > EXIM_STATIC_FLAGS = LFLAGS="-pthread --static" > endif > > +ifeq ($(BR2_PACKAGE_LIBEXECINFO),y) > +EXIM_DEPENDENCIES += libexecinfo > +define EXIM_EXTRALIBS > +$(call exim-config-add,EXTRALIBS,-lexecinfo) > +endef > +else > +EXIM_C_FLAGS = -DNO_EXECINFO > +endif I think this logic is not entirely correct. As I read it: * on non-glibc systems it automatically enables the new exim stack dump feature if libexecinfo is enabled * on glibc systems, which always have the backtrace() and related functions without additional libraries, we never enable this feature For consistency, the else branch should just be removed to avoid setting NO_EXECINFO, thus enabling the stack dump feature whenever it is possible. Luca
Bernd, Luca, All, On 2023-12-28 09:51 +0100, Luca Ceresoli via buildroot spake thusly: > On Wed, 27 Dec 2023 19:17:55 +0100 > Bernd Kuhls <bernd@kuhls.net> wrote: > > Upstream added optional support for execinfo [--SNIP--] > > @@ -126,6 +127,15 @@ ifeq ($(BR2_STATIC_LIBS),y) > > EXIM_STATIC_FLAGS = LFLAGS="-pthread --static" > > endif > > > > +ifeq ($(BR2_PACKAGE_LIBEXECINFO),y) > > +EXIM_DEPENDENCIES += libexecinfo > > +define EXIM_EXTRALIBS > > +$(call exim-config-add,EXTRALIBS,-lexecinfo) > > +endef > > +else > > +EXIM_C_FLAGS = -DNO_EXECINFO > > +endif > > I think this logic is not entirely correct. As I read it: > > * on non-glibc systems it automatically enables the new exim > stack dump feature if libexecinfo is enabled > * on glibc systems, which always have the backtrace() and related > functions without additional libraries, we never enable this feature > > For consistency, the else branch should just be removed to avoid > setting NO_EXECINFO, thus enabling the stack dump feature whenever it > is possible. If I understand correctly, your proposal would not work either: for a non-glibc config that does not have libexecinfo eabled, we do want to define NO_EXECINFO. Instead, the logic should be somthong like: if glibc: do nothing, execinfo is available elif libexecinfo enabled: add dependency and extra-lib else: define NO_EXECINFO Since libexecinfo embedds the fact that it is also non-glibc, we can rewrite the condition as: if libexecinfo: add dependency and extra-lib elif non-glibc; define NO_EXECINFO So, the 'else' clause only needs to be changed into: else ifneq ($(BR2_TOOLCHAIN_USES_GLIBC),y) Unless I missed something less obvious... Regards, Yann E. MORIN.
Hi Yann, On Fri, 29 Dec 2023 22:56:26 +0100 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > Bernd, Luca, All, > > On 2023-12-28 09:51 +0100, Luca Ceresoli via buildroot spake thusly: > > On Wed, 27 Dec 2023 19:17:55 +0100 > > Bernd Kuhls <bernd@kuhls.net> wrote: > > > Upstream added optional support for execinfo > [--SNIP--] > > > @@ -126,6 +127,15 @@ ifeq ($(BR2_STATIC_LIBS),y) > > > EXIM_STATIC_FLAGS = LFLAGS="-pthread --static" > > > endif > > > > > > +ifeq ($(BR2_PACKAGE_LIBEXECINFO),y) > > > +EXIM_DEPENDENCIES += libexecinfo > > > +define EXIM_EXTRALIBS > > > +$(call exim-config-add,EXTRALIBS,-lexecinfo) > > > +endef > > > +else > > > +EXIM_C_FLAGS = -DNO_EXECINFO > > > +endif > > > > I think this logic is not entirely correct. As I read it: > > > > * on non-glibc systems it automatically enables the new exim > > stack dump feature if libexecinfo is enabled > > * on glibc systems, which always have the backtrace() and related > > functions without additional libraries, we never enable this feature > > > > For consistency, the else branch should just be removed to avoid > > setting NO_EXECINFO, thus enabling the stack dump feature whenever it > > is possible. > > If I understand correctly, your proposal would not work either: for a > non-glibc config that does not have libexecinfo eabled, we do want to > define NO_EXECINFO. Exactly! > Instead, the logic should be somthong like: > > if glibc: > do nothing, execinfo is available > elif libexecinfo enabled: > add dependency and extra-lib > else: > define NO_EXECINFO > > Since libexecinfo embedds the fact that it is also non-glibc, we can > rewrite the condition as: > > if libexecinfo: > add dependency and extra-lib > elif non-glibc; > define NO_EXECINFO > > So, the 'else' clause only needs to be changed into: > else ifneq ($(BR2_TOOLCHAIN_USES_GLIBC),y) Correct, thanks for suggesting this fix. Luca
diff --git a/package/exim/exim.mk b/package/exim/exim.mk index 23d888e6f2..30b0d78c94 100644 --- a/package/exim/exim.mk +++ b/package/exim/exim.mk @@ -104,6 +104,7 @@ define EXIM_CONFIGURE_TOOLCHAIN $(call exim-config-add,HOSTCC,$(HOSTCC)) $(call exim-config-add,HOSTCFLAGS,$(HOSTCFLAGS)) $(EXIM_FIX_IP_OPTIONS_FOR_MUSL) + $(EXIM_EXTRALIBS) endef ifneq ($(call qstrip,$(BR2_PACKAGE_EXIM_CUSTOM_CONFIG_FILE)),) @@ -126,6 +127,15 @@ ifeq ($(BR2_STATIC_LIBS),y) EXIM_STATIC_FLAGS = LFLAGS="-pthread --static" endif +ifeq ($(BR2_PACKAGE_LIBEXECINFO),y) +EXIM_DEPENDENCIES += libexecinfo +define EXIM_EXTRALIBS +$(call exim-config-add,EXTRALIBS,-lexecinfo) +endef +else +EXIM_C_FLAGS = -DNO_EXECINFO +endif + # We need the host version of macro_predef during the build, before # building it we need to prepare the makefile. define EXIM_BUILD_CMDS @@ -136,7 +146,7 @@ define EXIM_BUILD_CMDS CFLAGS="-std=c99 $(HOST_CFLAGS)" \ LFLAGS="-fPIC $(HOST_LDFLAGS)" $(TARGET_MAKE_ENV) build=br $(MAKE) -C $(@D) $(EXIM_STATIC_FLAGS) \ - CFLAGS="-std=c99 $(TARGET_CFLAGS)" + CFLAGS="-std=c99 $(TARGET_CFLAGS) $(EXIM_C_FLAGS)" endef # Need to replicate the LFLAGS in install, as exim still wants to build
Upstream added optional support for execinfo https://git.exim.org/exim.git/commitdiff/204a7a2c2e8601558905dc34c576a627045a9f21 https://git.exim.org/exim.git/commitdiff/48ea675fee2d5fee8d33c525e28727b69114cfce in version 4.97 which was added to buildroot with commit faec3ca30e358575f70a036879029f63f7da9b29 Fixes: http://autobuild.buildroot.net/results/282/282882371e1d8c224c457bf65016f8abd11f8c45/ Signed-off-by: Bernd Kuhls <bernd@kuhls.net> --- package/exim/exim.mk | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)