Use zlib target instead of ZLIB_INCLUDE_DIRS and ZLIB_LIBRARIES #25569

FantasqueX · 2024-05-10T17:08:50Z

Currently, when one target needs zlib as a dependency, it needs to use both ocv_include_directories and target_link_libraries. However in modern CMake, only target_link_libraries is needed if zlib has a target_include_directories. In this patch, if zlib is found by find_package, an ALIAS target zlib will be created. CMake >= 3.1 ensures a target ZLIB::ZLIB.
https://cmake.org/cmake/help/latest/module/FindZLIB.html
If zlib is built, an library target zlib will be created.

Tested zlib (built or found) and zlib-ng each with libpng, libtiff and openexr on Windows and ArchLinux.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
The PR is proposed to the proper branch
There is a reference to the original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

asmorkalov · 2024-05-13T08:42:26Z

2024-05-11T12:51:24.1604533Z -- Check if the system is big endian - little endian
2024-05-11T12:51:24.1736149Z -- Found ZLIB: /usr/lib/aarch64-linux-gnu/libz.so (found suitable version "1.2.11", minimum required is "1.2.3") 
2024-05-11T12:51:24.1739357Z CMake Error at cmake/OpenCVFindLibsGrfmt.cmake:33 (add_library):
2024-05-11T12:51:24.1789268Z   add_library cannot create ALIAS target "zlib" because target "ZLIB::ZLIB"
2024-05-11T12:51:24.1790475Z   is imported but not globally visible.
2024-05-11T12:51:24.1791224Z Call Stack (most recent call first):
2024-05-11T12:51:24.1791936Z   CMakeLists.txt:820 (include)

FantasqueX · 2024-05-13T09:48:59Z

@asmorkalov Hi, thanks for your review. Could you provide your CMake command and some information about your environment? So, I could try to reproduce the error :)

asmorkalov · 2024-05-13T11:30:17Z

It happens with all Linux environments in CI. Pipelines: https://github.com/opencv/ci-gha-workflow/tree/main/.github/workflows, Dockerfiles: https://github.com/opencv-infrastructure/opencv-gha-dockerfile. Also you can extract all commands and env settings from CI raw log.

FantasqueX · 2024-05-13T14:33:20Z

I found the problem which only occurs when CMake < 3.18.

https://cmake.org/cmake/help/latest/command/add_library.html

New in version 3.18: An ALIAS can target a non-GLOBAL Imported Target. Such alias is scoped to the directory in which it is created and below. The ALIAS_GLOBAL target property can be used to check if the alias is global or not.

FantasqueX · 2024-05-14T17:49:05Z

The configure issue on Android seems to be resolved on my machine. However, I don't fully understand. Any suggestion is welcome :)

FantasqueX · 2024-05-20T15:41:24Z

@asmorkalov Hi, could you please approve the workflow and review this PR :)

FantasqueX · 2024-05-21T15:23:50Z

I think I should give up. I'm unable to fix those failing checks.

sturkmen72 · 2024-05-21T16:07:21Z

@FantasqueX let me try to help.

sturkmen72 · 2024-05-22T14:53:38Z

@FantasqueX let me try to help.

see sturkmen72@853f838 i tested it on Windows.

4.x...sturkmen72:opencv:zlib-help

FantasqueX · 2024-05-22T15:32:34Z

@asmorkalov Hi, thanks to @sturkmen72 we fix a bug in this PR. Could you please approve the workflow again?😊

asmorkalov · 2024-05-25T10:13:22Z

iOS:

-- Configuring done
CMake Error in 3rdparty/zlib/CMakeLists.txt:
  Target "zlib" INTERFACE_INCLUDE_DIRECTORIES property contains path:

    "/Users/opencv-cn/GHA-OCV-1/_work/opencv/opencv/ios/build/build-armv7-iphoneos/3rdparty/zlib"

  which is prefixed in the build directory.


CMake Error in 3rdparty/zlib/CMakeLists.txt:
  Target "zlib" INTERFACE_INCLUDE_DIRECTORIES property contains path:

    "/Users/opencv-cn/GHA-OCV-1/_work/opencv/opencv/opencv/3rdparty/zlib"

  which is prefixed in the source directory.


CMake Generate step failed.  Build files cannot be regenerated correctly.
-- Generating done

FantasqueX · 2024-05-30T15:37:20Z

