Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use zlib target instead of ZLIB_INCLUDE_DIRS and ZLIB_LIBRARIES #25569

Open
wants to merge 4 commits into
base: 4.x
Choose a base branch
from

Conversation

FantasqueX
Copy link
Contributor

@FantasqueX FantasqueX commented May 10, 2024

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

@FantasqueX FantasqueX marked this pull request as draft May 10, 2024 17:52
@FantasqueX FantasqueX marked this pull request as ready for review May 11, 2024 02:24
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.
@asmorkalov
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

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.

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@FantasqueX
Copy link
Contributor Author

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

@FantasqueX
Copy link
Contributor Author

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

@sturkmen72
Copy link
Contributor

@FantasqueX let me try to help.

@sturkmen72
Copy link
Contributor

sturkmen72 commented May 22, 2024

@FantasqueX let me try to help.

see sturkmen72@853f838 i tested it on Windows.

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

@FantasqueX
Copy link
Contributor Author

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

@asmorkalov
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants