bandwidthd: fix static build

Quoted 60 lines · Baruch Siach

Build against libpng requires -lz when built statically. Patch configure.ac to get the needed information from pkg-config.

This patch can not be directly upstreamed as it conflicts with upstream changes, but a proper fix would be based on the same idea.

Fixes:
http://autobuild.buildroot.net/results/e49/e496cc4315c06de61379222d3e842d4cd05b2f71/

Cc Nathaniel Roach <[email protected]>

Signed-off-by Baruch Siach <[email protected]>

---
 ...ac-fix-libpng-check-when-built-statically.patch | 29 ++++++++++++++++++++++
 package/bandwidthd/bandwidthd.mk                   |  2 +-
 2 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 package/bandwidthd/0001-configure.ac-fix-libpng-check-when-built-statically.patch

package/bandwidthd/0001-configure.ac-fix-libpng-check-when-built-statically.patch new file +29 -0

new file mode 100644
index 000000000000..f444ba70d513
@@ -0,0 +1,29 @@
+From 97998f008f5d900619194d80051dee1be68e4ace Mon Sep 17 00:00:00 2001
+Message-Id: <97998f008f5d900619194d80051dee1be68e4ace.1412231990.git.baruch@tkos.co.il>
+From: Baruch Siach <[email protected]>
+Date: Thu, 2 Oct 2014 09:12:28 +0300
+Subject: [PATCH] configure.ac: fix libpng check when built statically
+
+Find libpng library dependencies using pkg-config.
+
+Signed-off-by: Baruch Siach <[email protected]>
+---
+ configure.ac | 2 ++
+ 1 file changed, 2 insertions(+)
+
+diff --git a/configure.ac b/configure.ac
+index 3a2d0ba97bdf..9c2fa83cc93c 100644
+--- a/configure.ac
++++ b/configure.ac
+@@ -30,6 +30,8 @@ AC_CHECK_LIB(m, pow)
+ AC_CHECK_LIB(iconv, libiconv_open)
+ 
+ # Required Libraries
++PKG_CHECK_MODULES([libpng], libpng)
++LIBS="$LIBS $libpng_LIBS"
+ AC_CHECK_LIB(png, png_read_info, ,[AC_MSG_ERROR([Bandwidthd requires but cannot libpng])])
+ AC_CHECK_LIB(gd, gdImageCreate, ,[AC_MSG_ERROR([Bandwidthd requires but cannot find libgd])])
+ AC_CHECK_LIB(pcap, pcap_open_live, ,
+-- 
+2.1.0
+

package/bandwidthd/bandwidthd.mk +0 -0

index 3e11d6abb1e5..0b03b0b70278 100644
@@ -11,7 +11,7 @@ BANDWIDTHD_SITE = $(call github,nroach44,bandwidthd,$(BANDWIDTHD_VERSION))
 # download" by upstream.
 BANDWIDTHD_LICENSE = GPL
 

package/bandwidthd/bandwidthd.mk BANDWIDTHD_SITE = $(call github,nroach44,bandwidthd,$(BANDWIDTHD_VERSION)) · line 16

Quoted 4 lines

package/bandwidthd/bandwidthd.mk +1 -1

-BANDWIDTHD_DEPENDENCIES = gd libpng libpcap
+BANDWIDTHD_DEPENDENCIES = gd libpng libpcap host-pkgconf
 
 BANDWIDTHD_AUTORECONF = YES

Hi Baruch, I've already put a patch through [1] that bumps the version in BR, which AFAIK fixes the issues that we've had on the autobuilders.

I'm not sure what to do in this situation but I thought I'd just note this here in case my patch had been forgotten.

[1] http://patchwork.ozlabs.org/patch/381899/

Hi Nathaniel,

Quoted 7 lines · Nathaniel Roach

Quoted 6 lines · Baruch Siach

Build against libpng requires -lz when built statically. Patch configure.ac to get the needed information from pkg-config.

This patch can not be directly upstreamed as it conflicts with upstream changes, but a proper fix would be based on the same idea.

Quoted 8 lines

Quoted 6 lines

Fixes:
http://autobuild.buildroot.net/results/e49/e496cc4315c06de61379222d3e842d4cd05b2f71/

Cc Nathaniel Roach <[email protected]>

Signed-off-by Baruch Siach <[email protected]>

---

Hi Baruch, I've already put a patch through [1] that bumps the version in BR, which AFAIK fixes the issues that we've had on the autobuilders.

Well, I should have been aware to this patch since it seems I also commented on it, but I forgot. I still think this is the right solution, since it would also work if libpng changes its dependencies. Assuming, of course, that bandwidthd doesn't use libz directly.

Note that I also tried to upgrade to -r10, but build fails with this specific non-thread defconfig because bandwidthd now requires a toolchain supporting -lpthread. So technically this specific configuration is still broken after applying http://patchwork.ozlabs.org/patch/381899/.

baruch

Quoted 4 lines

I'm not sure what to do in this situation but I thought I'd just note this here in case my patch had been forgotten.

[1] http://patchwork.ozlabs.org/patch/381899/

Quoted 34 lines · Baruch Siach

Hi Nathaniel,

Quoted 7 lines · Nathaniel Roach

Quoted 6 lines · Baruch Siach

Build against libpng requires -lz when built statically. Patch configure.ac to get the needed information from pkg-config.

This patch can not be directly upstreamed as it conflicts with upstream changes, but a proper fix would be based on the same idea.

Quoted 8 lines

Quoted 6 lines

Fixes:
http://autobuild.buildroot.net/results/e49/e496cc4315c06de61379222d3e842d4cd05b2f71/

Cc Nathaniel Roach <[email protected]>

Signed-off-by Baruch Siach <[email protected]>

---

Hi Baruch, I've already put a patch through [1] that bumps the version in BR, which AFAIK fixes the issues that we've had on the autobuilders.

Well, I should have been aware to this patch since it seems I also commented on it, but I forgot. I still think this is the right solution, since it would also work if libpng changes its dependencies. Assuming, of course, that bandwidthd doesn't use libz directly.

Note that I also tried to upgrade to -r10, but build fails with this specific non-thread defconfig because bandwidthd now requires a toolchain supporting -lpthread. So technically this specific configuration is still broken after applying http://patchwork.ozlabs.org/patch/381899/.

baruch

Quoted 3 lines

I'm not sure what to do in this situation but I thought I'd just note this here in case my patch had been forgotten.

Quoted 1 line

Quoted 1 line

[1] http://patchwork.ozlabs.org/patch/381899/

I do vaguely remember some issue with -lpthread, could you send me the config?

(It was a while ago, but I'm sure I tested the patch with a basic ARM config, and IIRC I had the -lpthread issue once or twice but was never able to reproduce it after my patch)

Hi Nathaniel,

Quoted 40 lines · Nathaniel Roach

Quoted 24 lines · Baruch Siach

Hi Nathaniel,

Quoted 7 lines · Nathaniel Roach

Quoted 6 lines · Baruch Siach

Build against libpng requires -lz when built statically. Patch configure.ac to get the needed information from pkg-config.

This patch can not be directly upstreamed as it conflicts with upstream changes, but a proper fix would be based on the same idea.

Quoted 8 lines

Quoted 6 lines

Fixes:
http://autobuild.buildroot.net/results/e49/e496cc4315c06de61379222d3e842d4cd05b2f71/

Cc Nathaniel Roach <[email protected]>

Signed-off-by Baruch Siach <[email protected]>

---

Hi Baruch, I've already put a patch through [1] that bumps the version in BR, which AFAIK fixes the issues that we've had on the autobuilders.

Well, I should have been aware to this patch since it seems I also commented on it, but I forgot. I still think this is the right solution, since it would also work if libpng changes its dependencies. Assuming, of course, that bandwidthd doesn't use libz directly.

Quoted 11 lines

Note that I also tried to upgrade to -r10, but build fails with this specific non-thread defconfig because bandwidthd now requires a toolchain supporting -lpthread. So technically this specific configuration is still broken after applying http://patchwork.ozlabs.org/patch/381899/.

baruch

Quoted 4 lines

I'm not sure what to do in this situation but I thought I'd just note this here in case my patch had been forgotten.

[1] http://patchwork.ozlabs.org/patch/381899/

I do vaguely remember some issue with -lpthread, could you send me the config?

Quoted 3 lines

(It was a while ago, but I'm sure I tested the patch with a basic ARM config, and IIRC I had the -lpthread issue once or twice but was never able to reproduce it after my patch)

Get the config from
http://autobuild.buildroot.net/results/e49/e496cc4315c06de61379222d3e842d4cd05b2f71/config.

baruch

Quoted 10 lines · Baruch Siach

Quoted 6 lines · Nathaniel Roach

I do vaguely remember some issue with -lpthread, could you send me the config?

(It was a while ago, but I'm sure I tested the patch with a basic ARM config, and IIRC I had the -lpthread issue once or twice but was never able to reproduce it after my patch)

Get the config from
http://autobuild.buildroot.net/results/e49/e496cc4315c06de61379222d3e842d4cd05b2f71/config.

After testing that, you are indeed correct.

I've gone through the emails from a while ago and it's something like this:

Bandwidthd needs libpcap.
If libpcap was compiled with libusb that needs to be checked for/handled. And if libusb was used apparently /that/ is what needs pthreads, thus adding an additional dependency on the configure.ac.

I'm not sure why I added a hard pass/fail check for lpthread, as I would have thought I've done it like I handled like the lpng/zlib issue...

I'll have a play around and see what I can do.

Hi Nathaniel,

Quoted 24 lines · Nathaniel Roach

Quoted 10 lines · Baruch Siach

Quoted 6 lines · Nathaniel Roach

I do vaguely remember some issue with -lpthread, could you send me the config?

(It was a while ago, but I'm sure I tested the patch with a basic ARM config, and IIRC I had the -lpthread issue once or twice but was never able to reproduce it after my patch)

Get the config from
http://autobuild.buildroot.net/results/e49/e496cc4315c06de61379222d3e842d4cd05b2f71/config.

After testing that, you are indeed correct.

I've gone through the emails from a while ago and it's something like this:

Bandwidthd needs libpcap.
If libpcap was compiled with libusb that needs to be checked for/handled. And if libusb was used apparently /that/ is what needs pthreads, thus adding an additional dependency on the configure.ac.

I'm not sure why I added a hard pass/fail check for lpthread, as I would have thought I've done it like I handled like the lpng/zlib issue...

Quoted 1 line

I'll have a play around and see what I can do.

The libpcap package provides the helpful pcap-config utility that is more or less equivalent to pkg-config. grep pcap-config in buildroot to see how it is being used to fix static build problems.

baruch

Quoted 27 lines · Baruch Siach

Quoted 25 lines · Nathaniel Roach

Quoted 10 lines · Baruch Siach

Quoted 6 lines · Nathaniel Roach

I do vaguely remember some issue with -lpthread, could you send me the config?

(It was a while ago, but I'm sure I tested the patch with a basic ARM config, and IIRC I had the -lpthread issue once or twice but was never able to reproduce it after my patch)

Get the config from
http://autobuild.buildroot.net/results/e49/e496cc4315c06de61379222d3e842d4cd05b2f71/config.

After testing that, you are indeed correct.

I've gone through the emails from a while ago and it's something like this:

Bandwidthd needs libpcap.
If libpcap was compiled with libusb that needs to be checked for/handled. And if libusb was used apparently /that/ is what needs pthreads, thus adding an additional dependency on the configure.ac.

I'm not sure why I added a hard pass/fail check for lpthread, as I would have thought I've done it like I handled like the lpng/zlib issue...

I'll have a play around and see what I can do.

Quoted 3 lines

The libpcap package provides the helpful pcap-config utility that is more or less equivalent to pkg-config. grep pcap-config in buildroot to see how it is being used to fix static build problems.

Hi Baruch, I didn't end up using pcap-config, but could you check git version 4b07a0b3d3a280cdde582060cb29f3333ba4bf6e for me? It seems to work in the config you pasted above and I haven't had any issues in my other tests.

Quoted 4 lines

Hi Baruch, I didn't end up using pcap-config, but could you check git version 4b07a0b3d3a280cdde582060cb29f3333ba4bf6e for me? It seems to work in the config you pasted above and I haven't had any issues in my other tests.

I can confirm that upgrading bandwidthd to 4b07a0b3d3a280cdde582060cb29f3333ba4bf6e fixes the build of the config above. I still think that holding the complete knowledge of all your indirect dependencies, mandatory and optional, is not robust. A better future proof solution IMO is to use tools like pkg-config (or pcap-config in the case of libpcap) to list all dependencies that are in actual use for this specific build. But this is your call as upstream.

Baruch, Nathaniel,

Quoted 5 lines · Baruch Siach

Quoted 4 lines

Hi Baruch, I didn't end up using pcap-config, but could you check git version 4b07a0b3d3a280cdde582060cb29f3333ba4bf6e for me? It seems to work in the config you pasted above and I haven't had any issues in my other tests.

Quoted 7 lines

I can confirm that upgrading bandwidthd to 4b07a0b3d3a280cdde582060cb29f3333ba4bf6e fixes the build of the config above. I still think that holding the complete knowledge of all your indirect dependencies, mandatory and optional, is not robust. A better future proof solution IMO is to use tools like pkg-config (or pcap-config in the case of libpcap) to list all dependencies that are in actual use for this specific build. But this is your call as upstream.

I definitely agree with Baruch here. Using pkg-config is much more robust, as it figures out the indirect dependencies automatically, without hardcoding them in bandwidthd and/or Buildroot.

However, Baruch patch doesn't seem to be fully correct: it does both a PKG_CHECK_MODULES on libpng, and an AC_CHECK_LIB, which seems a bit redundant.

Therefore, Nathaniel, since you're now the upstream developer, could you make the necessary changes in configure.ac to use pkg-config instead, and then send a patch updating Buildroot to a new version of bandwidthd ?

In the mean time, I'll mark:

http://patchwork.ozlabs.org/patch/395809/
http://patchwork.ozlabs.org/patch/381899/

as 'Changes requested'.

Thanks,

Thomas

Yeah, I'm starting to lean that way too. Although I was a little hesitant adding another build dependency, I now realise that it's probably getting built anyway, and the version that's currently up on github was more a case of "getting it to work".

I've had a look at pcap-config and it doesn't seem to do -lpthreads (or from what I could see anything useful in this case), so I'll likely adapt Baruch's original patch into upstream for both lpcap and lpng.

It might take me a few days as my study load has increased, but I'll get it done.