opencv / opencv

Open Source Computer Vision Library
https://opencv.org
Apache License 2.0
76.54k stars 55.64k forks source link

Use zlib target instead of ZLIB_INCLUDE_DIRS and ZLIB_LIBRARIES #25569

Open FantasqueX opened 1 month ago

FantasqueX commented 1 month ago

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

asmorkalov commented 1 month ago
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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

FantasqueX commented 4 weeks ago

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

FantasqueX commented 3 weeks ago

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

sturkmen72 commented 3 weeks ago

@FantasqueX let me try to help.

sturkmen72 commented 3 weeks ago

@FantasqueX let me try to help.

see https://github.com/sturkmen72/opencv/commit/853f838965f5c4090b55df9e033a720c5ebb5f1b i tested it on Windows.

https://github.com/opencv/opencv/compare/4.x...sturkmen72:opencv:zlib-help

FantasqueX commented 3 weeks ago

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

asmorkalov commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@asmorkalov Hi, could you please approve the workflow?

FantasqueX commented 1 week ago

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

FantasqueX commented 1 day ago

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