Skip to content

Commit

Permalink
update vendored libpng from 1.6.43 to 1.6.45.
Browse files Browse the repository at this point in the history
  • Loading branch information
@sezero
sezero committed Jan 11, 2025
1 parent f1ed915 commit 51307c4
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion .gitmodules
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
[submodule "external/libpng"]
path = external/libpng
url = https://github.com/libsdl-org/libpng.git
branch = v1.6.43-SDL
branch = v1.6.45-SDL
[submodule "external/libwebp"]
path = external/libwebp
url = https://github.com/libsdl-org/libwebp.git
Expand Down
2 changes: 1 addition & 1 deletion external/libpng
Submodule libpng updated 166 files

6 comments on commit 51307c4

@sezero
Copy link
Contributor Author

@sezero sezero commented on 51307c4 Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madebr : Other than removing the two PNG_BUILD_ZLIB lines at 417 and 418 in our cmake, does anything need to be done with ZLIB_ROOT thing? (Can I leave these to you?)

@sezero
Copy link
Contributor Author

@sezero sezero commented on 51307c4 Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I do this:

--- CMakeLists.txt~
+++ CMakeLists.txt
@@ -414,8 +414,7 @@
         sdl_check_project_in_subfolder(external/zlib zlib SDLIMAGE_VENDORED)
         add_subdirectory(external/zlib EXCLUDE_FROM_ALL)
         register_license(zlib external/zlib/LICENSE)
-        # PNG_BUILD_ZLIB variable is used by vendored libpng
-        set(PNG_BUILD_ZLIB ON CACHE BOOL "libpng option to tell it should use 'our' vendored ZLIB library" FORCE)
+        set(ZLIB_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/external/zlib;${CMAKE_CURRENT_BINARY_DIR}/external/zlib")
         # ZLIB_INCLUDE_DIR variable is used by vendored libpng
         set(ZLIB_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/external/zlib;${CMAKE_CURRENT_BINARY_DIR}/external/zlib" CACHE STRING "path of zlib, passed to libpng" FORCE)
         # ZLIB_LIBRARY variable is used by vendored libpng

.. then vendored build with shared libpng (thus with shared zlib) seems
to succeed:

However, with -DSDLIMAGE_PNG_SHARED=OFF I get the following error:

-- Building for target architecture: i686
-- Found ZLIB: zlibstatic  
-- Performing Test HAVE_LD_VERSION_SCRIPT
-- Performing Test HAVE_LD_VERSION_SCRIPT - Success
-- Found AWK program: /usr/bin/gawk
-- SDL3_image backends:
-- - enabled:  png
-- - disabled: stb imageio wic avif bmp gif jpg jxl lbm pcx pnm qoi svg tga tif webp xcf xpm xv
-- Configuring done
CMake Error: install(EXPORT "PNGTargets" ...) includes target "png_shared" which requires target "zlibstatic" that is not in any export set.
CMake Error: install(EXPORT "PNGTargets" ...) includes target "png_static" which requires target "zlibstatic" that is not in any export set.
-- Generating done
CMake Generate step failed.  Build files cannot be regenerated correctly.

I don't know how to handle things from there.

@sezero
Copy link
Contributor Author

@sezero sezero commented on 51307c4 Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, removing the restrictions from appending ZLIB_LIBRARY to INSTALL_EXTRA_TARGETS fixes the zlibstatic issue:

--- CMakeLists.txt~
+++ CMakeLists.txt
@@ -432,9 +431,7 @@ if(SDLIMAGE_ZLIB)
         # ZLIB_INCLUDE_DIRS variable is used by vendored libpng
         set(ZLIB_INCLUDE_DIRS "${ZLIB_INCLUDE_DIR}")
         target_include_directories(${ZLIB_LIBRARY} INTERFACE $<BUILD_INTERFACE:${ZLIB_INCLUDE_DIRS}>)
-        if(SDLIMAGE_ZLIB_SHARED OR NOT SDLIMAGE_BUILD_SHARED_LIBS)
-            list(APPEND INSTALL_EXTRA_TARGETS ${ZLIB_LIBRARY})
-        endif()
+        list(APPEND INSTALL_EXTRA_TARGETS ${ZLIB_LIBRARY})
         set_target_properties(${ZLIB_LIBRARY} PROPERTIES EXPORT_NAME external_zlib)
         add_library(SDL3_image::external_zlib ALIAS ${ZLIB_LIBRARY})
     else()

I don't know why the restriction was there, so I haven't done anything: Leaving to you.

@sezero
Copy link
Contributor Author

@sezero sezero commented on 51307c4 Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, see #502

@madebr
Copy link
Contributor

@madebr madebr commented on 51307c4 Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why the restriction was there, so I haven't done anything: Leaving to you.

We only want to install zlib when it's a shared library, or when it's built as a static library AND SDL_image is built as a static library.

Your fix is perfect: CMake was complaining the generated CMake config files had references to files that were not being installed.
Not installing the config files sidesteps the issues completely.

@sezero
Copy link
Contributor Author

@sezero sezero commented on 51307c4 Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. It took me a while to notice the root of the issue, though.

P.S.: Testing a backport to SDL2 branch now.

Please sign in to comment.