Skip to content

Fix CMake modules for finding external libraries #2240

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lahwaacz
Copy link

What changes were proposed in this pull request?

  • allow finding header files in default paths - this is consistent with the logic for finding the library files
  • replace undefined variables ${SNAPPY_LIB_NAME} and ${LZ4_LIB_NAME} with the actual names

Why are the changes needed?

The CMake modules are broken. Building anything that depends on orc using cmake (e.g. Apache Arrow) does not work.

How was this patch tested?

Better than the original files 😏

Was this patch authored or co-authored using generative AI tooling?

No

- allow finding header files in default paths - this is consistent with
  the logic for finding the library files
- replace undefined variables ${SNAPPY_LIB_NAME} and ${LZ4_LIB_NAME}
  with the actual names
@github-actions github-actions bot added the BUILD label May 24, 2025
@dongjoon-hyun
Copy link
Member

cc @wgtmac

@wgtmac
Copy link
Member

wgtmac commented May 26, 2025

Thanks for creating this PR!

The CMake modules are broken. Building anything that depends on orc using cmake (e.g. Apache Arrow) does not work.

Could you elaborate this? I was thinking to remove these obsolete FindXXX.cmake files to use config mode of find_package so we don't have to maintain them.

@lahwaacz
Copy link
Author

Could you elaborate this?

Try to build the latest orc version and then arrow using the following cmake options (notice especially -DARROW_DEPENDENCY_SOURCE=SYSTEM and -DARROW_ORC=ON)

https://gitlab.archlinux.org/archlinux/packaging/packages/arrow/-/blob/main/PKGBUILD?ref_type=heads#L80-120

I was thinking to remove these obsolete FindXXX.cmake files to use config mode of find_package so we don't have to maintain them.

The "config mode" is still looking for some .cmake file provided by the library you are finding. Many packages do that, including protobuf, snappy, and zstd, but some don't (e.g. lz4, zlib). But cmake also has its standard FindZLIB module so you don't have to reimplement your own.

@wgtmac
Copy link
Member

wgtmac commented May 26, 2025

I think lz4 and zlib have exported their packages to be used by config mode:

@lahwaacz
Copy link
Author

lahwaacz commented May 26, 2025

But if you install lz4 using a different build system (e.g. Arch Linux uses meson), the CMake integration files are not installed.

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

Successfully merging this pull request may close these issues.

3 participants