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

fixes for using system packages #7177

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

Conversation

christian-rauch
Copy link

@christian-rauch christian-rauch commented Feb 22, 2025

I was building Open3D with most system packages enabled and some features deactivated. This uncovered a couple of compilation and linking issues, such as:

  1. usingExtractZIP when WITH_MINIZIP is off
  2. using GLFW APIs that are only available from version 3.4 onwards
  3. linking OpenSSL when it has not been found before when USE_SYSTEM_CURL and USE_SYSTEM_OPENSSL are set

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

Fixing the build

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

Copy link

update-docs bot commented Feb 22, 2025

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@ssheorey ssheorey self-requested a review March 8, 2025 01:00
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 @christian-rauch for these fixes! Please see one comment below.


if(NOT USE_SYSTEM_CURL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @christian-rauch we need SSL as an indirect dependency of cURL. So with USE_SYSTEM_CURL, we do not expliclty need to find and link system SSL. Can you update this so that SSL is ignored with USE_SYSTEM_CURL?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't this exactly what the commit is fixing? Previously, with USE_SYSTEM_CURL set to ON, we would not link SSL. Hence, the linking issues. So with USE_SYSTEM_CURL=ON, we still need to link SSL explicitly.

@christian-rauch
Copy link
Author

Btw, python util/check_style.py --apply requires an old clang-format version. Also with the oldest possible setting required_clang_format_major = 14 on Ubuntu 24.04, this failed in the regex. Ubuntu returns something like Ubuntu clang-format version 18.1.3 (1ubuntu1), i.e. it prepends Ubuntu to the string.

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

Successfully merging this pull request may close these issues.

3 participants