build: Meta-issue for follow-ups to initial CMake merge (#1113) #1235

issue hebasto openend this issue on March 10, 2023
  1. hebasto commented at 11:47 am on March 10, 2023: member

    Issues that have been (re)discovered in #1113 but have not been addressed there:

    Affecting both build systems:

    Affecting only CMake:

  2. real-or-random added the label documentation on Mar 10, 2023
  3. real-or-random added the label build on Mar 10, 2023
  4. theuni commented at 6:38 pm on March 10, 2023: contributor

    Another one for me: my mingw32 build with ctests fails due to missing valgrind:

    0Valgrind .............................. OFF
1...
2ctime_tests ......................... ON
3...

    /home/cory/dev/secp256k1/src/ctime_tests.c:14:4: error: #error “This tool cannot be compiled without memory-checking interface (valgrind or msan)” 14 | # error “This tool cannot be compiled without memory-checking interface (valgrind or msan)”

    Should ctime_tests be a dependent option, depending on valgrind?

  5. hebasto commented at 6:57 pm on March 10, 2023: member

    @theuni

    Another one for me: my mingw32 build with ctests fails due to missing valgrind:

    0Valgrind .............................. OFF
1...
2ctime_tests ......................... ON
3...

    /home/cory/dev/secp256k1/src/ctime_tests.c:14:4: error: #error “This tool cannot be compiled without memory-checking interface (valgrind or msan)” 14 | # error “This tool cannot be compiled without memory-checking interface (valgrind or msan)”

    Do I understand correctly, that option -DSECP256K1_BUILD_CTIME_TESTS=ON is forced, right?

    Should ctime_tests be a dependent option, depending on valgrind?

    I don’t think so, as MSan is another option to use for building the ctime_tests.

  6. theuni commented at 7:13 pm on March 10, 2023: contributor

    Do I understand correctly, that option -DSECP256K1_BUILD_CTIME_TESTS=ON is forced, right?

    Yes, it was leftover on my cmdline from a previous build.

    But on my win32 build with no valid runtime (valgrind/msan/whatever) I believe cmake should throw an error telling me “SECP256K1_BUILD_CTIME_TESTS can’t be set without valgrind/msan”.

  7. sipa commented at 7:15 pm on March 10, 2023: contributor

    Currently the build system (neither autotools nor cmake, I think) has any knowledge of whether msan is enabled (in CI it’s done by passing explicit CFLAGS), so it has to conservatively assume that it may be enabled.

    Bringing msan (and maybe sanitizers in general?) into scope of the configure scripts may be useful, and if that’s the case, the detection could be made more precise too (without valgrind or msan, no ctime tests).

  8. hebasto commented at 7:16 pm on March 10, 2023: member

    But on my win32 build with no valid runtime (valgrind/msan/whatever) I believe cmake should throw an error telling me “SECP256K1_BUILD_CTIME_TESTS can’t be set without valgrind/msan”.

    Sounds like a nice feature for both build systems–CMake-based one and Autotools-based other.

  9. hebasto commented at 11:36 pm on March 12, 2023: member

    The current Autotools-based build system provides a user with an option to override compiler flags using the CFLAGS variable.

    That is not the case in the new CMake-based build system for now. The order of compiler flags is as follows:

    Additionally, CMAKE_C_FLAGS can be initialized using CMake’s builtin defaults (for example, /DWIN32 /D_WINDOWS for MSVC) or by the CFLAGS environment variable.

    If a user sets the latter, its value is consumed first without a chance to override flags set by the build system.

  10. hebasto commented at 4:28 pm on March 13, 2023: member

    GCC:

    0cc1: warning: command-line option ‘-fvisibility-inlines-hidden’ is valid for C++/ObjC++ but not for C

    So removing this task from the list.

  11. hebasto commented at 4:14 pm on March 14, 2023: member

    During reviewing of #1113, there was a suggestion:

    And expressing Coverage as build type in CMake seems to use build types in the right way.

    Later, #1188 introduced a new target noverify_tests and made the other target tests dependent on whether the build system is configured for coverage analysis or not.

    Unfortunately, CMake has no any straightforward way to skip targets in certain configuration. My draft attempt uses the EXCLUDE_FROM_DEFAULT_BUILD_COVERAGE target property and generator expressions in the EXCLUDE_FROM_ALL target property which, in turn, requires CMake 3.19+. Needless to say, such an implementation increases the complexity of the code.

    As @real-or-random rightly noted, the current code works only for single-config generators.

    Suggesting to re-introduce a trivial option(SECP256K1_COVERAGE OFF) which enforces the only available build configuration “Coverage”.

  12. hebasto commented at 8:08 pm on March 18, 2023: member

    In case it would be interested for other developers, here is my preset for cross-building from Linux (I use Ubuntu 22.04) to macOS:

     0{
 1  "version": 1,
 2  "configurePresets": [
 3    {
 4      "name": "macos-x86_64",
 5      "displayName": "Cross-compiling for macOS x86_64",
 6      "generator": "Unix Makefiles",
 7      "binaryDir": "${sourceDir}/build",
 8      "cacheVariables": {
 9        "CMAKE_SYSTEM_NAME": "Darwin",
10        "CMAKE_SYSTEM_PROCESSOR": "x86_64",
11        "CMAKE_C_COMPILER": "/usr/bin/clang-14;--target=x86_64-apple-darwin",
12        "CMAKE_OSX_SYSROOT": "/home/hebasto/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers"
13      },
14      "environment": {
15        "LDFLAGS": "-fuse-ld=/usr/bin/ld64.lld-14 -mmacosx-version-min=10.15 -mlinker-version=609"
16      },
17      "warnings": {
18        "dev": true,
19        "uninitialized": true
20      }
21    }
22  ]
23}

    The llvm and lld packages are required to be installed.

    Regarding Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers see https://github.com/bitcoin/bitcoin/blob/master/contrib/macdeploy/README.md.

  13. hebasto commented at 11:07 am on March 22, 2023: member

    … I figured out that we could set COMPILE_FLAGS instead, which supports generator expressions. https://cmake.org/cmake/help/latest/prop_sf/COMPILE_FLAGS.html But if you ask me, that belongs exactly to the category of “making things more CMake-ish”, which should not hold up this PR.

    Revisiting this issue as we now have the minimum required CMake 3.13.

    Different cases, when we set compiler flags, are as follows:

    1. Specific to a build configuration. We use dedicated CMAKE_C_FLAGS_<CONFIG> variables for them.
    2. Global flags to request or suppress warnings and errors. We use the COMPILE_OPTIONS top-directory property to handles them which is a well known CMake practice for such cases. Please note that #1240 suggests to use add_compile_options() command instead of direct manipulation of the COMPILE_OPTIONS property.
    3. For target-specific flags we use the target_compile_options() which is the best CMake practice.

    I cannot see what we can do about the original issue, so removing it from the task list. Let me know if I should revert it back.

  14. real-or-random commented at 9:39 am on March 26, 2023: contributor

    During reviewing of #1113, there was a suggestion:

    And expressing Coverage as build type in CMake seems to use build types in the right way.

    Later, #1188 introduced a new target noverify_tests and made the other target tests dependent on whether the build system is configured for coverage analysis or not.

    Unfortunately, CMake has no any straightforward way to skip targets in certain configuration. My draft attempt uses the EXCLUDE_FROM_DEFAULT_BUILD_COVERAGE target property and generator expressions in the EXCLUDE_FROM_ALL target property which, in turn, requires CMake 3.19+. Needless to say, such an implementation increases the complexity of the code.

    As @real-or-random rightly noted, the current code works only for single-config generators.

    Suggesting to re-introduce a trivial option(SECP256K1_COVERAGE OFF) which enforces the only available build configuration “Coverage”.

    What about using a CMake preset for Coverage builds, which just sets the right values? I think this could be a bit nicer than an option SECP256K1_COVERAGE but we’d need to try.

  15. hebasto commented at 2:02 pm on March 28, 2023: member

    What about using a CMake preset for Coverage builds, which just sets the right values? I think this could be a bit nicer than an option SECP256K1_COVERAGE but we’d need to try.

    Done in #1251.

  16. hebasto commented at 12:28 pm on April 13, 2023: member

    From the recent discussion in #1274:

    I think that’s a reason to keep the macOS Valgrind support in the build system (plus it works on x86_64).

    Removing this task from the list. Please let me know if you would like me to revert it back.

  17. real-or-random referenced this in commit 0a446a312f on Apr 20, 2023
  18. real-or-random referenced this in commit 4b84f4bf0f on Apr 27, 2023
  19. real-or-random referenced this in commit b54a0672ef on May 12, 2023
  20. dderjoel referenced this in commit 53216ab915 on May 23, 2023
  21. real-or-random referenced this in commit 908e02d596 on May 26, 2023
  22. real-or-random referenced this in commit d75dc59b58 on May 31, 2023
  23. hebasto commented at 6:59 pm on June 23, 2023: member
    • Consider setting CMake’s default of /MD for MSVC also in autotools builds.

    #1113 (comment):

    Nevertheless, as for reasonable defaults, my impression is that /MD has fewer issues and is what most users would expect, in particular from a library (see also jedisct1/libsodium#1215 ), and I guess there’s a reason why the CMake default is MultiThreadedDLL (where the DLL part corresponds to /MD) @sipsorcery What do you think about that?

  24. sipsorcery commented at 8:31 pm on June 23, 2023: member

    Without looking into which dll or exe it was applying to /MD would be a BIG change for the Windows build. The fact that the final bitcoind is a statically linked exe cascades down to most, if not all, of the dependencies.

    In my experience /MD (dynamically linked) is the default but then so are dynamically linked exe’s. I suspect someone decided to make bitcoind statically linked to avoid dll hell and/or a dependency introducing a vulnerability. Either way it is currently statically linked and again it would be a BIG change to make it not so.

    If bitcoind on Windows is going to stay statically linked then I think it would end up being a lot of fighting to change any of the artifacts to /MD.

    /MT and /MTd get my vote.

  25. real-or-random commented at 8:36 am on June 24, 2023: contributor

    @sipsorcery Thanks for commenting.

    Perhaps there’s a slight misunderstanding here. I think there are two separate issues:

    1. How should the bitcoind build libsecp256k1?
    2. What is the default for libsecp256k1?

    The latter is our question. libsecp256k1 has many other in the ecosystem users beside bitcoind. (In fact, that’s the reason why it is a separate library.) And we would like to set a reasonable default for them.

    In my experience /MD (dynamically linked) is the default but then so are dynamically linked exe’s.

    This makes me think that we should make /MD the default. Just because that’s most common on Windows.

    I suspect someone decided to make bitcoind statically linked to avoid dll hell and/or a dependency introducing a vulnerability. Either way it is currently statically linked and again it would be a BIG change to make it not so. If bitcoind on Windows is going to stay statically linked then I think it would end up being a lot of fighting to change any of the artifacts to /MD.

    Yes, I see that this will be a mess. But when building libsecp256k1 for bitcoind, we can override anything we want and simply set /MT or /MTd.

  26. hebasto commented at 12:09 pm on June 27, 2023: member
    • Consider setting CMake’s default of /MD for MSVC also in autotools builds. Perhaps make CMAKE_MSVC_RUNTIME_LIBRARY a CMake cache variable. ([build: Add CMake-based build system #1113 (comment)](/bitcoin-core-secp256k1/1113/#issuecomment-1458005771))

    On the current master branch, the user is able to specify the MSVC runtime library by providing the CMAKE_MSVC_RUNTIME_LIBRARY variable at the configuration stage. I’m not sure how the caching of it might improve the user experience.

    Going to skip this goal for now.

    cc @real-or-random

  27. hebasto commented at 1:25 pm on June 27, 2023: member
    • Consider setting CMake’s default of /MD for MSVC also in autotools builds… ([build: Add CMake-based build system #1113 (comment)](/bitcoin-core-secp256k1/1113/#issuecomment-1458005771))

    I think it’s OK to leave the defaults as they are for the following reason: After introducing the CMake-based build system, using MSVC with Autotools can be reasonably considered for testing purposes only. Additionally, it is beneficial to test Windows binaries with different MSVC runtime library options.

    Anyway, both routines, in CMake and in Autotools, allow the user to specify the MSVC runtime library explicitly.

    Removing this task from the list. Please let me know if you would like me to revert it back.

  28. hebasto commented at 1:58 pm on June 27, 2023: member
    • Consider CMAKE_COMPILE_WARNING_AS_ERROR for CMake 3.24+ ([build: Improve SECP_TRY_APPEND_DEFAULT_CFLAGS macro #1241 (comment)](/bitcoin-core-secp256k1/1241/#issuecomment-1509008580))

    CMake continuously introduces new abstractions for different aspects of the building process. Another example is DLL_NAME_WITH_SOVERSION: #1270 (review).

    I don’t think it would be beneficial to have two code paths depending on the actual CMake version. It would introduce additional complexity to the code and potentially lead to deviations in the user experience.

    We might review such new CMake’s features in the future when we are about to bump the minimum required CMale version.

    It seems unreasonable to have a tracking issue for such features.

    Removing this task from the list. Please let me know if you would like me to revert it back.

  29. real-or-random commented at 10:04 pm on June 27, 2023: contributor

    I think it’s OK to leave the defaults as they are for the following reason: After introducing the CMake-based build system, using MSVC with Autotools can be reasonably considered for testing purposes only.

    That’s a good point, I buy that argument.

  30. real-or-random commented at 10:11 pm on June 27, 2023: contributor

    I don’t think it would be beneficial to have two code paths depending on the actual CMake version. It would introduce additional complexity to the code and potentially lead to deviations in the user experience.

    I agree in general. Though there may be exceptions on a case-by-case basis. We have a few places where we check the value of CMAKE_VERSION, and I think they all make sense.

    We might review such new CMake’s features in the future when we are about to bump the minimum required CMale version.

    You mean when we bump, we should just read CMake’s changelog? That makes sense to me.

    Another possibility is that we add a comment to the affected code, e.g., TODO: Consider DLL_NAME_WITH_SOVERSION when bumping to CMake x.y. I would ACK such a comment, but yeah, just reading the changelog seems good enough, too.

  31. hebasto commented at 5:55 am on June 28, 2023: member

    I don’t think it would be beneficial to have two code paths depending on the actual CMake version. It would introduce additional complexity to the code and potentially lead to deviations in the user experience.

    I agree in general. Though there may be exceptions on a case-by-case basis. We have a few places where we check the value of CMAKE_VERSION, and I think they all make sense.

    Agree. That was my idea that I failed to express clearly and fully :)

    We might review such new CMake’s features in the future when we are about to bump the minimum required CMale version.

    You mean when we bump, we should just read CMake’s changelog?

    Yes.

    Another possibility is that we add a comment to the affected code, e.g., TODO: Consider DLL_NAME_WITH_SOVERSION when bumping to CMake x.y.

    Once we start adding such comments, we have to do some work at every CMake release.

    I would ACK such a comment, but yeah, just reading the changelog seems good enough, too.

    I’d prefer the latter :)

  32. real-or-random referenced this in commit e4ab12025e on Jun 28, 2023
  33. real-or-random referenced this in commit 69918b823f on Jun 28, 2023
  34. real-or-random referenced this in commit 9e6d1b0e9b on Jul 3, 2023
  35. real-or-random referenced this in commit e4af41c61b on Jan 17, 2024
  36. real-or-random referenced this in commit 3777e3f36a on Jan 17, 2024
  37. theStack referenced this in commit 2d2c6abb0e on Jan 24, 2024
  38. hebasto commented at 4:30 pm on March 22, 2024: member

    The recommended practice is to treat a project name and target names as independent concepts.

    From the book, section 4.6:

    Target names need not be related to the project name. While they are sometimes the same, the two things are separate concepts. Changing one shouldn’t imply that the other must also change. Project and target names should rarely change anyway, since doing so would break any downstream consumer that relied on the existing names. Therefore, set the project name directly rather than via a variable. Choose a target name according to what the target does rather than the project it is part of. Assume the project will eventually need to define more than one target. These practices reinforce better habits, which will be important when working on more complex, multi-target projects.

  39. real-or-random commented at 11:41 am on June 28, 2024: contributor

    The recommended practice is to treat a project name and target names as independent concepts.

    Okay, then let’s just drop this item.

    Then we’re only left with one item: unified docs. We can consider that one tracked in #1372.

  40. real-or-random closed this on Jun 28, 2024


hebasto theuni sipa real-or-random sipsorcery

Labels
user-documentation build

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 11:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me