-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Update ZLib 3rdparty library to use ZLib-ng and Minizip-ng so patching is not necessary #7022
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # 3rdparty/vtk/vtk_build.cmake
… its place. (The removes the patch to zlib required for it to use integrated minizip)
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
PACKAGE ZLIB | ||
TARGETS ZLIB::ZLIB | ||
) | ||
list(APPEND Open3D_3RDPARTY_PRIVATE_TARGETS_FROM_SYSTEM Open3D::3rdparty_zlib) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think You should set USE_SYSTEM_ZLIB
to OFF
if the package can't be found.
Good call, I'll make that update
…On Tue, Oct 29, 2024, 21:39 Meet Gandhi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In 3rdparty/find_dependencies.cmake
<#7022 (comment)>:
> @@ -853,6 +853,25 @@ if (BUILD_LIBREALSENSE)
endif()
endif()
+if(USE_SYSTEM_ZLIB)
+ open3d_find_package_3rdparty_library(3rdparty_zlib
+ PACKAGE ZLIB
+ TARGETS ZLIB::ZLIB
+ )
+ list(APPEND Open3D_3RDPARTY_PRIVATE_TARGETS_FROM_SYSTEM Open3D::3rdparty_zlib)
I think You should set USE_SYSTEM_ZLIB to OFF if the package can't be
found.
—
Reply to this email directly, view it on GitHub
<#7022 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABW54TMKOAU44GVRLK7LFHDZ6BPGPAVCNFSM6AAAAABQNDMACKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMBTGU4DQOJQHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dbs4261. Looks good in general. We can merge after:
- addressing @meetgandhi-dev 's comment
- CI passes. Please check the Ubuntu (OFF, OFF) failing workflow - libpng seems to be looking for some zlib functions.
https://github.com/isl-org/Open3D/actions/runs/11466322539/job/31906749596?pr=7022
Type
Motivation and Context
Open3D has used a patched version of ZLib that was patched to also build minizip. This causes problems when including Open3D in a larger project if some other dependency provides a version of ZLib that isn't patched before Open3D's provider is called. The current approach of WITH_MINIZIP is not descriptive enough, firstly because it can only be used with a system installed minizip package, and secondly the option is not connected to using a system version of ZLib. With this change, external dependency providers like the system package manager or VCPKG could provide one or both of these dependencies. If only a system zlib is provided, minizip will be built from source. This patch uses the NG versions of these libraries in comparability mode purely because the updated cmake build system is easier to integrate with. Existing builds using the WITH_MINIZIP option will not break as minizip will be provided by default, however the unused option will produce a warning from cmake.
Checklist:
python util/check_style.py --apply
to apply Open3D code styleto my code.
updated accordingly.
results (e.g. screenshots or numbers) here.
Description
This patch changes the ZLib provider to provide the ZLib-ng library in compatibility mode without any patch. Additionally it adds a Minizip-ng provider also in compatibility mode. Both of these providers are now optional, and can instead use
find_package(...)
if USE_SYSTEM_ZLIB and USE_SYSTEM_MINIZIP are turned on respectively.