Message ID | 20160824231003.29262-2-arnout@mind.be |
---|---|
State | Changes Requested |
Headers | show |
Hi Arnout, On Thu, Aug 25, 2016 at 01:10:03AM +0200, Arnout Vandecappelle (Essensium/Mind) wrote: > libgcrypt pulls in gpg-error which links with libintl if available. > Since iputils doesn't use libtool, -lintl has to be passed explicitly > for static builds. > > Fixes: > http://autobuild.buildroot.net/results/f81/f81eabb37788aa6dcdadf4034889c84bef78b876 > http://autobuild.buildroot.net/results/f29/f296e8fba1e79f96d3d119aa4c8207ed4b80694b > http://autobuild.buildroot.net/results/478/478a88cd892a119970cfca717ea7f5517bfc4cea > http://autobuild.buildroot.net/results/920/920d2d1d967691779f79fa54a38fac5adc5a8ee4 > > Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > --- > package/iputils/iputils.mk | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/package/iputils/iputils.mk b/package/iputils/iputils.mk > index d6398f7..47f3e28 100644 > --- a/package/iputils/iputils.mk > +++ b/package/iputils/iputils.mk > @@ -26,6 +26,10 @@ endif > IPUTILS_MAKE_OPTS = $(TARGET_CONFIGURE_OPTS) USE_SYSFS=no USE_IDN=no\ > CFLAGS="$(TARGET_CFLAGS) -D_GNU_SOURCE" > > +ifeq ($(BR2_NEEDS_GETTEXT)$(BR2_PACKAGE_GETTEXT),yy) > +IPUTILS_MAKE_OPTS += ADDLIB='-lintl' > +endif I think this should be limited to BR2_STATIC_LIBS. It may not change much the end result, but it makes the motivation clearer. baruch
Hello, On Thu, 25 Aug 2016 01:10:03 +0200, Arnout Vandecappelle (Essensium/Mind) wrote: > +ifeq ($(BR2_NEEDS_GETTEXT)$(BR2_PACKAGE_GETTEXT),yy) > +IPUTILS_MAKE_OPTS += ADDLIB='-lintl' > +endif Why do you need both BR2_NEEDS_GETTEXT=y and BR2_PACKAGE_GETTEXT=y ? Thomas
On 25-08-16 10:18, Thomas Petazzoni wrote: > Hello, > > On Thu, 25 Aug 2016 01:10:03 +0200, Arnout Vandecappelle > (Essensium/Mind) wrote: > >> +ifeq ($(BR2_NEEDS_GETTEXT)$(BR2_PACKAGE_GETTEXT),yy) >> +IPUTILS_MAKE_OPTS += ADDLIB='-lintl' >> +endif > > Why do you need both BR2_NEEDS_GETTEXT=y and BR2_PACKAGE_GETTEXT=y ? Historical accident: I wrote this patch before patch 1/2. Without patch 1: BR2_NEEDS_GETTEXT means: if a package needs intl, it needs to select gettext. libgpg-error, however, only links with libintl if it is available. So it doesn't really _need_ gettext. Only if the gettext package is actually selected, it will link with it. In other words, only if BR2_PACKAGE_GETTEXT=y, iputils needs to link with libintl. In the autobuild failure, gettext was selected. Since gettext sorts alphabetically before libgpg-error, it was already built when libgpg-error was built, so libgpg-error links with it, so iputils needs libintl. Then I wrote patch 1/2 to fix that dependency. In that patch, I decided to use BR2_NEEDS_GETTEXT to always force libgpg-error to link with libintl, because that is what is done by all other packages. As you write in your other reply, that's wrong though: a user who doesn't care about i18n would build libintl even though it isn't needed in this case. I just followed the pattern that is used everywhere else. Oh, and BR2_NEEDS_GETTEXT is still needed as well, because the gettext package can also be built for glibc and musl, but in that case gpg-error doesn't link with it (because the functionality is already available in libc). TBH, the more I think about it, the more I am convinced that we need a more fundamental solution: * Make gettext/iconv part of the toolchain; remove any explicit dependency on gettext. * Add a Config.in option to the toolchain (e.g. BR2_USE_LIBINTL) to enable i18n. This option just selects the gettext package. That's just to make it easier to the user, otherwise they'd have to go and select gettext in the target packages menus. * If a package really needs libintl, it will also select BR2_USE_LIBINTL if BR2_NEEDS_GETTEXT(_IF_LOCALE). * Perhaps also make BR2_PACKAGE_GETTEXT a blind option, is there any use for it in glibc/musl? * Add -lintl to link if BR2_USE_LIBINTL. Perhaps add a make variable: ifeq ($(BR2_USE_LIBINTL),y) TARGET_LIBINTL_IF_NEEDED = -lintl endif If BR2_PACKAGE_GETTEXT is not a blind option, then there is still a possibility that it is selected without BR2_USE_LIBINTL, so it becomes more complicated :-) Regards, Arnout
On 25-08-16 10:56, Arnout Vandecappelle wrote: > * Add -lintl to link if BR2_USE_LIBINTL. Perhaps add a make variable: > ifeq ($(BR2_USE_LIBINTL),y) Oh, and as per Baruch's suggestion, this could be ifeq ($(BR2_USE_LIBINTL)$(BR2_STATIC_LIBS),yy) Regards, Arnout > TARGET_LIBINTL_IF_NEEDED = -lintl > endif
Hello, On Thu, 25 Aug 2016 10:56:12 +0200, Arnout Vandecappelle wrote: > >> +ifeq ($(BR2_NEEDS_GETTEXT)$(BR2_PACKAGE_GETTEXT),yy) > >> +IPUTILS_MAKE_OPTS += ADDLIB='-lintl' > >> +endif > > > > Why do you need both BR2_NEEDS_GETTEXT=y and BR2_PACKAGE_GETTEXT=y ? > > Historical accident: I wrote this patch before patch 1/2. Without patch 1: > > BR2_NEEDS_GETTEXT means: if a package needs intl, it needs to select gettext. Correct. > libgpg-error, however, only links with libintl if it is available. So it > doesn't really _need_ gettext. Only if the gettext package is actually selected, > it will link with it. In other words, only if BR2_PACKAGE_GETTEXT=y, iputils > needs to link with libintl. In this case, as I said in my e-mail, the only thing you should do is: ifeq ($(BR2_PACKAGE_GETTEXT),y) <pkg>_DEPENDENCIES += gettext endif and not do *anything* in the Config.in. > In the autobuild failure, gettext was selected. Since gettext sorts > alphabetically before libgpg-error, it was already built when libgpg-error was > built, so libgpg-error links with it, so iputils needs libintl. > > Then I wrote patch 1/2 to fix that dependency. In that patch, I decided to use > BR2_NEEDS_GETTEXT to always force libgpg-error to link with libintl, because > that is what is done by all other packages. As you write in your other reply, > that's wrong though: a user who doesn't care about i18n would build libintl even > though it isn't needed in this case. I just followed the pattern that is used > everywhere else. Not correct. There are several packages that optionally depend on gettext. For example, look at coreutils, it does exactly what I saying above: it depends on gettext if BR2_PACKAGE_GETTEXT=y and that's it. Same for gdbm, gptfdisk, libconfuse, popt, etc, etc. So no, you've not followed the pattern used everywhere else :-) > Oh, and BR2_NEEDS_GETTEXT is still needed as well, because the gettext package > can also be built for glibc and musl, but in that case gpg-error doesn't link > with it (because the functionality is already available in libc). Just use the optional dependency I mentioned above. When the gettext package is built with a musl or glibc toolchain, it doesn't build and install a libintl library, since the C library already provides the functionality. So adding gettext as an optional dependency is just fine. > TBH, the more I think about it, the more I am convinced that we need a more > fundamental solution: Maybe, yes. > * Make gettext/iconv part of the toolchain; remove any explicit dependency on > gettext. > > * Add a Config.in option to the toolchain (e.g. BR2_USE_LIBINTL) to enable i18n. > This option just selects the gettext package. That's just to make it easier to > the user, otherwise they'd have to go and select gettext in the target packages > menus. > > * If a package really needs libintl, it will also select BR2_USE_LIBINTL if > BR2_NEEDS_GETTEXT(_IF_LOCALE). > > * Perhaps also make BR2_PACKAGE_GETTEXT a blind option, is there any use for it > in glibc/musl? There is: when you need gettext programs on the target, which is the case of the ecryptfs-utils package. Thomas
On 25-08-16 15:07, Thomas Petazzoni wrote: > Hello, > > On Thu, 25 Aug 2016 10:56:12 +0200, Arnout Vandecappelle wrote: [snip] >> Then I wrote patch 1/2 to fix that dependency. In that patch, I decided to use >> BR2_NEEDS_GETTEXT to always force libgpg-error to link with libintl, because >> that is what is done by all other packages. As you write in your other reply, >> that's wrong though: a user who doesn't care about i18n would build libintl even >> though it isn't needed in this case. I just followed the pattern that is used >> everywhere else. > > Not correct. There are several packages that optionally depend on > gettext. For example, look at coreutils, it does exactly what I saying > above: it depends on gettext if BR2_PACKAGE_GETTEXT=y and that's it. > > Same for gdbm, gptfdisk, libconfuse, popt, etc, etc. > > So no, you've not followed the pattern used everywhere else :-) Ahem. Looking back in my console logs: I grepped for BR2_NEEDS_GETTEXT and I found the pattern I mentioned. Self-fulfilling prophecy :-) >> Oh, and BR2_NEEDS_GETTEXT is still needed as well, because the gettext package >> can also be built for glibc and musl, but in that case gpg-error doesn't link >> with it (because the functionality is already available in libc). > > Just use the optional dependency I mentioned above. When the gettext > package is built with a musl or glibc toolchain, it doesn't build and > install a libintl library, since the C library already provides the > functionality. So adding gettext as an optional dependency is just fine. I mean it is needed (or rather, makes sense) for iputils. If BR2_PACKAGE_GETTEXT && !BR2_NEEDS_GETTEXT, then libgpg-error doesn't link with libintl, so iputils doesn't need -lintl. And since libintl doesn't even exist, adding -lintl will give an error. No? > >> TBH, the more I think about it, the more I am convinced that we need a more >> fundamental solution: > > Maybe, yes. But that's for post-2016.08 for sure :-) Regards, Arnout > >> * Make gettext/iconv part of the toolchain; remove any explicit dependency on >> gettext. >> >> * Add a Config.in option to the toolchain (e.g. BR2_USE_LIBINTL) to enable i18n. >> This option just selects the gettext package. That's just to make it easier to >> the user, otherwise they'd have to go and select gettext in the target packages >> menus. >> >> * If a package really needs libintl, it will also select BR2_USE_LIBINTL if >> BR2_NEEDS_GETTEXT(_IF_LOCALE). >> >> * Perhaps also make BR2_PACKAGE_GETTEXT a blind option, is there any use for it >> in glibc/musl? > > There is: when you need gettext programs on the target, which is the > case of the ecryptfs-utils package. > > Thomas >
Hello, On Thu, 25 Aug 2016 15:17:37 +0200, Arnout Vandecappelle wrote: > > Not correct. There are several packages that optionally depend on > > gettext. For example, look at coreutils, it does exactly what I saying > > above: it depends on gettext if BR2_PACKAGE_GETTEXT=y and that's it. > > > > Same for gdbm, gptfdisk, libconfuse, popt, etc, etc. > > > > So no, you've not followed the pattern used everywhere else :-) > > Ahem. Looking back in my console logs: I grepped for BR2_NEEDS_GETTEXT and I > found the pattern I mentioned. Self-fulfilling prophecy :-) Hehe :-) > >> Oh, and BR2_NEEDS_GETTEXT is still needed as well, because the gettext package > >> can also be built for glibc and musl, but in that case gpg-error doesn't link > >> with it (because the functionality is already available in libc). > > > > Just use the optional dependency I mentioned above. When the gettext > > package is built with a musl or glibc toolchain, it doesn't build and > > install a libintl library, since the C library already provides the > > functionality. So adding gettext as an optional dependency is just fine. > > I mean it is needed (or rather, makes sense) for iputils. If > BR2_PACKAGE_GETTEXT && !BR2_NEEDS_GETTEXT, then libgpg-error doesn't link with > libintl, so iputils doesn't need -lintl. And since libintl doesn't even exist, > adding -lintl will give an error. No? Hum, yes, I guess so. So, to sum up: * Your PATCH 1 needs to be adjusted to: ifeq ($(BR2_PACKAGE_GETTEXT),y) LIBGPG_ERROR_DEPENDENCIES += gettext endif * Your PATCH 2 needs to be adjusted to: # When gettext is enabled (BR2_PACKAGE_GETTEXT=y), and provides # libintl (BR2_NEEDS_GETTEXT=y), we need to link with libintl # explicitly when static linking, since our dependencies might use it. ifeq ($(BR2_NEEDS_GETTEXT)$(BR2_PACKAGE_GETTEXT)$(BR2_STATIC_LIBS),yyy) IPUTILS_MAKE_OPTS += ADDLIB='-lintl' endif Is this correct? Thomas
On 25-08-16 15:37, Thomas Petazzoni wrote: > Hello, > > On Thu, 25 Aug 2016 15:17:37 +0200, Arnout Vandecappelle wrote: [snip] >>>> Oh, and BR2_NEEDS_GETTEXT is still needed as well, because the gettext package >>>> can also be built for glibc and musl, but in that case gpg-error doesn't link >>>> with it (because the functionality is already available in libc). >>> >>> Just use the optional dependency I mentioned above. When the gettext >>> package is built with a musl or glibc toolchain, it doesn't build and >>> install a libintl library, since the C library already provides the >>> functionality. So adding gettext as an optional dependency is just fine. >> >> I mean it is needed (or rather, makes sense) for iputils. If >> BR2_PACKAGE_GETTEXT && !BR2_NEEDS_GETTEXT, then libgpg-error doesn't link with >> libintl, so iputils doesn't need -lintl. And since libintl doesn't even exist, >> adding -lintl will give an error. No? > > Hum, yes, I guess so. So, to sum up: > > * Your PATCH 1 needs to be adjusted to: > > ifeq ($(BR2_PACKAGE_GETTEXT),y) > LIBGPG_ERROR_DEPENDENCIES += gettext > endif > > * Your PATCH 2 needs to be adjusted to: > > # When gettext is enabled (BR2_PACKAGE_GETTEXT=y), and provides > # libintl (BR2_NEEDS_GETTEXT=y), we need to link with libintl > # explicitly when static linking, since our dependencies might use it. Small improvement: # When gettext is enabled (BR2_PACKAGE_GETTEXT=y), and provides # libintl (BR2_NEEDS_GETTEXT=y), libgpg-error will link with libintl, # and libgpg-error is pulled in by libgcrypt. Since iputils doesn't use # libtool, we have to link with libintl explicitly for static linking, And the whole thing should go under the libgcrypt condition. I'll fix up, test and resubmit this evening. Regards, Arnout > ifeq ($(BR2_NEEDS_GETTEXT)$(BR2_PACKAGE_GETTEXT)$(BR2_STATIC_LIBS),yyy) > IPUTILS_MAKE_OPTS += ADDLIB='-lintl' > endif > > Is this correct? > > Thomas >
diff --git a/package/iputils/iputils.mk b/package/iputils/iputils.mk index d6398f7..47f3e28 100644 --- a/package/iputils/iputils.mk +++ b/package/iputils/iputils.mk @@ -26,6 +26,10 @@ endif IPUTILS_MAKE_OPTS = $(TARGET_CONFIGURE_OPTS) USE_SYSFS=no USE_IDN=no\ CFLAGS="$(TARGET_CFLAGS) -D_GNU_SOURCE" +ifeq ($(BR2_NEEDS_GETTEXT)$(BR2_PACKAGE_GETTEXT),yy) +IPUTILS_MAKE_OPTS += ADDLIB='-lintl' +endif + ifeq ($(BR2_PACKAGE_LIBCAP),y) IPUTILS_MAKE_OPTS += USE_CAP=yes IPUTILS_DEPENDENCIES += libcap
libgcrypt pulls in gpg-error which links with libintl if available. Since iputils doesn't use libtool, -lintl has to be passed explicitly for static builds. Fixes: http://autobuild.buildroot.net/results/f81/f81eabb37788aa6dcdadf4034889c84bef78b876 http://autobuild.buildroot.net/results/f29/f296e8fba1e79f96d3d119aa4c8207ed4b80694b http://autobuild.buildroot.net/results/478/478a88cd892a119970cfca717ea7f5517bfc4cea http://autobuild.buildroot.net/results/920/920d2d1d967691779f79fa54a38fac5adc5a8ee4 Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> --- package/iputils/iputils.mk | 4 ++++ 1 file changed, 4 insertions(+)