I pushed a commit to fix iOS build according to https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source
Since I don't have a Mac device, I cannot test it locally :(

FantasqueX · 2024-06-03T07:04:55Z

@asmorkalov Hi, could you please approve the workflow?

FantasqueX · 2024-06-05T15:12:10Z

@asmorkalov Hi, could you please approve the workflow again? I fixed the test locally.

FantasqueX · 2024-06-17T10:43:45Z

@asmorkalov Hi, could you please approve the workflow again? I fixed the test locally.

asmorkalov · 2024-06-18T15:21:31Z

3rdparty/zlib-ng/CMakeLists.txt

-if(NOT BUILD_SHARED_LIBS)
-  ocv_install_target(${ZLIB_LIBRARY} EXPORT OpenCVModules ARCHIVE DESTINATION ${OPENCV_3P_LIB_INSTALL_PATH} COMPONENT dev)
-endif()


It should break static linkage, if OpenCV is build against own zlib-ng, but not system-wide.

Sorry for late reply. This ocv_install_target statement isn't removed. It is moved to outer space because both static linkage and dynamic linkage need this statement in Android build. So, this will only affect dynamic linkage.

We should not redistribute system or sysroot libraries even for static OpenCV builds. Only own 3rdparty built binaries are redistributed.

Which builder configuration does check of your modification?

https://github.com/opencv/ci-gha-workflow/blob/main/.github/workflows/OCV-PR-4.x-Android-Test.yaml
However, I cannot find previous build failure log.

3rdparty/zlib/CMakeLists.txt

opencv-alalek · 2024-07-01T17:27:39Z

3rdparty/libpng/CMakeLists.txt

@@ -75,7 +75,7 @@ if(MSVC)
 endif(MSVC)

 add_library(${PNG_LIBRARY} STATIC ${OPENCV_3RDPARTY_EXCLUDE_FROM_ALL} ${lib_srcs} ${lib_hdrs})
-target_link_libraries(${PNG_LIBRARY} ${ZLIB_LIBRARIES})


_LIBRARY is not used in documentation. Only _LIBRARIES is there:

https://cmake.org/cmake/help/book/mastering-cmake/chapter/Finding%20Packages.html

The same is related to FindZLIB: https://cmake.org/cmake/help/latest/module/FindZLIB.html

Yeah. Previously OpenCV used this variable. So, I used it. Essentially, we just need a name for interface library if we link system zlib and target library if we build zlib manually.

opencv-alalek · 2024-07-01T17:38:37Z

3rdparty/zlib-ng/CMakeLists.txt

-if(NOT BUILD_SHARED_LIBS)
-  ocv_install_target(${ZLIB_LIBRARY} EXPORT OpenCVModules ARCHIVE DESTINATION ${OPENCV_3P_LIB_INSTALL_PATH} COMPONENT dev)
-endif()


We should not redistribute system or sysroot libraries even for static OpenCV builds. Only own 3rdparty built binaries are redistributed.

Which builder configuration does check of your modification?

asmorkalov · 2024-09-13T10:23:17Z

@FantasqueX please rebase the patch and fix conflicts.

FantasqueX · 2024-09-13T10:27:03Z

I think I can rebase the patch this weekend :)

Currently, when one target needs zlib as a dependency, it needs to use both ocv_include_directories and target_link_libraries. However in modern CMake, only target_link_libraries is needed if zlib has a target_include_directories. In this patch, if zlib is found by find_package, an ALIAS target zlib will be created. CMake >= 3.1 ensures a target ZLIB::ZLIB. https://cmake.org/cmake/help/latest/module/FindZLIB.html If zlib is built, an library target zlib will be created.

3rdparty/zlib-ng/CMakeLists.txt

3rdparty/zlib/CMakeLists.txt

FantasqueX · 2024-09-27T15:48:51Z

@asmorkalov Could you please elaborate why those changes cause breakage? I still hope this patch will be merged.

FantasqueX marked this pull request as draft May 10, 2024 17:52

FantasqueX force-pushed the use-zlib-target-1 branch from 0b785fc to bb70678 Compare May 11, 2024 02:23

FantasqueX marked this pull request as ready for review May 11, 2024 02:24

FantasqueX force-pushed the use-zlib-target-1 branch from bb70678 to a5d85bd Compare May 11, 2024 03:55

