Kodi Community Forum

Full Version: cmake usage of pkg-config is broken
You're currently viewing a stripped down version of our content. View the full version with proper formatting.
Due to lack of developer area I use this part of the forum.

To make a long story short: the current usage of pkg-config is broken, it just throws results from pkg-config away.

Now a few questions about how to address the problem:

On which platforms is pkg-config expected to exist? I just noticed that some of the Linux Jenkins builds has a private pkg-config. Was it just forgotten to add? Realistically every Linux installation out there will have a copy of it, especially in the light of the fact that kodi relies on a large number of external libraries which will most likely fail to build if pkg-config does not exist. I would make it a hard failure if ${PKG_CONFIG_FOUND} is not set after find_package(PkgConfig),  if "UNIX" is set, which would cover Linux and FreeBSD. cmake will fail anyway for me if pkg-config is moved away. Any opinions on that?

Most of the *.cmale files that make  use of pkg_check_modules should probably be wrapped like:


if (PKG_CONFIG_FOUND)
pkg_check_modules(ExtLib extlib QUIET)
else()
find_path(ExtLib_INCLUDE_DIRS NAMES extlib/extlib.h)
find_library(ExtLib_LIBRARIES NAMES extlib libextlib)
include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(ExtLib REQUIRED_VARS ExtLib_LIBRARY ExtLib_INCLUDE_DIRS)
endif()
if (ExtLib_FOUND)
if(NOT TARGET ExtLib::ExtLib)
add_library(ExtLib::ExtLib UNKNOWN IMPORTED)
set_target_properties(ExtLib::ExtLib PROPERTIES IMPORTED_LOCATION "${ExtLib_LIBRARIES}" INTERFACE_INCLUDE_DIRECTORIES "${ExtLib_INCLUDE_DIRS}")
endif()
endif()

So essentially mimic the pkg_check_modules result for remaining systems that do not have pkg-config.

I have to find out of set_target_properties is actually complete, there are likely cases where *_CLAGS and *_LDFLAGS have to be used.

I'm willing to do the conversion, just need some input as stated above.

Thanks.
Umm there's a whole development section just below the discussion one?

Thread moved to Kodi application development
(2019-03-21, 21:29)DarrenHill Wrote: [ -> ]Umm there's a whole development section just below the discussion one?
Well the UI layout was so weird that the page had just these GSOC* sub-forums, and not this "Kodi Application" forum. Thanks anyway.
(2019-03-21, 17:24)olh_ Wrote: [ -> ]To make a long story short: the current usage of pkg-config is broken, it just throws results from pkg-config away.
After poking around in the buildsystem, the bug is that (for example) core_required_dep() gets something like "ASS", which uses "FindASS", which exports among others "PC_ASS_INCLUDE_DIRS", which then is unneccessarly translated to "ASS_INCLUDE_DIR", which is again translated to "ASS_INCLUDE_DIRS", which then is passed to "SYSTEMS_INCLUDES", which finally ends up in the call to include_directories(). Same goes for ASS_LIBRARIES and the *_DEFINES.

So the first step would be to split detection into pkgconfig and non-pkgconfig, both have to export the same PREFIX, like ASS in my example. As a result, the correct verbatim pkgconfig value will by used for building.
No it's not broken. It is good practice _not to trust_ pkg-config alone. We did that in the beginning and had all sorts of issues with (broken) libs randomly setting bogus flags and breaking builds in arcane ways.
Do you have an example of such breakage? Looks like one breakage is traded by another.
There have been quite a few, sadly not easy to find without going through a ton of git history, so I don't have an example ready. But rest assured we have good reasons for doing it like this.
How about you tell us what your actual issue is? What is not working?
pkg-config has a Required.private field which lists other required includes so that at compile time, but not linktime, interfaces can be found. There is also Libs.private, which is for statically linking. Static linking is not used by me, so I can not tell how broken the current way of pkg-config usage is in this regard. But Required.private must be handled, just by using pkgconfig verbatim. The fix for Waylandpp looks like that:


