cmake: make Threads package optional - #354
Conversation
Make find_package(Threads) optional because there are platforms where this package may not be required, and because find_package(Threads REQUIRED) errors obscure more detailed error messages and makes issues harder to debug. For example with CMP0155 enabled which turns CMAKE_CXX_SCAN_FOR_MODULES on, find_package(Threads) fails on freebsd and openbsd CI jobs which lack a clang-scan-deps tool. Also when CMP0137 is disabled or CMAKE_TRY_COMPILE_NO_PLATFORM_VARIABLES is set to true find_package(Threadss) fails in the llvm CI job. This change lets builds in both of those cases succeed. The freebsd CMP0155 error looks like: + cmake /home/runner/work/libmultiprocess/libmultiprocess -G Ninja -- The CXX compiler identification is Clang 16.0.6 -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Check for working CXX compiler: /usr/bin/c++ - skipped -- Detecting CXX compile features -- Detecting CXX compile features - done -- Performing Test CMAKE_HAVE_LIBC_PTHREAD -- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed -- Looking for pthread_create in pthreads -- Looking for pthread_create in pthreads - not found -- Looking for pthread_create in pthread -- Looking for pthread_create in pthread - not found -- Check if compiler accepts -pthread -- Check if compiler accepts -pthread - no CMake Error at /usr/local/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:233 (message): Could NOT find Threads (missing: Threads_FOUND) Call Stack (most recent call first): /usr/local/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE) /usr/local/share/cmake/Modules/FindThreads.cmake:226 (FIND_PACKAGE_HANDLE_STANDARD_ARGS) CMakeLists.txt:41 (find_package) Inside the CMakeConfigureLog.yaml file there are "/bin/sh: CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND: not found" errors.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
~0: Same here. This won't happen in reality on current master, so better to leave in #209 |
| find_package(Threads) | ||
| if(Threads_FOUND) | ||
| target_link_libraries(mpdeps INTERFACE Threads::Threads) | ||
| endif() |
There was a problem hiding this comment.
Make
find_package(Threads)optional because there are platforms where this package may not be required...
But the code does not look platform-dependent.
According to the docs, FindThreads is capable of figuring out:
... if the thread functions are provided by the system libraries and no special flags are needed to use them.
Therefore, I don't see this change is necessary.
There was a problem hiding this comment.
re: #354 (comment)
According to the docs,
FindThreadsis capable of figuring out
The docs are wrong. The REQUIRED keyword here hides useful error messages and replaces them with misleading "Could NOT find Threads" messages. This has happened to me on multiple occasions as described in the commit message and misled me and wasted my time.
If you think there is an advantage REQUIRED provides here, it would be good to know what it is. The only thing I see REQUIRED doing here is replacing real errors with fake and misleading ones. It seems clear to me find_package(Threads) is a helpful way to add thread dependencies and find_package(Threads REQUIRED) is a footgun.
re: #354 (comment)
This won't happen in reality on current master
That's a bold prediction! It does not happen on current master, but it happened to me twice on different occasions changing the policy version and can easily happen with changes to platforms, toolchain files or any of the spaghetti code module files distributed with cmake.
There was a problem hiding this comment.
The docs are wrong.
Is there something we can report upstream? Can you share the situation and output of the suppressed diagnostics?
There was a problem hiding this comment.
Thanks for the replies! I will close this PR because as obnoxious as find_package(Threads REQUIRED) behavior is for producing misleading error and hiding useful debug information, the REQUIRED keyword actually does seem to be used in bitcoin core and leveldb, so maybe it is not as much as a problem for others as it has been for me. And there could be other ways of avoiding harms like #352, or maybe moving the find_package call later so more useful errors can be triggered earlier. When I first made this change a year ago as part of #209, I didn't know techniques for debugging find_package errors and found this very confusing, but now I won't have this problem anymore.
Sorry for pushing back earlier. I pushed back because review comments were arguing against removing REQUIRED without acknowledging the harms it caused or pointing to any benefits it offered. But thinking about it more, one real benefit that REQUIRED offers is more predictability and less variance between builds. For example with 45c7dca, if libmultiprocess is built on two openbsd systems, and one has clang-scan-deps installed and one doesn't, find_package may succeed on one system and fail on the other causing the library to be built with slightly different threading flags even if both builds succeed and are usable.
I still think 45c7dca is a correct change, just that benefits may not outweigh this cost. It is correct because the cmake FindThreads module is not a hard dependency of libmultiprocess (unlike Cap'n Proto). FindThreads is just a convenience module provided with cmake to help with portability and it is perfectly possible to write multithreaded code without it, avoiding its warts and bugs.
re: purpleKarrot #354 (comment)
The docs are wrong.
Is there something we can report upstream? Can you share the situation and output of the suppressed diagnostics?
I encountered these errors a year ago but you can see the error output in 45c7dca commit message and the CMAKE_CXX_SCAN_FOR_MODULES bug is reported upstream https://gitlab.kitware.com/cmake/cmake/-/work_items/27228. I described the bad CMAKE_TRY_COMPILE_NO_PLATFORM_VARIABLES/ FindThreads interaction in #163 (comment) with steps to reproduce and you actually replied to that and seemed to think it was the same underlying bug.
re: maflcko #354 (comment)
Ok, what I wanted to say was that this doesn't practically happen in reality. Someone changing the policy isn't something a real end-user does, but more a dev-only thing.
I don't think this is true because these errors depend on individual platform configurations. The same policies can work on one platform and cause problems on other platforms and this is exactly what we seen with FindThreads where the bug isn't even really triggered by the platform, but just by what packages are currently installed. Better error output is better for everyone and is not just a temporary development need.
re: hebasto #354 (review)
In the following example, the build fails during the linking stage because it doesn't configure the thread library properly:
That example seems to be artificial (setup is not described) unlike the real examples in the commit message, and even in that case cannot find -lpthread seems like a much better error than Could NOT find Threads (missing: Threads_FOUND) would be because it points to a clear cause.
Ok, what I wanted to say was that this doesn't practically happen in reality. Someone changing the policy isn't something a real end-user does, but more a dev-only thing. The dev should know what they are doing, and should know what they are signing up for, so I don't think it makes sense to accommodate for that outside the pull that changes the policy version. If this can easily happen in reality on a platform, it would be good to know the platform. |
There was a problem hiding this comment.
I'm still not convinced that trading code correctness for easier debugging is an improvement.
In the following example, the build fails during the linking stage because it doesn't configure the thread library properly:
$ cmake -B build -DCMAKE_CXX_COMPILER=g++-10
-- The CXX compiler identification is GNU 10.5.0
-- Check for working CXX compiler: /usr/bin/g++-10
-- Check for working CXX compiler: /usr/bin/g++-10 -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Looking for C++ include pthread.h
-- Looking for C++ include pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed
-- Looking for pthread_create in pthreads
-- Looking for pthread_create in pthreads - not found
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - not found
-- Check if compiler accepts -pthread
-- Check if compiler accepts -pthread - no
-- Could NOT find Threads (missing: Threads_FOUND)
-- Performing Test HAVE_PTHREAD_GETNAME_NP
-- Performing Test HAVE_PTHREAD_GETNAME_NP - Failed
-- Performing Test HAVE_PTHREAD_SETNAME_NP_3ARG
-- Performing Test HAVE_PTHREAD_SETNAME_NP_3ARG - Failed
-- Performing Test HAVE_PTHREAD_THREADID_NP
-- Performing Test HAVE_PTHREAD_THREADID_NP - Failed
-- Performing Test HAVE_PTHREAD_GETTHREADID_NP
-- Performing Test HAVE_PTHREAD_GETTHREADID_NP - Failed
-- Configuring done
-- Generating done
-- Build files have been written to: /libmultiprocess/build
$ cmake --build build
Scanning dependencies of target mputil
[ 14%] Building CXX object CMakeFiles/mputil.dir/src/mp/util.cpp.o
[ 14%] Built target mputil
Scanning dependencies of target mpgen
[ 28%] Building CXX object CMakeFiles/mpgen.dir/src/mp/gen.cpp.o
[ 42%] Linking CXX executable mpgen
/usr/bin/ld: cannot find -lpthread
collect2: error: ld returned 1 exit status
make[2]: *** [CMakeFiles/mpgen.dir/build.make:92: mpgen] Error 1
make[1]: *** [CMakeFiles/Makefile2:216: CMakeFiles/mpgen.dir/all] Error 2
make: *** [Makefile:141: all] Error 2
|
Thanks for the reviews! Closing as described #354 (comment) |
5 participants
Make
find_package(Threads)optional because there are platforms where this package may not be required, and becausefind_package(Threads REQUIRED)errors obscure more detailed error messages and make issues harder to debug.There errors can happen on different platforms with different cmake policy settings. See commit message for details.