| Message ID | 20161106163559.4788-1-s.martin49@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
Hello, On Sun, 6 Nov 2016 17:35:59 +0100, Samuel Martin wrote: > + # Try to find lib Z > + IF(BUILD_THIRDPARTY) > +@@ -35,6 +39,9 @@ IF(BUILD_THIRDPARTY) > + SET(PNG_INCLUDE_DIRNAME ${OPENJPEG_SOURCE_DIR}/thirdparty/libpng PARENT_SCOPE) > + ELSE (BUILD_THIRDPARTY) > + IF (ZLIB_FOUND) > ++ # Static only build: > ++ # it is not necessary to invoke pkg_check_module on libpng, because libpng > ++ # only depends on zlib, which is already checked. This is correct today, but in the future? The whole point of pkg-config is to not make this sort of assumption. > + FIND_PACKAGE(PNG) > + IF(PNG_FOUND) > + message(STATUS "Your system seems to have a PNG lib available, we will use it") > +@@ -66,12 +73,24 @@ IF(BUILD_THIRDPARTY) > + SET(OPJ_HAVE_LIBTIFF 1 PARENT_SCOPE) > + ELSE (BUILD_THIRDPARTY) > + FIND_PACKAGE(TIFF) > ++ # Static only build: > ++ # it is necessary to invoke pkg_check_module on libtiff since it may have > ++ # several other dependencies not declared by its cmake module, but they are > ++ # in the its pkgconfig module. > ++ IF(PKG_CONFIG_FOUND) > ++ FOREACH(pc_tiff_module tiff tiff3 tiff4 tiff-3 tiff-4 libtiff libtiff3 libtiff4 libtiff-3 libtiff-4) I fail to understand why one needs to call both FIND_PACKAGE(TIFF) and then check again with PKG_CHECK_MODULES(). Isn't the latter sufficient? Best regards, Thomas
Hi Thomas, all, On Sun, Nov 6, 2016 at 10:50 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Sun, 6 Nov 2016 17:35:59 +0100, Samuel Martin wrote: > > >> + # Try to find lib Z >> + IF(BUILD_THIRDPARTY) >> +@@ -35,6 +39,9 @@ IF(BUILD_THIRDPARTY) >> + SET(PNG_INCLUDE_DIRNAME ${OPENJPEG_SOURCE_DIR}/thirdparty/libpng PARENT_SCOPE) >> + ELSE (BUILD_THIRDPARTY) >> + IF (ZLIB_FOUND) >> ++ # Static only build: >> ++ # it is not necessary to invoke pkg_check_module on libpng, because libpng >> ++ # only depends on zlib, which is already checked. > > This is correct today, but in the future? The whole point of pkg-config > is to not make this sort of assumption. Some cmake modules are so poorly written, they only set the flags relative to the package itself (i.e. setting LDFLAGS="-L/usr/lib -lfoo" for package foo), and completly ignore/miss/forget the indirect dependencies that are required in the case of static build. However, cmake can leverage pkgconfig to set all these flags, so the proper fix is indeed using it in the module. But that's another story for another patch... ;-) A future-proof fix is fixing the FindTIFF.cmake module from the cmake package itself, making it using pkgconfig to get all the libs in the LDFLAGS instead of only -ltiff. This is not easily possible since we conditionally build the host-cmake package (patching host-cmake would mean no-op-ing commit c2d80a8c5d8b97cdc84c297a3d2d6896fff6560b). > >> + FIND_PACKAGE(PNG) >> + IF(PNG_FOUND) >> + message(STATUS "Your system seems to have a PNG lib available, we will use it") >> +@@ -66,12 +73,24 @@ IF(BUILD_THIRDPARTY) >> + SET(OPJ_HAVE_LIBTIFF 1 PARENT_SCOPE) >> + ELSE (BUILD_THIRDPARTY) >> + FIND_PACKAGE(TIFF) >> ++ # Static only build: >> ++ # it is necessary to invoke pkg_check_module on libtiff since it may have >> ++ # several other dependencies not declared by its cmake module, but they are >> ++ # in the its pkgconfig module. >> ++ IF(PKG_CONFIG_FOUND) >> ++ FOREACH(pc_tiff_module tiff tiff3 tiff4 tiff-3 tiff-4 libtiff libtiff3 libtiff4 libtiff-3 libtiff-4) > > I fail to understand why one needs to call both FIND_PACKAGE(TIFF) and > then check again with PKG_CHECK_MODULES(). Isn't the latter sufficient? Indeed FIND_PACKAGE(TIFF) is redundant with what PKG_CHECK_MODULES() does. But to give a chance to this patch to get upstream I have to take care platform missing pkgconfig (yeah! it exists :( ...), so FIND_PACKAGE() is always called, and PKG_CHECK_MODULES() is called only when available (mea culpa, the commit log does not reflect this). Regards,
Hello, On Mon, 7 Nov 2016 06:17:36 +0100, Samuel Martin wrote: > >> + # Try to find lib Z > >> + IF(BUILD_THIRDPARTY) > >> +@@ -35,6 +39,9 @@ IF(BUILD_THIRDPARTY) > >> + SET(PNG_INCLUDE_DIRNAME ${OPENJPEG_SOURCE_DIR}/thirdparty/libpng PARENT_SCOPE) > >> + ELSE (BUILD_THIRDPARTY) > >> + IF (ZLIB_FOUND) > >> ++ # Static only build: > >> ++ # it is not necessary to invoke pkg_check_module on libpng, because libpng > >> ++ # only depends on zlib, which is already checked. > > > > This is correct today, but in the future? The whole point of pkg-config > > is to not make this sort of assumption. > > Some cmake modules are so poorly written, they only set the flags > relative to the package itself (i.e. setting LDFLAGS="-L/usr/lib > -lfoo" for package foo), and completly ignore/miss/forget the indirect > dependencies that are required in the case of static build. > However, cmake can leverage pkgconfig to set all these flags, so the > proper fix is indeed using it in the module. But that's another story > for another patch... ;-) > > A future-proof fix is fixing the FindTIFF.cmake module from the cmake > package itself, making it using pkgconfig to get all the libs in the > LDFLAGS instead of only -ltiff. This is not easily possible since we > conditionally build the host-cmake package (patching host-cmake would > mean no-op-ing commit c2d80a8c5d8b97cdc84c297a3d2d6896fff6560b). This I understand. What I don't understand is your reasoning of "for libpng, we don't need to use pkg-config, because the only indirect dependency is zlib, which is already checked". This is not good, as you make an assumption on which indirect libraries libpng might use. Even though I agree libpng is unlikely to change and grow additional dependencies, you should also use pkg-config on libpng just like you're doing on libtiff. > > I fail to understand why one needs to call both FIND_PACKAGE(TIFF) and > > then check again with PKG_CHECK_MODULES(). Isn't the latter sufficient? > > Indeed FIND_PACKAGE(TIFF) is redundant with what PKG_CHECK_MODULES() > does. But to give a chance to this patch to get upstream I have to > take care platform missing pkgconfig (yeah! it exists :( ...), so > FIND_PACKAGE() is always called, and PKG_CHECK_MODULES() is called > only when available (mea culpa, the commit log does not reflect this). OK, makes sense. Thanks for the explanation, Thomas
On Mon, Nov 7, 2016 at 9:19 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Mon, 7 Nov 2016 06:17:36 +0100, Samuel Martin wrote: > >> >> + # Try to find lib Z >> >> + IF(BUILD_THIRDPARTY) >> >> +@@ -35,6 +39,9 @@ IF(BUILD_THIRDPARTY) >> >> + SET(PNG_INCLUDE_DIRNAME ${OPENJPEG_SOURCE_DIR}/thirdparty/libpng PARENT_SCOPE) >> >> + ELSE (BUILD_THIRDPARTY) >> >> + IF (ZLIB_FOUND) >> >> ++ # Static only build: >> >> ++ # it is not necessary to invoke pkg_check_module on libpng, because libpng >> >> ++ # only depends on zlib, which is already checked. >> > >> > This is correct today, but in the future? The whole point of pkg-config >> > is to not make this sort of assumption. >> >> Some cmake modules are so poorly written, they only set the flags >> relative to the package itself (i.e. setting LDFLAGS="-L/usr/lib >> -lfoo" for package foo), and completly ignore/miss/forget the indirect >> dependencies that are required in the case of static build. >> However, cmake can leverage pkgconfig to set all these flags, so the >> proper fix is indeed using it in the module. But that's another story >> for another patch... ;-) >> >> A future-proof fix is fixing the FindTIFF.cmake module from the cmake >> package itself, making it using pkgconfig to get all the libs in the >> LDFLAGS instead of only -ltiff. This is not easily possible since we >> conditionally build the host-cmake package (patching host-cmake would >> mean no-op-ing commit c2d80a8c5d8b97cdc84c297a3d2d6896fff6560b). > > This I understand. What I don't understand is your reasoning of "for > libpng, we don't need to use pkg-config, because the only indirect > dependency is zlib, which is already checked". This is not good, as you > make an assumption on which indirect libraries libpng might use. Ha! Sorry, I did not get you were talking about png here :s > > Even though I agree libpng is unlikely to change and grow additional > dependencies, you should also use pkg-config on libpng just like you're > doing on libtiff. As you said, I don't think it's gonna change, plus I may be sometime lazy! ;) > >> > I fail to understand why one needs to call both FIND_PACKAGE(TIFF) and >> > then check again with PKG_CHECK_MODULES(). Isn't the latter sufficient? >> >> Indeed FIND_PACKAGE(TIFF) is redundant with what PKG_CHECK_MODULES() >> does. But to give a chance to this patch to get upstream I have to >> take care platform missing pkgconfig (yeah! it exists :( ...), so >> FIND_PACKAGE() is always called, and PKG_CHECK_MODULES() is called >> only when available (mea culpa, the commit log does not reflect this). > > OK, makes sense. > > Thanks for the explanation, > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
Hello, On Sun, 6 Nov 2016 17:35:59 +0100, Samuel Martin wrote: > This change adds a patch to openjpeg fixing the tiff indirect > dependencies in case of static build. > > A similar patch for upstream master has been submitted [1]. > > Fixes: > http://autobuild.buildroot.net/results/d0d/d0d22727311d6300e0e400728126170407bfd699/ > and many others... > > [1] https://github.com/uclouvain/openjpeg/pull/866 > > Signed-off-by: Samuel Martin <s.martin49@gmail.com> > Cc: Olivier Schonken <olivier.schonken@gmail.com> > --- > ...tiff-append-flags-found-by-pkg-config-if-.patch | 72 ++++++++++++++++++++++ > 1 file changed, 72 insertions(+) > create mode 100644 package/openjpeg/0001-thirdparty-tiff-append-flags-found-by-pkg-config-if-.patch Applied to master, thanks. Thomas
diff --git a/package/openjpeg/0001-thirdparty-tiff-append-flags-found-by-pkg-config-if-.patch b/package/openjpeg/0001-thirdparty-tiff-append-flags-found-by-pkg-config-if-.patch new file mode 100644 index 0000000..be50902 --- /dev/null +++ b/package/openjpeg/0001-thirdparty-tiff-append-flags-found-by-pkg-config-if-.patch @@ -0,0 +1,72 @@ +From 38f50c7d9ad3ba06b64583045665203afb53cbd9 Mon Sep 17 00:00:00 2001 +From: Samuel Martin <s.martin49@gmail.com> +Date: Sun, 6 Nov 2016 16:29:08 +0100 +Subject: [PATCH] thirdparty: tiff: append flags found by pkg-config if + available + +This change allows to get all required CFLAGS/LDFLAGS in case of static only +build. + +This build issue [1] was triggered by the Buildroot farms. + +[1] http://autobuild.buildroot.net/results/d0d/d0d22727311d6300e0e400728126170407bfd699/build-end.log + +Signed-off-by: Samuel Martin <s.martin49@gmail.com> +--- + thirdparty/CMakeLists.txt | 23 +++++++++++++++++++++-- + 1 file changed, 21 insertions(+), 2 deletions(-) + +diff --git a/thirdparty/CMakeLists.txt b/thirdparty/CMakeLists.txt +index d1e68ca..225f51d 100644 +--- a/thirdparty/CMakeLists.txt ++++ b/thirdparty/CMakeLists.txt +@@ -1,5 +1,9 @@ + # 3rd party libs + ++IF(NOT BUILD_THIRDPARTY) ++ include(FindPkgConfig) ++ENDIF(NOT BUILD_THIRDPARTY) ++ + #------------ + # Try to find lib Z + IF(BUILD_THIRDPARTY) +@@ -35,6 +39,9 @@ IF(BUILD_THIRDPARTY) + SET(PNG_INCLUDE_DIRNAME ${OPENJPEG_SOURCE_DIR}/thirdparty/libpng PARENT_SCOPE) + ELSE (BUILD_THIRDPARTY) + IF (ZLIB_FOUND) ++ # Static only build: ++ # it is not necessary to invoke pkg_check_module on libpng, because libpng ++ # only depends on zlib, which is already checked. + FIND_PACKAGE(PNG) + IF(PNG_FOUND) + message(STATUS "Your system seems to have a PNG lib available, we will use it") +@@ -66,12 +73,24 @@ IF(BUILD_THIRDPARTY) + SET(OPJ_HAVE_LIBTIFF 1 PARENT_SCOPE) + ELSE (BUILD_THIRDPARTY) + FIND_PACKAGE(TIFF) ++ # Static only build: ++ # it is necessary to invoke pkg_check_module on libtiff since it may have ++ # several other dependencies not declared by its cmake module, but they are ++ # in the its pkgconfig module. ++ IF(PKG_CONFIG_FOUND) ++ FOREACH(pc_tiff_module tiff tiff3 tiff4 tiff-3 tiff-4 libtiff libtiff3 libtiff4 libtiff-3 libtiff-4) ++ pkg_check_modules(PC_TIFF QUIET ${pc_tiff_module}) ++ IF(PC_TIFF_FOUND) ++ break() ++ ENDIF(PC_TIFF_FOUND) ++ ENDFOREACH() ++ ENDIF(PKG_CONFIG_FOUND) + IF(TIFF_FOUND) + message(STATUS "Your system seems to have a TIFF lib available, we will use it") + SET(OPJ_HAVE_TIFF_H 1 PARENT_SCOPE) + SET(OPJ_HAVE_LIBTIFF 1 PARENT_SCOPE) +- SET(TIFF_LIBNAME ${TIFF_LIBRARIES} PARENT_SCOPE) +- SET(TIFF_INCLUDE_DIRNAME ${TIFF_INCLUDE_DIR} PARENT_SCOPE) ++ SET(TIFF_LIBNAME ${TIFF_LIBRARIES} ${PC_TIFF_STATIC_LIBRARIES} PARENT_SCOPE) ++ SET(TIFF_INCLUDE_DIRNAME ${TIFF_INCLUDE_DIR} ${PC_TIFF_STATIC_INCLUDE_DIRS} PARENT_SCOPE) + ELSE (TIFF_FOUND) # not found + SET(OPJ_HAVE_TIFF_H 0 PARENT_SCOPE) + SET(OPJ_HAVE_LIBTIFF 0 PARENT_SCOPE) +-- +2.10.2 +
This change adds a patch to openjpeg fixing the tiff indirect dependencies in case of static build. A similar patch for upstream master has been submitted [1]. Fixes: http://autobuild.buildroot.net/results/d0d/d0d22727311d6300e0e400728126170407bfd699/ and many others... [1] https://github.com/uclouvain/openjpeg/pull/866 Signed-off-by: Samuel Martin <s.martin49@gmail.com> Cc: Olivier Schonken <olivier.schonken@gmail.com> --- ...tiff-append-flags-found-by-pkg-config-if-.patch | 72 ++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 package/openjpeg/0001-thirdparty-tiff-append-flags-found-by-pkg-config-if-.patch