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

Update ZLib 3rdparty library to use ZLib-ng and Minizip-ng so patching is not necessary #7022

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

dbs4261
Copy link
Contributor

@dbs4261 dbs4261 commented Oct 22, 2024

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes #
  • New feature (non-breaking change which adds functionality). Resolves #
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) Resolves #

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:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

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.

Copy link

update-docs bot commented Oct 22, 2024

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)

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.

@dbs4261
Copy link
Contributor Author

dbs4261 commented Oct 30, 2024 via email

@ssheorey ssheorey self-requested a review November 6, 2024 15:40
Copy link
Member

@ssheorey ssheorey left a 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:

@ssheorey ssheorey added the status / needs info Waiting for information from reporter / author label Dec 16, 2024
@ssheorey ssheorey marked this pull request as draft December 18, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status / needs info Waiting for information from reporter / author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants