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

[hebasto]

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

Affecting both build systems:

• Unified docs for configuration autotools/CMake/manual builds (build: Add CMake-based build system #1113 (comment) and Add section on configuration flags to README #1142); WIP -- doc: Add unified docs for configuration autotools/CMake/manual builds #1372
• Improve MSVC warning options (build: Add CMake-based build system #1113 (comment)); solved by build: Bump MSVC warning level up to W3 #1328
• Consider changing DLL_EXPORT TO SECP256k1_DLL_EXPORT (build: Add CMake-based build system #1113 (comment)); solved by build: Improvements to symbol visibility logic on Windows (attempt 3) #1367
• 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)) See build: Meta-issue for follow-ups to initial CMake merge (#1113) #1235 (comment) and build: Meta-issue for follow-ups to initial CMake merge (#1113) #1235 (comment)
• Print out a warning in the tests if both VERIFY and COVERAGE are defined. For clarity, this should be done in the C source, not in the build system. (build: Add CMake-based build system #1113 (comment)); solved by test: Warn if both VERIFY and COVERAGE are defined #1333
• Actually check if ARM assembly is supported (build: Add CMake-based build system #1113 (comment) and also tracked in build: Rename arm to arm32 and check if it's really supported #1034); solved by build: Rename arm to arm32 and check if it's really supported #1304
• Set -fvisibility-inlines-hidden? (build: Add CMake-based build system #1113 (comment)). See build: Meta-issue for follow-ups to initial CMake merge (#1113) #1235 (comment).
• Consider dropping support for Valgrind on MacOS (build: Add CMake-based build system #1113 (comment)). See build: Meta-issue for follow-ups to initial CMake merge (#1113) #1235 (comment).

Affecting only CMake:

• Changelog entry (build: Add CMake-based build system #1113 (comment); solved by changelog: Add entry for CMake #1225)
• Add CMake instructions to release process (solved by Add CMake instructions to release process #1226)
• Compute string secp256k1 (e.g., in target_link_libraries) as substring of PROJECT_NAME. That will help with distinguishing forks. (cmake: Rename project to "libsecp256k1" #1229 (comment)).
• Consider recommending CMake-native build commands in the README (build: Add CMake-based build system #1113 (comment), solved by cmake: Recommend native CMake commands in README #1483)
• Use GENERATOR_IS_MULTI_CONFIG in the future, which requires CMake 3.9. (build: Add CMake-based build system #1113 (comment); solved by cmake: Bugfix and other improvements after bumping CMake up to 3.13 #1239)
• See if we can use CMake templates to have something similar to --enable-dev-mode. (build: Add CMake-based build system #1113 (comment); solved by cmake: Add dev-mode #1234)
• Consider dropping support for building static and shared libraries at the same time (build: Add CMake-based build system #1113 (comment)) or alternatively, enable position-independent code globally (build: Add CMake-based build system #1113 (comment); solved by Build: allow static or shared but not both #1230)
• configure_package_config_file(... COMPATIBILITY is SameMajorVersion but should probably be SameMinorVersion before 1.0.0 according to our SemVer interpretation. But the latter requires CMake 3.11. (https://gnusha.org/secp256k1/2023-03-08.log; solved by cmake: Bugfix and other improvements after bumping CMake up to 3.13 #1239)
• make check should run tests in parallel, like ctest -j (https://gnusha.org/secp256k1/2023-03-08.log; WIP in by cmake: Emulate GNU Autotools 'make check -jN' #1294
• Consider using COMPILE_FLAGS or COMPILE_OPTIONS to add CFLAGS. COMPILE_OPTIONS requires CMake 3.11, while we currently target CMake 3.1 (build: Add CMake-based build system #1113 (comment)) See build: Meta-issue for follow-ups to initial CMake merge (#1113) #1235 (comment)
• Consider CMAKE_COMPILE_WARNING_AS_ERROR for CMake 3.24+ (build: Improve SECP_TRY_APPEND_DEFAULT_CFLAGS macro #1241 (comment)) See build: Meta-issue for follow-ups to initial CMake merge (#1113) #1235 (comment)
• Make SECP256K1_INSTALL default to PROJECT_IS_TOP_LEVEL on CMake 3.21+ (cmake: Make installation optional #1263); solved by cmake: Some improvements using PROJECT_IS_TOP_LEVEL variable #1284

[theuni]

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

Valgrind .............................. OFF
...
ctime_tests ......................... ON
...

/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?

[hebasto]

@theuni

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

Valgrind .............................. OFF
...
ctime_tests ......................... ON
...

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

[theuni]

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

[sipa]

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

[hebasto]

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.

[hebasto]

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:

• CMAKE_C_FLAGS
• CMAKE_C_FLAGS_<CONFIG>
• flags added by add_compile_options()
• flags added by target_compile_options()

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.

[hebasto]

• Set -fvisibility-inlines-hidden? (build: Add CMake-based build system #1113 (comment))

GCC:

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

So removing this task from the list.

[hebasto]

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

[hebasto]

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:

{
  "version": 1,
  "configurePresets": [
    {
      "name": "macos-x86_64",
      "displayName": "Cross-compiling for macOS x86_64",
      "generator": "Unix Makefiles",
      "binaryDir": "${sourceDir}/build",
      "cacheVariables": {
        "CMAKE_SYSTEM_NAME": "Darwin",
        "CMAKE_SYSTEM_PROCESSOR": "x86_64",
        "CMAKE_C_COMPILER": "/usr/bin/clang-14;--target=x86_64-apple-darwin",
        "CMAKE_OSX_SYSROOT": "/home/hebasto/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers"
      },
      "environment": {
        "LDFLAGS": "-fuse-ld=/usr/bin/ld64.lld-14 -mmacosx-version-min=10.15 -mlinker-version=609"
      },
      "warnings": {
        "dev": true,
        "uninitialized": true
      }
    }
  ]
}

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.

[hebasto]

• Consider using COMPILE_FLAGS or COMPILE_OPTIONS to add CFLAGS. COMPILE_OPTIONS requires CMake 3.11, while we currently target CMake 3.1 (build: Add CMake-based build system #1113 (comment))

... 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 cmake: Improve and document compiler flag checks #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.

[real-or-random]

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.

[hebasto]

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.

[hebasto]

• Consider dropping support for Valgrind on MacOS (build: Add CMake-based build system #1113 (comment))

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.

[hebasto]

• 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?

[sipsorcery]

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.

[real-or-random]

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

[hebasto]

• 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))

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

[hebasto]

• Consider setting CMake's default of /MD for MSVC also in autotools builds... (build: Add CMake-based build system #1113 (comment))

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.

[hebasto]

• Consider CMAKE_COMPILE_WARNING_AS_ERROR for CMake 3.24+ (build: Improve SECP_TRY_APPEND_DEFAULT_CFLAGS macro #1241 (comment))

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

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.

[real-or-random]

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.

[real-or-random]

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.

[hebasto]

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 :)

[hebasto]

• Compute string secp256k1 (e.g., in target_link_libraries) as substring of PROJECT_NAME. That will help with distinguishing forks. (cmake: Rename project to "libsecp256k1" #1229 (comment)).

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.

[real-or-random]

• Compute string secp256k1 (e.g., in target_link_libraries) as substring of PROJECT_NAME. That will help with distinguishing forks. (cmake: Rename project to "libsecp256k1" #1229 (comment)).

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.