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

find_package(Threads) regardless of BUILD_ONLY_LIB #1653

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

zxjtan
Copy link
Contributor

@zxjtan zxjtan commented Jan 5, 2024

Retroactive follow-up to #1576: I realised that #1549 introduces usage of pthreads into the main library code. I also infer that the reason find_package(Threads) was originally gated behind OQS BUILD_ONLY_LIB was because only test code used pthreads. Hence, CMake should now always look for the Threads package (on non-MSVC compilers).

Just doing the above breaks Zephyr builds. Hence, this PR also adds a new CMake variable OQS_USE_PTHREADS that will be true if (CMAKE_USE_PTHREADS_INIT AND NOT OQS_EMBEDDED_BUILD). It also changes code gated on #CMAKE_USE_PTHREADS_INIT macro to gate on #OQS_USE_PTHREADS instead.

@zxjtan zxjtan force-pushed the main branch 4 times, most recently from 3e19c2d to 6a0aa38 Compare January 18, 2024 22:29
@zxjtan zxjtan marked this pull request as ready for review January 18, 2024 23:07
@zxjtan zxjtan requested a review from dstebila as a code owner January 18, 2024 23:07
@SWilson4
Copy link
Member

@zxjtan Sorry it's taken so long for us to provide feedback on this PR! It looks reasonable to me (pending a rebase on the latest main), but I don't have enough knowledge about pthreads (and threading in general) to feel comfortable approving it. @baentsch @dstebila you reviewed #1549 and #1576; would you also be able to chime in here?

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in reviewing this. I now understand the logic and tested successfully against oqs-provider. Thanks for the improvement, @zxjtan !

@baentsch baentsch merged commit 688bdb4 into open-quantum-safe:main Jan 31, 2024
34 checks passed
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.

4 participants