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

CMake: Use cmake for xxhash, fix export #70

Merged
merged 2 commits into from
Dec 27, 2024

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Dec 17, 2024

xxhash provides exported CMake config when built with CMake.
This PR makes use of that config instead of reyling on pkg-config.
It also adds the corresponding find_dependency to this package's exported CMake config.

The PR also changes the xxhash library usage to PRIVATE - there is no xxhash API exposed by ls-qpack AFAICS.

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 17, 2024

From microsoft/vcpkg#42767.

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 17, 2024

I could add a fallback to pkg-config...

@talregev
Copy link
Contributor

talregev commented Dec 18, 2024

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 18, 2024

try to update vcpkg tag on the ci.

This is left to repo owners. This PR is not meant to cause build regressions.

@dg0yt dg0yt force-pushed the cmake-config branch 3 times, most recently from 7577d1a to e00e77e Compare December 18, 2024 07:53
find_package(xxHash CONFIG)
if(NOT xxHash_FOUND)
find_package(PkgConfig REQUIRED)
pkg_check_modules(LS_QPACK_XXH REQUIRED IMPORTED_TARGET libxxhash GLOBAL)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the LS_QPACK_ prefix here because of the side effects of GLOBAL when ls-qpack is used as a subproject.

if(LSQPACK_XXH)
target_link_libraries(test_${TARGET} ls-qpack)
else()
target_link_libraries(test_${TARGET} ls-qpack PkgConfig::XXH)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This usage requirement is handled by CMake when linking ls-qpack.

@talregev
Copy link
Contributor

@litespeedtech These changes and also the other changes in the next PRs are already merge in vcpkg in patch.

You can take a look here:
microsoft/vcpkg#42767

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 24, 2024

More interestingly, test usage of msh3 passes in microsoft/vcpkg#42788.

@litespeedtech litespeedtech merged commit fe77c94 into litespeedtech:master Dec 27, 2024
2 checks passed
@dg0yt dg0yt deleted the cmake-config branch December 27, 2024 21:37
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