libmultiprocess
libmultiprocess copied to clipboard
cmake: Increase cmake policy version
Increase cmake policy version from 3.12 to 4.1 to stop using old and deprecated CMake policies in standalone builds.
Also stop overriding policy version if a cmake parent project has already set one, so parent projects are able to control which policies are enabled by default based on their own needs. Specifically, in the Bitcoin Core subtree, this change causes the libmultiprocess cmake policy version to increase from 3.12 to 3.22, which is the version Bitcoin Core uses.
This PR also adds a new newdeps CI job to test CMake 4.1 and the master branch of Cap'n Proto. This PR doesn't change the minimum version of cmake required to build the project, which is still 3.12.
This PR is an alternative to #163 which increases the policy version to 3.31 but doesn't include fixes for CI jobs, and doesn't allow the bitcoin core build to choose a lower policy version. This PR is also an alternative to #175 which sets the policy version to 3.22 like the bitcoin build, but also causes builds with earlier versions of cmake to fail.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviews
See the guideline for information on the review process. A summary of reviews will appear here.
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #175 (Set cmake_minimum_required(VERSION 3.22) by maflcko)
- #163 (build: set cmake policy version to 3.31 by purpleKarrot)
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.
Updated 6e29c02ec5afdb86874e83aa46c93b0d88c27968 -> 233b394123fcb96c5164dc1f4b1876b8d0ac6514 (pr/policy.1 -> pr/policy.2, compare) with a few fixes for CI errors: openbsd and freebsd git log errors and newdeps git clone TLS certificate error
Updated 233b394123fcb96c5164dc1f4b1876b8d0ac6514 -> 31206fc95a80c818c61b95e548f362dc9a10c7c8 (pr/policy.2 -> pr/policy.3, compare) with more CI fixes: openbsd and freebsd FindThreads errors and newdeps capnproto build openssl include error
Updated 31206fc95a80c818c61b95e548f362dc9a10c7c8 -> e6fd980b6c6960ea3bbda9194de1d1583bdef702 (pr/policy.3 -> pr/policy.4, compare) to fix more CI errors: openbsd and freebsd FindThreads errors and new llvm build FindThreads error.
Updated e6fd980b6c6960ea3bbda9194de1d1583bdef702 -> c33b81291896a4e1c82625f7c3bc423675899304 (pr/policy.4 -> pr/policy.5, compare) to fix CI errors: openbsd and freebsd "/bin/sh: CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND: not found" error, and default and llvm IWYU errors
Updated c33b81291896a4e1c82625f7c3bc423675899304 -> 1c2958bfffd60c8cf9ec9715699c1496f6377922 (pr/policy.5 -> pr/policy.6, compare) with better debug output and better commit message after figuring out root cause of the find_package threads failure
I am not happy with this. The title of the topic says "Increase cmake policy version", which I would expect to be a one line change. Instead, it has +95/-17 changes. The changes themselves are questionable:
- the length of the bash buildscript is doubled. Why isn't this written in a portable cmake script? Why is CMake preferred over handwritten Makefiles when the actual CI logic is written in bash?
- The ``cmake_minimum_required()` call has a 25 line comment to explain a rationale. If you have to explain what you are doing, you are doing it wrong.
- Putting
Threads::Threadsbehind a non-namespaced abstractionmpdepsmakes the build less robust.
Please don't proceed in this direction. Make sure the CMakeLists.txt files are declarative and follow best practices instead of inventing new approaches and deviations.
I'm struggling to create a MRE for the issue with the FindThreads module on FreeBSD. The following works just fine:
cmake_minimum_required(VERSION 3.12...4.1)
project(freebsd-threads CXX)
set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED YES)
find_package(Threads REQUIRED)
What am I missing?
re: https://github.com/bitcoin-core/libmultiprocess/pull/209#issuecomment-3292167145
I am not happy with this. The title of the topic says "Increase cmake policy version", which I would expect to be a one line change. Instead, it has +95/-17 changes.
Happy to change the title if you have a suggestion. This PR is only increasing the policy version and improving the CI, and I did not feel the CI changes were were mentioning since they are supporting changes, not the goal of the PR.
* the length of the bash buildscript is doubled. Why isn't this written in a portable cmake script? Why is CMake preferred over handwritten Makefiles when the actual CI logic is written in bash?
I think the language question should be raised new issue if you want to discuss it. (But just to provide a little context here if it can avoid an argument about the language of a ~70 line script, I just personally find bash a lot nicer and more useful than cmake and am much familiar with it. More generally, bash is commonly used in CI, there many more developers who know bash than cmake.)
The script does increase from 37 to 67 lines here to do two things:
- To support cloning and building unreleased versions of cap'n proto, so we can fix or report compatibility issues upstream cap'n proto earlier, and not be blindsided by new changes.
- To provide better output when find_package fails, because by default the information cmake provides is not useful for debugging.
IMO both changes are worth making and do not have any real downsides.
If you have to explain what you are doing, you are doing it wrong.
Interesting point. It might be helpful if you could explain practical downsides of the change. Unless you also believe that if you have to explain why a change is wrong, then it must be right?
Putting Threads::Threads behind a non-namespaced abstraction mpdeps makes the build less robust.
Can you explain this a little more as well? Is there a practical downside here or a different approach you would recommend?
re: https://github.com/bitcoin-core/libmultiprocess/pull/209#issuecomment-3292177333
I'm struggling to create a MRE for the issue with the
FindThreadsmodule on FreeBSD. The following works just fine:
The error happens when you don't have clang-scan-deps installed on the build machine, as seems to be the case currently with our freebsd and openbs vms. This issue is not specifically related to the find_package(Threads REQUIRED) call but the find_package(Threads REQUIRED) call makes it harder to understand what is wrong because it obscures the real error. Making the threads package optional produces a clearer error later, and I think it's just more correct anyway since modern platforms don't use separate threading libraries.
If it helps there is a failing CI run with more complete output:
https://github.com/ryanofsky/libmultiprocess/actions/runs/17622518177/job/50071319558
And the relevant cmake policy is https://cmake.org/cmake/help/latest/policy/CMP0155.html
Make sure the
CMakeLists.txtfiles are declarative and follow best practices instead of inventing new approaches and deviations.
To be sure, I agree with you it would be better if Bitcoin Core followed cmake's recommended best practices and used new policies instead of setting an really old policy version.
But bitcoin core isn't doing that, and other bitcoin core developers have given reasons why it shouldn't do that, and while I don't agree with these reasons, I think it is a reasonable judgement call, and they are just choosing a different tradeoffs than I would. This PR makes libmultiprocess compatible with either approach for choosing a policy version and stops using really old policies (older than the ones bitcoin core chooses) in any case.
The error happens when you don't have
clang-scan-depsinstalled on the build machine, as seems to be the case currently with our freebsd and openbs vms.
Here is a build log from GHA using FreeBSD:
-- The CXX compiler identification is Clang 19.1.7
-- 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 - found
-- Found Threads: TRUE
-- Configuring done (1.2s)
-- Generating done (0.0s)
-- Build files have been written to: /home/runner/work/github-actions/github-actions/build
Here is a build log from GHA using FreeBSD:
Is that link right? It seems to be a 404 for me. I also don't think I understand what it indicates if find_package(Threads REQUIRED) works in another build. We already know it works in many builds. You can see some builds where it is failing in https://github.com/bitcoin-core/libmultiprocess/pull/209#issuecomment-3271994909 and https://github.com/ryanofsky/libmultiprocess/actions/runs/17622518177/job/50071319558. Making the threads library optional improves error output, works well and doesn't have other downsides, and makes sense conceptually on modern platforms that do not use separate libraries for threading.
Here is a build log from GHA using FreeBSD:
Is that link right? It seems to be a 404 for me.
Sorry. I've made that repo public. The link should work now.
I also don't think I understand what it indicates if find_package(Threads REQUIRED) works in another build. We already know it works in many builds. You can see some builds where it is failing in #209 (comment) and https://github.com/ryanofsky/libmultiprocess/actions/runs/17622518177/job/50071319558. Making the threads library optional improves error output, works well and doesn't have other downsides, and makes sense conceptually on modern platforms that do not use separate libraries for threading.
I want to report the issue upstream and I need an MRE for it.
I want to report the issue upstream and I need an MRE for it.
Oh, thanks! It seems like your branch is https://github.com/hebasto/github-actions/compare/250915-freebsd-cmake-threads and I agree I don't see an obvious reason why that CI succeeds while my CI runs such as https://github.com/ryanofsky/libmultiprocess/actions/runs/17622518177/job/50071319558 with branch https://github.com/ryanofsky/libmultiprocess/commits/citest/policy at 3b8a0e6e4590ae50cb681de6034fdf547cdb8cf3 failed. Some things I might suggest:
- Starting from my failing commit and deleting unnecessary steps to find a minimal failing case
- Printing yaml build log unconditionally instead of only on failure in your current branch
- Callling clang-scan-deps --version or seeing if it is available
I want to report the issue upstream and I need an MRE for it.
Also, I don't think there is necessarily there is necessarily an upstream bug here, since we are asking cmake to look for a package and it fails and reports the failure. It just doesn't seem to do a good job reporting the cause of the failure.
@ryanofsky
Some things I might suggest:
- Starting from my failing commit and deleting unnecessary steps to find a minimal failing case
Thank you for the suggestion.
The culprit turned out to be the "Ninja" generator, which was quite surprising to me.
The culprit turned out to be the "Ninja" generator, which was quite surprising to me.
Nice! And that is really surprising. I would expect the generator not to matter much during configuration.