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

[package] xxhash/0.8.2: missing options #25778

Open
vasama opened this issue Oct 31, 2024 · 3 comments · May be fixed by #25795
Open

[package] xxhash/0.8.2: missing options #25778

vasama opened this issue Oct 31, 2024 · 3 comments · May be fixed by #25795
Labels
bug Something isn't working

Comments

@vasama
Copy link
Contributor

vasama commented Oct 31, 2024

Description

The recipe is missing options for XXH_STATIC_LINKING_ONLY and XXH_IMPLEMENTATION (header only).

Package and Environment Details

  • Package Name/Version: xxhash/0.8.2

Conan profile

N/A

Steps to reproduce

N/A

Logs

N/A

@vasama vasama added the bug Something isn't working label Oct 31, 2024
@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 1, 2024

Could you clarify what you want exactly? Would you want xxhash recipe to expose a header_only option so that it would just package header of xxhash without any compilation? I don't see any reason to do that in a package manager when we have a proper build alternative.

@vasama
Copy link
Contributor Author

vasama commented Nov 1, 2024

I don't see any reason to do that in a package manager

Yeah, usually I would agree. In this case there's a clear performance benefit to enabling inlining of those functions, however.

XXH_STATIC_LINKING_ONLY on the other hand is required for static allocation of hash states.

@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 1, 2024

XXH_STATIC_LINKING_ONLY is defined by default through compilation of xxhah.c https://github.com/Cyan4973/xxHash/blob/v0.8.2/xxhash.c#L40

Well feel free to open a PR adding header_only option. No need to add other options since it doesn't make sense when we are building xxhash (XXH_STATIC_LINKING_ONLY always defined by design), and if it's header_only, users are on their own and free to define XXH_STATIC_LINKING_ONLY or not themselves (it's how all "C header only" libraries work in conancenter recipes, users are responsible of defining IMPLEMENTATION macro stuff themselves, and eventually the STATIC macro stuff when it exists).

@vasama vasama linked a pull request Nov 2, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants