pnggroup / libpng

LIBPNG: Portable Network Graphics support, official libpng repository
http://libpng.sf.net
Other
1.25k stars 614 forks source link

libpng configuration package is completely broken #179

Open rcdailey opened 7 years ago

rcdailey commented 7 years ago

So right now, the CMake script attempts to install exports for libpng but they're not usable by find_package() because they are not following the proper rules documented by CMake. I'm happy to fix this for you, but I want to make you aware of certain changes that need to happen.

The fact that the project is named "libpng" instead of "png" will make things quirky on linux because the 'lib' part is usually implied on that platform. Since it doesn't seem reasonable to rename everything to just "png" (unless you are ok with this; it's a lot of refactoring but possible to do), the config package installation logic will be inconsistent.

In most cases, users will be expected to do this:

find_package(png CONFIG)

This will trigger the config package search for libpng, as opposed to the "FindPNG.cmake" that comes with CMake (this is the module package structure which is the older way of doing it).

There are 2 main requirements that I think you want:

  1. New installations of libpng should remain structurally backward compatible with module package (i.e. people that use FindPNG.cmake should still be able to do so)
  2. New installations of libpng should install config and starget scripts in CMake's deterministic structure so that config package searches work (this will be a new feature since the current way it is being done is non-functional).

When users do find_package(png CONFIG), CMake searches for a png-config.cmake, which will in turn do an include() to include png-targets.cmake. So the final install structure would look like:

Under ${CMAKE_INSTALL_PREFIX}/lib/png/[cmake/]:

png-config.cmake
png-targets.cmake

You're already sort-of doing png-config.cmake, but the name is incorrect. You're installing the name as libpng, which will require users to do find_package(libpng), which breaks interoperability with the module package style.

How much flexibility do I have in restructuring and renaming things from the installation logic standpoint? I want to fix all of this but I need to know what I can and can't do. Happy to go over detail items if you want.

There's a lot of weirdness with the library naming as well, but I recognize for backward compatibility reasons, you probably can't allow the naming convention for the libraries to change. Normally the VERSION and SOVERSION symlinking and management is done for you by CMake, if you set it up right. Right now, the script assumes responsibility for doing all of that by hand.

skirk commented 7 years ago

I too would like to see the changes suggested by @rcdailey to get into repo. As a solution for now I've added few lines to my (forked) CMakeLists:

#install Config file for find_package calls in user projects 
 configure_file(cmake/libpngConfig.cmake.in cmake/lib${PNG_LIB_NAME}Config.cmake @ONLY)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/cmake/lib${PNG_LIB_NAME}Config.cmake DESTINATION lib/libpng)

However this is far from perfect as I need to find the package using find_package(libpng16 CONFIG)

and in order to find the generated Config files in dependent projects I currently need to add some prefix paths such as: -DCMAKE_PREFIX_PATH:Path=${CMAKE_INSTALL_PREFIX}/lib/libpng)

All this could be streamlined with @rcdailey suggested changes.

BotellaA commented 1 year ago

Any update on this issue?

jbowler commented 8 months ago

There have been many changes to the cmake build system which was unmaintained at the time you wrote the original bug report. It should be fixed now and it would certainly be helpful to know if issues like this still exist.

jbowler commented 1 month ago

@ctruta: seems to be fixed.