search

bitcoin-core / secp256k1

Optimized C library for EC operations on curve secp256k1
MIT License
2.07k stars 1k forks source link

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

Closed hebasto closed 3 months ago

hebasto commented 1 year ago

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

Affecting both build systems:

Affecting only CMake:

theuni commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

hebasto commented 1 year ago

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 commented 1 year ago

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, https://github.com/bitcoin-core/secp256k1/pull/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 commented 1 year ago

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 commented 1 year ago

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

real-or-random commented 1 year ago

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 commented 1 year ago

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 https://github.com/bitcoin-core/secp256k1/pull/1251.

hebasto commented 1 year ago

From the recent discussion in https://github.com/bitcoin-core/secp256k1/pull/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 commented 1 year ago
  • [ ] Consider setting CMake's default of /MD for MSVC also in autotools builds.

https://github.com/bitcoin-core/secp256k1/pull/1113#issuecomment-1458005771:

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

CMake continuously introduces new abstractions for different aspects of the building process. Another example is DLL_NAME_WITH_SOVERSION: https://github.com/bitcoin-core/secp256k1/pull/1270#discussion_r1243384113.

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 7 months ago

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 commented 3 months ago

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 https://github.com/bitcoin-core/secp256k1/pull/1372.