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
  1. 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:

  2. 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
  3. 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.

  4. 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.
  5. real-or-random added the label bug on Jun 18, 2024
  6. real-or-random added the label build on Jun 18, 2024
  7. 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.

  8. 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.

  9. 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.

  10. 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.

  11. 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.

  12. real-or-random approved
  13. real-or-random commented at 6:07 pm on June 24, 2024: contributor
    ACK 158f9e5eae583b1520af9ee727db342e7408dab1
  14. 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 ?
  15. 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?)

  16. 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.

  17. real-or-random merged this on Jun 25, 2024
  18. real-or-random closed this on Jun 25, 2024

  19. hebasto deleted the branch on Jun 25, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-09 07:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me