pkg_check_modules(WAYLANDPP wayland-client++ wayland-egl++ wayland-cursor++ QUIET)
if(WAYLANDPP_FOUND)
  pkg_get_variable(WAYLANDPP_PKGDATADIR wayland-client++ pkgdatadir)
  # Promote to cache variables so all code can access it
  set(WAYLANDPP_PROTOCOLS_DIR "${WAYLANDPP_PKGDATADIR}/protocols" CACHE INTERNAL "")
  set(WAYLANDPP_DEFINITIONS -DHAVE_WAYLAND=1)
endif()
pkg_check_modules(WAYLANDPP_SCANNER wayland-scanner++ QUIET)
if(WAYLANDPP_SCANNER_FOUND)
  pkg_get_variable(WAYLANDPP_SCANNER_HOST wayland-scanner++ wayland_scannerpp)
  find_program(WAYLANDPP_SCANNER wayland-scanner++ PATHS ${WAYLANDPP_SCANNER_HOST})
endif()

Without it, the required wayland flags are ignored.

Also the PREFIX_CFLAGS_OTHER setting has to be considered. There are a few packages in my build that do set extra defines that way. If they are essential or not, no idea at this point. At least they compile, not data about runtime behavior at this point.
(2019-03-22, 19:13)olh_ Wrote: [ -> ]  set(WAYLANDPP_DEFINITIONS -DHAVE_WAYLAND=1)
To make the above complete, it sould proably be set(WAYLANDPP_DEFINITIONS -DHAVE_WAYLAND=1 ${WAYLANDPP_CFLAGS_OTHER}), just in case there will be required content.
And this goes essentially for most of the other FindXYZ.cmake files.
since you seem to have issues with wayland, although you still didn't say which exactly, here is an example of the issues plain pkg-config uses can cause: 13810 (PR)
(2019-03-23, 09:29)wsnipex Wrote: [ -> ]since you seem to have issues with wayland, although you still didn't say which exactly, here is an example of the issues plain pkg-config uses can cause: 13810 (PR)

This pull request does not list an example with details. The only usable detail is 'user uses LinuxFromScratch', which by itself most likely means 'user does not know enough how to build a valid system'. This can not be used as evidence that pkg-config output is unreliable.

Let me point you to pkg-config FAQ #2 at https://people.freedesktop.org/~dbn/pkg-...guide.html:
Quote:My library z installs header files which include libx headers. What do I put in my z.pc file?

If the x library has pkg-config support, add it to the Requires.private field. If it does not, augment the Cflags field with the necessary compiler flags for using the libx headers. In either case, pkg-config will output the compiler flags when --static is used or not.

If a pkgA needs header files from pkgB, then pkgA must use 'Requires.private: pkgB' to make sure additional CFLAGS and most importantly the required include paths of pkgB. Kodi just throws all that info away by rechecking if "pkgA.h" really exists in PC_pkgA_INCLUDE_DIRS.

WaylandPP is actually a very good example of breakage caused by the way Kodi constructs the compiler flags. The headers of WaylandPP do include headers of Wayland, but WaylandPP does not directly expose the linktime ABI to its users. It all happens to work if Wayland goes straight into /usr/include, then the breakage will remain unnoticed. If Wayland is in a subdirectory of a system include path, WaylandPP can not possibly work.

I briefly looked through all *.cmake files that use pkg_check_modules(). There are gems like "set(ALSA_INCLUDE_DIRS "") # Don't want these added as 'timer.h' is a dangerous file". This looks like a valid workaround. As of today alsa.pc still includes /usr/include/alsa despite the fact that most applications seem to use <alsa/asoundlib.h> (or whateve the appropriate API would be). That comment would need an refresh to really state what is broken. It is not plain the timer.h, but the fact that the 'alsa' suffix in the emitted include path causes trouble.
I know how pkg-config works. But you still didn't show an actual error and why you think you need static linking when NOT using our depends system(that works fine with linking waylandpp)
I did not say that static linking is required. I did say that the usage of "WaylandPP" can not work if "WaylandPP" continues to rely on the headers provided by "Wayland" AND if those headers are outside of the standard include paths.

But yeah, if you want some error: it is something like "wayland-client-core.h not found".