Skip to content

Conversation

kou
Copy link
Contributor

@kou kou commented Apr 25, 2025

a6e835d added find_dependency(Xxhash REQUIRED). It uses our build/fbcode_builder/CMake/FindXxhash.cmake but our FindXxhash.cmake isn't installed. So find_dependency(Xxhash REQUIRED) is failed when xxHash/xxHashConfig.cmake isn't installed by xxHash.

For example, xxHash package in CentOS 9 stream doesn't install xxHash/xxHashConfig.cmake.

See also: facebookincubator/velox#13153

We can fix this by install our FindXxhash.cmake and use it in FBThriftConfig.cmake.

facebook@a6e835d
added `find_dependency(Xxhash REQUIRED)`. It uses our
`build/fbcode_builder/CMake/FindXxhash.cmake` but our
`FindXxhash.cmake` isn't installed. So `find_dependency(Xxhash
REQUIRED)` is failed when `xxHash/xxHashConfig.cmake` isn't installed
by xxHash.

For example, xxHash package in CentOS 9 stream doesn't install
`xxHash/xxHashConfig.cmake`.

See also: facebookincubator/velox#13153

We can fix this by install our `FindXxhash.cmake` and use it in
`FBThriftConfig.cmake`.
DESTINATION ${CMAKE_INSTALL_DIR}
)
# This is used in FBThriftConfig.cmake.
install(

Choose a reason for hiding this comment

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

@kou will this work outside of fbcode_builder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

But the path hardcodes fb specific infra path ../build/fbcode_builder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You refer ../build/fbcode_builder in https://github.com/facebook/fbthrift/pull/651/files#diff-72e4eecafeb87f57668ee019db6f11aa91d85647850c63e4bfb4d0ae2c38341eR43 , right?

We can use ../build/fbcode_builder in install(FILES) here because it's a static path that is only used in this project. It doesn't depend on installed system and projects that use this such as Velox. Because users including Velox use only DESTINATION path (${CMAKE_INSTALL_DIR}) via find_package(FBThrift).

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

Successfully merging this pull request may close these issues.

3 participants