bvschaik / julius

An open source re-implementation of Caesar III
GNU Affero General Public License v3.0
2.79k stars 312 forks source link

CMake: Use pkg-config to find system libpng #674

Closed sulix closed 1 year ago

sulix commented 1 year ago

Many linux systems have multiple versions of libpng installed, and CMake's FindPNG can sometimes get confused and provide mismatched headers and library versions.

For example, on OpenSUSE Tumbleweed, find_package(PNG) will give the path to libpng16 headers (/usr/include/libpng16), but the libpng12 library (/usr/lib64/libpng.so, which is a symlink to /usr/lib64/libpng12.so).

This gives the following, somewhat inscrutable error:

/usr/lib64/gcc/x86_64-suse-linux/12/../../../../x86_64-suse-linux/bin/ld: CMakeFiles/julius.dir/src/graphics/screenshot.c.o: in function `image_write_header':
screenshot.c:(.text+0x26e): undefined reference to `png_set_longjmp_fn'
/usr/lib64/gcc/x86_64-suse-linux/12/../../../../x86_64-suse-linux/bin/ld: CMakeFiles/julius.dir/src/graphics/screenshot.c.o: in function `image_write_rows':
screenshot.c:(.text+0x364): undefined reference to `png_set_longjmp_fn'
collect2: error: ld returned 1 exit status

pkg-config allows searching for individual libpng versions, which are configured as separate libraries. Use these in Julius' CMakeLists.txt to prefer, in order (when SYSTEM_LIB is enabled)S:

This should work properly on any setup I can think of, and at least shouldn't be a regression if pkg_config isn't available.

bvschaik commented 1 year ago

If it's CMake's FindPNG that gets confused, shouldn't this be fixed in CMake's FindPNG?

Having this workaround in Julius kinda defeats the purpose of those FindXXX scripts.

QuakeIV commented 1 year ago

seems like pushing something upstream to cmake is self evidently the long term solution to that problem, not the short term one

sulix commented 1 year ago

Excellent points.

I'm not a CMake expert, but while I agree CMake's the better place to fix it, I think there are some reasons to support pkg-config directly here as well:

  1. It'll take a while for any CMake fix to land in the user's distro.
  2. CMake seems to be moving away from centrally-supplied FindXXX.cmake files, into preferring either the library to provide either a pkg-config file or its own CMake config, so it may be worth introducing support for these anyway. (For example, SDL3 is dropping sdl-config scripts in favour of these).

(The other reason is that I haven't worked out exactly how to fix this in or contribute to upstream CMake yet, and probably won't have that much time to look at it in the short term. Sorry!)

That being said, I'm not hugely concerned if this PR doesn't go in: I just wanted to make sure there was a description of how the problem can be solved somewhere.

bvschaik commented 1 year ago

Do you have any sources for point 2?

As for getting a fix in CMake: you can open an issue at their issue tracker, from my experience they're quite responsive.

crudelios commented 1 year ago

I'm pretty sure I saw something related to this before, explaining the current state of find_package is a mess.

sulix commented 1 year ago

Do you have any sources for point 2?

For the sdl-compat removal, see: https://github.com/libsdl-org/SDL/commit/e0d904e90b15c7be45210e51b8969d3beab71437 and https://github.com/libsdl-org/SDL/issues/6140 (which does revert this for SDL2, but notes that it won't exist in SDL3).

I can't remember exactly what page I was reading about the move away from FindXXX.cmake files. The Using Dependencies page does note that "module" mode is less reliable than "config" mode, but both of those are normally exposed with find_package(), so it doesn't sound as serious as I remembered…

sulix commented 1 year ago

As for getting a fix in CMake: you can open an issue at their issue tracker, from my experience they're quite responsive.

Thanks: I've sent a report upstream. https://gitlab.kitware.com/cmake/cmake/-/issues/24348

bvschaik commented 1 year ago

Closing since it's been reported to CMake.