cmake: Do not modify build types when integrating by downstream project #1543

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:240618-cmake-conf changing 1 files +17 −15

hebasto commented at 2:52 pm on June 18, 2024: member
The CMAKE_BUILD_TYPE and CMAKE_CONFIGURATION_TYPES must be managed by the downstream project.

Suggesting to review with git diff -w.

Fixes std::out_of_range exception from CMake in https://github.com/hebasto/bitcoin/pull/192 when running configuration step using “Ninja Multi-Config” generator:
0$ cmake -B build -G "Ninja Multi-Config" 1... 2-- Configuring done (17.1s) 3terminate called after throwing an instance of 'std::out_of_range' 4 what(): map::at 5Aborted (core dumped)
Here are related discussions:
- https://discourse.cmake.org/t/uncaught-exception-when-trying-to-generate-a-project-using-ninja-multi-config/11051
- https://gitlab.kitware.com/cmake/cmake/-/issues/26064

cmake: Do not modify build types when integrating by downstream project

The `CMAKE_BUILD_TYPE` and `CMAKE_CONFIGURATION_TYPES` variables must be
managed by the downstream project.

Suggesting to review with `git diff -w`.

158f9e5eae

fanquake commented at 3:36 pm on June 18, 2024: member

Fixes std::out_of_range exception in https://github.com/hebasto/bitcoin/pull/192 when using “Ninja Multi-Config” generator.

Can you at least elaborate some amount? How is changing something CMake related in this C project, the solution to some C++ issue in a different project? Theres no details or CI failure etc in the linked PR, so it’s not even clear what’s broken there.
real-or-random commented at 8:12 pm on June 18, 2024: contributor

Yeah, I tend to agree that we shouldn’t touch these variables. But it’s a bit unclear how this fixes a std::out_of_range exception.
real-or-random added the label bug on Jun 18, 2024
real-or-random added the label build on Jun 18, 2024
real-or-random commented at 8:28 pm on June 18, 2024: contributor

Yeah, I tend to agree that we shouldn’t touch these variables.

Hm, on a second thought, I’m not sure. I believe we want to build libsecp256k1 with the default RelWithDebInfo (-O2 -g in our case) even when we build it as part of Core. Because (almost) all our testing uses this config.

edit: What does the current autotools build do?

By the way, this is one of these issues that makes me wonder if the GNU build system was actually that bad. The amount of “custom” logic that is needed to convince CMake to do the right thing is still large.
fanquake commented at 10:54 am on June 20, 2024: member

How is changing something CMake related in this C project, the solution to some C++ issue in a different project?

After offline discussion, it’s been clarified that the std::out_of_range exception is actually coming from CMake itself, with it being uncertain as to whether the CMake developers consider it a bug or just “misuse”. @hebasto if you have a link to upstream discussion, it’d be good to have it here as well.
hebasto commented at 10:57 am on June 20, 2024: member

Fixes std::out_of_range exception in hebasto/bitcoin#192 when using “Ninja Multi-Config” generator.

Can you at least elaborate some amount? How is changing something CMake related in this C project, the solution to some C++ issue in a different project? Theres no details or CI failure etc in the linked PR, so it’s not even clear what’s broken there.

I’ve updated the PR description trying to make it more descriptive and providing more details.

@hebasto if you have a link to upstream discussion, it’d be good to have it here as well.

Done.
real-or-random commented at 11:14 am on June 20, 2024: contributor

I’ve updated the PR description trying to make it more descriptive and providing more details.

@hebasto if you have a link to upstream discussion, it’d be good to have it here as well.

Done.

There’s also an GitLab issue now: https://gitlab.kitware.com/cmake/cmake/-/issues/26064. I took the freedom to add the link to the PR description.
hebasto commented at 5:54 pm on June 24, 2024: member

The CMake’s bug has been fixed in https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9618.

I tested CMake version 3.30.0-rc4 and no exceptions happen.

Nevertheless, this PR’s changes are still meaningful and required.
real-or-random approved
real-or-random commented at 6:07 pm on June 24, 2024: contributor

ACK 158f9e5eae583b1520af9ee727db342e7408dab1
fanquake commented at 8:57 am on June 25, 2024: member

Tested that this change (combined with https://github.com/hebasto/bitcoin/pull/192) no-longers results in CMake segfaulting when configuring. I assume there’s nothing else we need to do, even if CMake also changes its docs here: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9618#note_1538910 ?
real-or-random commented at 9:28 am on June 25, 2024: contributor

The CMAKE_BUILD_TYPE and CMAKE_CONFIGURATION_TYPES must be managed by the downstream project.

All the CMake issues mention only CMAKE_CONFIGURATION_TYPES? Have you a reference for the claim that also CMAKE_BUILD_TYPE should be managed by the top-level project? (Well, it’s probably also just a good idea for consistency?)
hebasto commented at 10:19 am on June 25, 2024: member

The CMAKE_BUILD_TYPE and CMAKE_CONFIGURATION_TYPES must be managed by the downstream project.

All the CMake issues mention only CMAKE_CONFIGURATION_TYPES? Have you a reference for the claim that also CMAKE_BUILD_TYPE should be managed by the top-level project? (Well, it’s probably also just a good idea for consistency?)

The CMAKE_BUILD_TYPE being a cache variable is dedicated to be modifiable externally and it should be managed by the top-level project for consistency.

The same technique is suggested in the book Professional CMake: A Practical Guide, section 15.3. Custom Build Types.
real-or-random merged this on Jun 25, 2024
real-or-random closed this on Jun 25, 2024
hebasto deleted the branch on Jun 25, 2024

Contributors
hebasto fanquake real-or-random

Labels
bug build