Conversation
|
I've pushed Please update your PR such that it points to patch into the new This makes a review easier (or possible at all ;) ) I've also created https://github.com/cpp-pm/libpng/tree/hunter-1.6.50-neroburner with a few fixes we should integrate/discuss |
|
ah I can change the base myself. Nice! 😁 |
| if(${CMAKE_MAJOR_VERSION} GREATER 3 OR | ||
| (${CMAKE_MAJOR_VERSION} EQUAL 3 AND ${CMAKE_MINOR_VERSION} GREATER_EQUAL 12)) | ||
| # For CMake >= 3.12, find_package(<PackageName>) searches prefixes given by | ||
| # <PackageName>_ROOT CMake variable and <PackageName>_ROOT | ||
| # environment variable. | ||
| cmake_policy(SET CMP0074 NEW) | ||
| endif() |
There was a problem hiding this comment.
1.6.50 has cmake_minimum_required(VERSION 3.14...4.0). So no need to guard here anymore
| hunter_add_package(ZLIB) | ||
| find_package(ZLIB CONFIG REQUIRED) |
There was a problem hiding this comment.
can we move that down to the original find_package(ZLIB) call?
| option(PNG_STATIC "Build libpng as a static library" ON) | ||
| if(APPLE) | ||
| option(PNG_FRAMEWORK "Build libpng as a framework bundle" ON) | ||
| option(PNG_FRAMEWORK "Build libpng as a framework bundle" OFF) |
There was a problem hiding this comment.
please explain the reasoning behind changing this flag
There was a problem hiding this comment.
I just followed the original Hunterfied CMakeLists.txt file.
Line 58 in 88f37c2
| if(UNIX AND NOT (APPLE OR BEOS OR HAIKU OR EMSCRIPTEN)) | ||
| check_library_exists(m pow "" PNG_HAVE_LIBM_POW) | ||
| if(LIB_M_REQUIRED) | ||
| set(M_LIBRARY m) |
There was a problem hiding this comment.
why have M_LIBRARY instead of using the already used PNG_LINK_LIBRARIES of this project?
| target_link_libraries(png_shared | ||
| PUBLIC ${PNG_LINK_LIBRARIES}) | ||
| endif() | ||
| include_directories(${CMAKE_CURRENT_SOURCE_DIR}) |
There was a problem hiding this comment.
I think this isn't needed, part of the public includes of the png library
There was a problem hiding this comment.
True, just tested and it's safe to remove.
| INTERFACE "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/libpng${PNGLIB_ABI_VERSION}>") | ||
| target_link_libraries(png_framework | ||
| PUBLIC ${PNG_LINK_LIBRARIES}) | ||
| PRIVATE ZLIB::zlib ${M_LIBRARY}) |
There was a problem hiding this comment.
why change from PUBLIC to PRIVATE
Can't we just leave the original line with PNG_LINK_LIBRARIES?
| endif() | ||
| target_link_libraries(${TARGET_NAME} PRIVATE ZLIB::zlib ${M_LIBRARY}) | ||
|
|
||
| if(PNG_TESTS AND PNG_SHARED) |
There was a problem hiding this comment.
we could replace the PNG_SHARED with BUILD_SHARED_LIBS
There was a problem hiding this comment.
I rolled back most of the constants to the original code base in #2
| endif() | ||
|
|
||
| if(PNG_SHARED AND PNG_TOOLS) | ||
| if(BUILD_SHARED_LIBS) |
There was a problem hiding this comment.
PNG_TOOLS got removed, please readd
| @@ -0,0 +1,16 @@ | |||
| include(CMakeFindDependencyMacro) | |||
There was a problem hiding this comment.
I think we can use the PNGConfig.cmake file already available
| ) | ||
|
|
||
| # Create a CMake Config File that can be used via find_package(PNG CONFIG) | ||
| if(NOT SKIP_INSTALL_CONFIG_FILE AND NOT SKIP_INSTALL_ALL) |
There was a problem hiding this comment.
can we keep most of the original code and change just the bits that need to be changed? Smaller patch size we need to reapply each update
|
@NeroBurner I tested your modification on the |
|
happy to hear it works for you. please respond to the review items as well and improve/fix them. I'd like to improve the PR even more |
2 participants
This pull request adds support for libpng v1.6.50.
It is an update based on the existing hunter-1.6.36 branch. The underlying libpng source code is from the official v1.6.50 release.
Changes are confined to the Hunter-specific CMake configuration files (CMakeLists.txt and cmake/Config.cmake.in).
Tested and confirmed working on macOS.