-
-
Notifications
You must be signed in to change notification settings - Fork 55.6k
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
base: 4.x
Are you sure you want to change the base?
Conversation
0b785fc
to
bb70678
Compare
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.
bb70678
to
a5d85bd
Compare
|
@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 :) |
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.
I found the problem which only occurs when CMake < 3.18. https://cmake.org/cmake/help/latest/command/add_library.html
|
The configure issue on Android seems to be resolved on my machine. However, I don't fully understand. Any suggestion is welcome :) |
@asmorkalov Hi, could you please approve the workflow and review this PR :) |
I think I should give up. I'm unable to fix those failing checks. |
@FantasqueX let me try to help. |
see sturkmen72@853f838 i tested it on Windows. |
@asmorkalov Hi, thanks to @sturkmen72 we fix a bug in this PR. Could you please approve the workflow again?😊 |
iOS:
|
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 |
@asmorkalov Hi, could you please approve the workflow? |
@asmorkalov Hi, could you please approve the workflow again? I fixed the test locally. |
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
Patch to opencv_extra has the same branch name.