cmake: Add dev-mode #1234

pull real-or-random wants to merge 2 commits into bitcoin-core:master from real-or-random:202303-cmake-dev-mode changing 2 files +21 −0
  1. real-or-random commented at 10:00 am on March 10, 2023: contributor

    To use, invoke cmake with argument --preset dev-mode.

    One disadvantage over ./configure --enable-dev-mode is that CMake does not provide a way to “hide” presets from users. That is, cmake --list-presets will list dev-mode, and it will also appear in cmake-gui, even though it’s not selectable there due to a bug in cmake-gui.

    Solves one item in #1224.

  2. in CMakePresets.json:19 in d12195836d outdated
    0@@ -0,0 +1,23 @@
    1+{
    2+  "version": 3,
    3+  "configurePresets": [
    4+    {
    5+      // Don't set "generator" here because developers should still be able to
    6+      // select their preferred generators. As a result we're affected by CMake
    7+      // bug https://gitlab.kitware.com/cmake/cmake/-/issues/23341, which means
    8+      // that dev-mode is not selectable in cmake-gui. But in our case, that's
    9+      // probably rather a feature than a bug.
    


    real-or-random commented at 10:12 am on March 10, 2023:
    GitHub highlights these lines because JSON doesn’t have comments. But some parsers are very forgiving, and CMake’s parser seems to be of that kind.
  3. hebasto commented at 10:41 am on March 10, 2023: member
    Concept ACK.
  4. hebasto cross-referenced this on Mar 10, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by hebasto
  5. real-or-random commented at 2:18 pm on March 11, 2023: contributor
    nb: This is not urgent, but anyway my availability in the next weeks will be restricted. If this requires changes, feel free to take over and make changes in a separate branch / PR.
  6. hebasto commented at 4:00 pm on March 11, 2023: member

    To use, invoke cmake with argument --preset dev-mode.

    Presets are available in CMake 3.19+ only. Should it be documented?

  7. real-or-random commented at 5:16 pm on March 11, 2023: contributor

    Presets are available in CMake 3.19+ only. Should it be documented?

    I tend to say it’s not necessary. It’s for devs only and if you invoke cmake --preset on a lower cmake version, I assume it will just error out with CMake Error: Unknown argument as for other unknown arguments.

  8. in CMakePresets.json:3 in d12195836d outdated
    0@@ -0,0 +1,23 @@
    1+{
    2+  "version": 3,
    


    hebasto commented at 7:56 pm on March 11, 2023:

    That makes this preset usable only with CMake 3.21+.

    And CMake 3.19 fails silently when cmake --preset dev-mode.


    real-or-random commented at 9:38 pm on March 11, 2023:
    Hm, I see. Do you have a suggestion? I don’t think we can do much about it, except hope that noone tries to use this with 3.19 or 3.20.

    hebasto commented at 10:15 pm on March 11, 2023:
    As it is a developer-specific feature, I’m OK with the current patch.
  9. hebasto approved
  10. hebasto commented at 7:56 pm on March 11, 2023: member
    ACK d12195836d91b6d00423c3c42fd43cdcec6543b7, tested on Ubuntu 22.04.
  11. hebasto commented at 3:29 pm on March 13, 2023: member
    Maybe add /CMakeUserPresets.json to the .gitignore?
  12. hebasto commented at 3:54 pm on March 13, 2023: member

    Well, cmake --preset dev-mode will place the build artifacts in the source tree, which is discouraged.

    There are two options to avoid such behavior:

    • cmake --preset dev-mode -B build or
    • add "binaryDir": "${sourceDir}/build" to the CMakePresets.json
  13. real-or-random commented at 2:01 pm on March 16, 2023: contributor

    Maybe add /CMakeUserPresets.json to the .gitignore?

    Done. I took the version without the leading slash because this file could in principle exist in src/ or examples/ too.


    Well, cmake –preset dev-mode will place the build artifacts in the source tree, which is discouraged.

    Okay, yes, this command was just intended as an example.

    There are two options to avoid such behavior:

    • cmake –preset dev-mode -B build or
    • add “binaryDir”: “${sourceDir}/build” to the CMakePresets.json

    My thinking is that, while ${sourceDir}/build is typically used, this should remain a local decision of the user/developer and not be the responsibility of the template. For example, if someone wants to maintain multiple for testing etc. … I guess the binaryDir is rather useful for automated builds, e.g, to facilitate CI configs.

  14. in .gitignore:63 in e8a4a94e4b outdated
    58@@ -59,5 +59,7 @@ build-aux/compile
    59 build-aux/test-driver
    60 libsecp256k1.pc
    61 
    62+### CMake
    63+CMakeUserPresets.json
    


    hebasto commented at 2:13 pm on March 16, 2023:

    Maybe add /CMakeUserPresets.json to the .gitignore?

    Done. I took the version without the leading slash because this file could in principle exist in src/ or examples/ too.

    https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html:

    CMakePresets.json and CMakeUserPresets.json live in the project’s root directory.


    real-or-random commented at 2:30 pm on March 16, 2023:
    Ok, you’re right, fixed.
  15. hebasto approved
  16. hebasto commented at 2:13 pm on March 16, 2023: member
    ACK e8a4a94e4b70cc9bc7c6942204a211bfdb492622
  17. hebasto commented at 2:15 pm on March 16, 2023: member

    My thinking is that, while ${sourceDir}/build is typically used, this should remain a local decision of the user/developer and not be the responsibility of the template.

    Command line options allow the user/developer to override a template.

  18. real-or-random force-pushed on Mar 16, 2023
  19. hebasto approved
  20. hebasto commented at 2:31 pm on March 16, 2023: member
    re-ACK 712090f417b440da62c73db0603eea58edce4cda
  21. real-or-random commented at 2:32 pm on March 16, 2023: contributor

    My thinking is that, while ${sourceDir}/build is typically used, this should remain a local decision of the user/developer and not be the responsibility of the template.

    Command line options allow the user/developer to override a template.

    Would you prefer this to be added? I’m fine with either, but I still tend to think that this should not be the concern of the template. Pragmatically speaking, it doesn’t make a big difference, of course.

  22. hebasto commented at 2:41 pm on March 16, 2023: member

    My thinking is that, while ${sourceDir}/build is typically used, this should remain a local decision of the user/developer and not be the responsibility of the template.

    Command line options allow the user/developer to override a template.

    Would you prefer this to be added? I’m fine with either, but I still tend to think that this should not be the concern of the template. Pragmatically speaking, it doesn’t make a big difference, of course.

    Let’s merge it in its current state :)

  23. hebasto cross-referenced this on Mar 28, 2023 from issue cmake: Use "coverage" preset instead of "Coverage" build type and document it by hebasto
  24. real-or-random commented at 11:08 am on April 18, 2023: contributor
    @theuni Would you like to review this? :)
  25. theuni commented at 3:32 pm on April 18, 2023: contributor

    Presets are available in CMake 3.19+ only. Should it be documented?

    I tend to say it’s not necessary. It’s for devs only and if you invoke cmake --preset on a lower cmake version, I assume it will just error out with CMake Error: Unknown argument as for other unknown arguments.

    I just tested with 3.16.3, and it just silently swallows the unknown option and continues. Since I would otherwise have assumed it worked, I agree it’s worth adding a quick note about required version.

  26. theuni approved
  27. theuni commented at 3:33 pm on April 18, 2023: contributor
    ACK modulo comment nit.
  28. real-or-random force-pushed on Apr 19, 2023
  29. real-or-random force-pushed on Apr 19, 2023
  30. real-or-random commented at 10:41 am on April 19, 2023: contributor
    Updated to add a comment on the required CMake version.
  31. cmake: Add dev-mode CMake preset
    To use, invoke `cmake` with argument `--preset dev-mode`.
    
    Solves one item in #1235.
    
    One disadvantage over `./configure --enable-dev-mode` is that CMake
    does not provide a way to "hide" presets from users. That is,
    `cmake --list-presets` will list dev-mode, and it will also appear
    in `cmake-gui`, even though it's not selectable there due to bug
    https://gitlab.kitware.com/cmake/cmake/-/issues/23341. (So in our
    case, that's probably rather a feature than a bug.)
    
    We curently use version 3 presets which require CMake 3.21+.
    Unfortunately, CMake versions before 3.19 may ignore the `--preset`
    argument silently. So if the preset is not picked up, make sure you
    have a recent enough CMake version.
    
    More unfortunately, we can't even spell this warning out in
    CMakePresets.json because CMake does not support officially support
    comments in JSON, see
     - https://gitlab.kitware.com/cmake/cmake/-/issues/21858
     - https://gitlab.kitware.com/cmake/cmake/-/merge_requests/5853 .
    We could use a hack hinted at in
    https://gitlab.kitware.com/cmake/cmake/-/issues/21858#note_908543
    but that's risky, because it could simply break for future versions,
    and we probably want to use presets not only for dev mode.
    0a446a312f
  32. gitignore: Add CMakeUserPresets.json
    This file is specifically intended for *local* CMake templates
    (as opposed to CMakePresets.json).
    ce5ba9e24d
  33. in CMakePresets.json:10 in 50ef1658c2 outdated
     5+  // Unfortunately, CMake versions before 3.19 may ignore the `--preset`
     6+  // argument silently. So if the preset is not picked up, make sure you
     7+  // have a recent enough CMake version.
     8+  //
     9+  // Note: JSON does not support officially, but CMake apparently
    10+  // ignores lines starting with whitespace followed by "//" silently.
    


    hebasto commented at 3:53 pm on April 20, 2023:

    real-or-random commented at 4:36 pm on April 20, 2023:

    Urghs, thanks for doing the research here. So they first enabled comment support intentionally, then disabled it intentionally. Currently, it just works by accident.

    I’m not sure how we should deal with this. Having the comments is risky because future versions may not support them. (This PR here uses presets only for dev mode, but we may add other presets, e.g., for coverage….)

    It seems that CMake’s parser defeats all the hacks I could imagine or find in https://stackoverflow.com/questions/244777/can-comments-be-used-in-json. Things we could do:

    • Have a template file and strip // lines using grep, generating the actual .json from it. But this needs some mechanism to make sure that the file is up to date etc… All doable but seems overkill for this tiny issue.
    • (https://hjson.github.io/ is an entire off-the-shelf solution for this, but that adds a maintainer dependency and is hard to remember.)
    • We could just drop the comments. CMake templates support a cmakeMinimumRequired field which I think is enough to document the need for CMake 3.21. A better warning should then be added to build docs (to be written :D). The comment about the missing generator thing can be in the commit message (not optimal but okayish).

    I lean towards dropping comments. Sad that we need to spend time on this…


    hebasto commented at 4:44 pm on April 20, 2023:

    I lean towards dropping comments.

    Agree.

  34. real-or-random force-pushed on Apr 20, 2023
  35. real-or-random commented at 5:25 pm on April 20, 2023: contributor
    Forced-push to remove the comments and move their contents to the commit message.
  36. in CMakePresets.json:3 in ce5ba9e24d
    0@@ -0,0 +1,19 @@
    1+{
    2+  "cmakeMinimumRequired": {"major": 3, "minor": 21, "patch": 0}, 
    3+  "version": 3,
    


    hebasto commented at 4:38 pm on April 21, 2023:
    A note for reviewers: version 3 is required as we omit the generator entry.
  37. hebasto approved
  38. hebasto commented at 4:38 pm on April 21, 2023: member
    ACK ce5ba9e24dfcceb49ed7f83a87548fd8b3b0cab2
  39. real-or-random requested review from theuni on Apr 25, 2023
  40. theuni approved
  41. theuni commented at 10:02 am on April 27, 2023: contributor
    ACK ce5ba9e24dfcceb49ed7f83a87548fd8b3b0cab2
  42. real-or-random merged this on Apr 27, 2023
  43. real-or-random closed this on Apr 27, 2023

  44. sipa referenced this in commit b4eb644b6c on May 12, 2023
  45. hebasto referenced this in commit 49c52ea2b1 on May 13, 2023
  46. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  47. sipa referenced this in commit 901336eee7 on Jun 21, 2023
  48. vmta referenced this in commit 8f03457eed on Jul 1, 2023

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-11-21 16:15 UTC

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