Use Zlib as a package

[theta682]

1. automatic linking of the library and add include directory
2. properly specify include directories
3. don't force absolute paths for m library (require system provided)

[theta682]

In future, I'd suggest reading version information from configure.ac like in https://gitlab.com/libtiff/libtiff/-/blob/master/cmake/AutotoolsVersion.cmake

[theta682]

@ctruta can you review and merge this PR?

[ctruta]

Hello and thank you for your contribution.

There are a couple of fixes that I want to get out the door right now, with libpng-1.6.40, but I will take a closer look at this PR for version 1.6.41.

[theta682]

@ctruta in CMake 3.27 cmake_minimun_required will deprecate versions older than 3.5. It will produce a warning and later it will be completely removed.

[buxtonpaul]

Just to comment I have this commit and it resolves my issues using libpng as an imported project using Cmake Fetchpackage.

[theta682]

@ctruta it has been a while since 1.6.40 release. Can you merge this PR now?

[bebuch]

I'm also waiting for this. We needed to patch the latest release because our CMake 3.27 had some issues with it. This patch solves them. Would be great to see it in 1.6.41!

[theta682]

@ctruta the PR was rebased and ready for merge

[theta682]

@jbowler can you review this?

[jbowler]

@jbowler can you review this?

I can't because it is cmake; this is @ctruta's area and I know next to nothing about cmake.

[theta682]

@ctruta what else is needed to have this PR merged?

[jbowler]

Well, you asked me to review it and I can't do that but I can test it. It doesn't work. I first verified that a cmake build with the current libpng16 HEAD works; it all worked just fine in so far as it goes (it doesn't seem to run any tests, but I just typed "make").

With the PR (gh pr pull 475) "cmake" works too, it finds /usr/lib64/libz.so just like it does without the PR but then the build fails at the link of pngvalid. Curiously pngtest does link, so it is finding zlib somewhere, however pngvalid just fails with an undefined reference to crc32.

This is a standard system; Gentoo (admittedly a ~dev build) with zlib 1.3. It works without the change it does not work with it. There's absolutely nothing weird about the environment I used; it's just a git clone.

A clue might be that after the report about the missing symbol there's another one. Here's all the error output:

[ 88%] Linking C executable pngvalid
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: CMakeFiles/pngvalid.dir/contrib/libtests/pngvalid.c.o: undefined reference to symbol 'crc32'
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: /usr/lib64/libz.so.1: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
make[2]: *** [CMakeFiles/pngvalid.dir/build.make:98: pngvalid] Error 1
make[1]: *** [CMakeFiles/Makefile2:606: CMakeFiles/pngvalid.dir/all] Error 2
make: *** [Makefile:146: all] Error 2

[jbowler]

It's because pngvalid calls zlib::crc32 directly. It's not the only test program fails to build. You should test this stuff with an up-to-date GCC as well as clang. Other programs are failing because -lm isn't there; it looks like none of the external libraries are being added to the command line.

[theta682]

It's because pngvalid calls zlib::crc32 directly. It's not the only test program fails to build. You should test this stuff with an up-to-date GCC as well as clang. Other programs are failing because -lm isn't there; it looks like none of the external libraries are being added to the command line.

Thank you for testing. I'll update my PR to fix other components

[theta682]

@jbowler I force-pushed the fix. CMake automatically adds ZLIB::ZLIB and m libraries for all dependent executables

[jbowler]

That seems to fix it. Tested with:

CC="" CFLAGS="-Wall -Wextra" cmake
make all
make test

For gcc-11, gcc-12, gcc-13 and cmake. Also checked:

configure
make distcheck

Since that's the only thing that can break (so far as I can see) with this change (it only changes CMakeLists.txt)

Still needs a review from @ctruta but it seems to me that adding -lz and (if present) -lm in all cases is required anyway to support static builds.

[ctruta]

Hello and thank you @theta682 for your continuing insistence and patience. I wish I could have been more involved with the PNG project in the recent past, but "life happened".

In any case, as you probably noticed, I applied some (most?) of your initial changes in commit f229456, leaving out the "use zlib as a package" part. I'm glad that you and @jbowler were able to develop it further. Today I gave it another spin. It passed all of my offline and online testing, and it is now integrated.

Thanks again!