asmorkalov added category: build/install category: 3rdparty labels May 13, 2024

asmorkalov self-requested a review May 13, 2024 08:12

asmorkalov added this to the 4.11.0 milestone Jun 18, 2024

asmorkalov reviewed Jun 18, 2024

View reviewed changes

asmorkalov self-assigned this Jun 18, 2024

asmorkalov requested a review from opencv-alalek July 1, 2024 09:46

opencv-alalek reviewed Jul 1, 2024

View reviewed changes

FantasqueX force-pushed the use-zlib-target-1 branch from 7bbc378 to fce4fa0 Compare September 17, 2024 08:38

fix zlib-ng version

e334eab

asmorkalov reviewed Sep 17, 2024

View reviewed changes

3rdparty/zlib-ng/CMakeLists.txt Show resolved Hide resolved

3rdparty/zlib/CMakeLists.txt Show resolved Hide resolved

3rdparty/zlib/CMakeLists.txt Show resolved Hide resolved

asmorkalov modified the milestones: 4.11.0, 4.12.0 Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use zlib target instead of ZLIB_INCLUDE_DIRS and ZLIB_LIBRARIES #25569

Use zlib target instead of ZLIB_INCLUDE_DIRS and ZLIB_LIBRARIES #25569

FantasqueX commented May 10, 2024 •

edited

Loading

asmorkalov commented May 13, 2024

FantasqueX commented May 13, 2024

asmorkalov commented May 13, 2024

FantasqueX commented May 13, 2024

FantasqueX commented May 14, 2024

FantasqueX commented May 20, 2024

FantasqueX commented May 21, 2024

sturkmen72 commented May 21, 2024

sturkmen72 commented May 22, 2024 •

edited

Loading

FantasqueX commented May 22, 2024

asmorkalov commented May 25, 2024

FantasqueX commented May 30, 2024 •

edited

Loading

FantasqueX commented Jun 3, 2024

FantasqueX commented Jun 5, 2024

FantasqueX commented Jun 17, 2024

asmorkalov Jun 18, 2024

FantasqueX Jun 25, 2024

opencv-alalek Jul 1, 2024

FantasqueX Jul 2, 2024

opencv-alalek Jul 1, 2024

FantasqueX Jul 2, 2024

opencv-alalek Jul 1, 2024

asmorkalov commented Sep 13, 2024

FantasqueX commented Sep 13, 2024

FantasqueX commented Sep 27, 2024

Use zlib target instead of ZLIB_INCLUDE_DIRS and ZLIB_LIBRARIES #25569

Are you sure you want to change the base?

Use zlib target instead of ZLIB_INCLUDE_DIRS and ZLIB_LIBRARIES #25569

Conversation

FantasqueX commented May 10, 2024 • edited Loading

Pull Request Readiness Checklist

asmorkalov commented May 13, 2024

FantasqueX commented May 13, 2024

asmorkalov commented May 13, 2024

FantasqueX commented May 13, 2024

FantasqueX commented May 14, 2024

FantasqueX commented May 20, 2024

FantasqueX commented May 21, 2024

sturkmen72 commented May 21, 2024

sturkmen72 commented May 22, 2024 • edited Loading

FantasqueX commented May 22, 2024

asmorkalov commented May 25, 2024

FantasqueX commented May 30, 2024 • edited Loading

FantasqueX commented Jun 3, 2024

FantasqueX commented Jun 5, 2024

FantasqueX commented Jun 17, 2024

asmorkalov Jun 18, 2024

Choose a reason for hiding this comment

FantasqueX Jun 25, 2024

Choose a reason for hiding this comment

opencv-alalek Jul 1, 2024

Choose a reason for hiding this comment

FantasqueX Jul 2, 2024

Choose a reason for hiding this comment

opencv-alalek Jul 1, 2024

Choose a reason for hiding this comment

FantasqueX Jul 2, 2024

Choose a reason for hiding this comment

opencv-alalek Jul 1, 2024

Choose a reason for hiding this comment

asmorkalov commented Sep 13, 2024

FantasqueX commented Sep 13, 2024

FantasqueX commented Sep 27, 2024

FantasqueX commented May 10, 2024 •

edited

Loading

sturkmen72 commented May 22, 2024 •

edited

Loading

FantasqueX commented May 30, 2024 •

edited

Loading