hebasto commented at 12:23 PM on June 28, 2022: member

This PR adds a CMake-based build system.

Added build instructions and examples to the README.md file.

Ways to integrate with downstream CMake-based projects:

if secp256k1 is a subtree (including Bitcoin Core project) -- add_subdirectory(secp256k1)
if secp256k1 has been installed -- find_package(secp256k1 0.2.1 CONFIG), see https://github.com/hebasto/secp256k1-CMake-example

Added a few toolchain files for easy cross compiling.

Discussions on IRC:

Related PRs:

Implementation notes

Minimum required CMake version is 3.1. This was required to provide C_STANDARD property.

In turn, this choice of CMake version implies it is not possible to build with default CMake on Debian 8, which has CMake v3.0.2 only.

Also see:

Autotools -- CMake Feature Parity Tables

1. Configuration options

Autotool-based build system features being listed according to the ./configure --help output.

Autotools	CMake

A screenshot of grouped options from cmake-gui:

2. `make` targets

Autotools	CMake
`make`	`make`
`make check`	`make check`
`make install`	`make install` *

Installation of lib/pkgconfig/libsecp256k1.pc not implemented.

in CMakeLists.txt:24 in be3bb3072c outdated

   0 | @@ -0,0 +1,111 @@
   1 | +# Copyright 2022
   2 | +# Distributed under the MIT software license, see the accompanying
   3 | +# file COPYING or https://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +cmake_minimum_required(VERSION 3.1)
   6 | +project(secp256k1 VERSION 0.1.0 LANGUAGES C)
   7 | +
   8 | +set(CMAKE_C_STANDARD 90)

fanquake commented at 12:33 PM on June 28, 2022:

Might be worth adding a comment that in cmake, 89/90 are the same option.

real-or-random commented at 2:08 PM on June 28, 2022:

I don't think that's necessary, they're also the same outside of cmake. "C89" and "C90" are just synonyms.

in CMakeLists.txt:3 in be3bb3072c outdated

   0 | @@ -0,0 +1,111 @@
   1 | +# Copyright 2022
   2 | +# Distributed under the MIT software license, see the accompanying
   3 | +# file COPYING or https://www.opensource.org/licenses/mit-license.php.

fanquake commented at 12:35 PM on June 28, 2022:

Should probably use the same copyright header as every other file in the project. i.e:

/***********************************************************************
 * Copyright (c) 2022                                                                                      *
 * Distributed under the MIT software license, see the accompanying      *
 * file COPYING or https://www.opensource.org/licenses/mit-license.php. *
   ***********************************************************************/

hebasto commented at 1:13 PM on June 28, 2022:

CMake language have no /*...*/ comments.

in README.md:85 in be3bb3072c outdated

  78 | +    $ make
  79 | +    $ sudo make install  # optional
  80 | +
  81 | +To adjust the build system configuration, the following options can be provided to `cmake` call:
  82 | +
  83 | +| Option | Default value | Description |

fanquake commented at 12:38 PM on June 28, 2022:

Does cmake have the equivalent of ./configure --help that will just output the options? Otherwise you are creating documentation that will by default, go out of date, and/or need to be manually updated to stay in sync with the build system. It's easier to just get the options from the build system itself (as above), then they will always be correct.

hebasto commented at 1:24 PM on June 28, 2022:

Does cmake have the equivalent of ./configure --help that will just output the options?

cmake -LH ..

fanquake commented at 2:21 PM on June 28, 2022:

Nice. Then I think the table could be dropped in favour of a line similar to above. i.e:

"To compile optional modules (such as Schnorr signatures), you need to run cmake with additional flags (such as -DENABLE_MODULE_SCHNORRSIG=ON). Run cmake -LH .. to see the full list of available flags."

real-or-random commented at 9:57 AM on June 30, 2022:

I agree, this will be easier.

hebasto commented at 2:57 PM on June 30, 2022:

Thanks! Updated.

fanquake commented at 12:58 PM on June 28, 2022: member

Concept ACK (if there's buy-in from the maintainers here). Tested using the suggested instructions, on a macos box, but the build fails to link:

[ 78%] Building C object src/CMakeFiles/tests.dir/tests.c.o
[ 85%] Linking C executable tests
ld: library not found for -lcrt0.o
clang: error: linker command failed with exit code 1 (use -v to see invocation)
gmake[2]: *** [src/CMakeFiles/tests.dir/build.make:98: src/tests] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:212: src/CMakeFiles/tests.dir/all] Error 2
gmake: *** [Makefile:136: all] Error 2

Building on Ubuntu and cross-compiling for Windows and running the tests seem to work as expected.

hebasto force-pushed on Jun 28, 2022

real-or-random commented at 2:12 PM on June 28, 2022: contributor

Weak Concept ACK. I think there's user demand for this, and if we get some occasional help from the Core build people, maintenance should be doable.

hebasto commented at 2:18 PM on June 28, 2022: member

Updated be3bb3072cf52202b3f334e340bf4992e19b8b5c -> 697107c06e0837014ae24124dd6db81d6cb87485 (pr1113.01 -> pr1113.02, diff):

addressed @fanquake's comment:

Tested using the suggested instructions, on a macos box, but the build fails to link

hebasto force-pushed on Jun 28, 2022

hebasto commented at 5:13 PM on June 28, 2022: member

Updated 697107c06e0837014ae24124dd6db81d6cb87485 -> dba933ac9ffaea9fedf62a9f324891f440c2ca76 (pr1113.02 -> pr1113.03, diff):

make install works as expected now :tiger2:

hebasto force-pushed on Jun 28, 2022

hebasto commented at 8:02 PM on June 28, 2022: member

Updated dba933ac9ffaea9fedf62a9f324891f440c2ca76 -> ec372dc0b8a95053a771614931982e29291b6dd8 (pr1113.03 -> pr1113.04, diff):

added an example of cross compiling for Android
added an example of native compiling on Windows with Visual Studio

hebasto cross-referenced this on Jun 28, 2022 from issue Is there an option to create a shared library for Android applications? by gecng

in CMakeLists.txt:9 in ec372dc0b8 outdated

   0 | @@ -0,0 +1,111 @@
   1 | +# Copyright 2022
   2 | +# Distributed under the MIT software license, see the accompanying
   3 | +# file COPYING or https://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +cmake_minimum_required(VERSION 3.1)
   6 | +project(secp256k1 VERSION 0.1.0 LANGUAGES C)
   7 | +
   8 | +set(CMAKE_C_STANDARD 90)
   9 | +set(CMAKE_C_STANDARD_REQUIRED ON)

real-or-random commented at 9:23 AM on June 29, 2022:

I don't think we'll need this. I think this is rather for cases where you for example need C11 because your code uses C11 features. Then setting this to ON then forces an error if the compiler can do only C99.

hebasto commented at 7:24 PM on June 29, 2022:

Thanks! Updated.

in CMakeLists.txt:14 in ec372dc0b8 outdated

   9 | +set(CMAKE_C_STANDARD_REQUIRED ON)
  10 | +set(CMAKE_C_EXTENSIONS OFF)
  11 | +
  12 | +option(ENABLE_DEV_MODE "enable all binaries and modules by default but individual options can still be overridden explicitly" OFF)
  13 | +
  14 | +option(BUILD_BENCHMARK "compile benchmark" ON)

real-or-random commented at 9:24 AM on June 29, 2022:

option(BUILD_BENCHMARK "compile benchmarks" ON)

maybe we should even change the autoconf switch to plural, we have multiple benchmark binaries.

real-or-random commented at 9:26 AM on June 29, 2022:

nit: shouldn't this be rather "build benchmarks" (instead of "compile") if that's the wording of the variable. (I think "build" is better, it refers to the entire process, where "compile" wouldn't include "link").

hebasto commented at 7:24 PM on June 29, 2022:

Thanks! Updated.

in CMakeLists.txt:26 in ec372dc0b8 outdated

  21 | +option(ENABLE_MODULE_EXTRAKEYS "enable extrakeys module" ${ENABLE_DEV_MODE})
  22 | +option(ENABLE_MODULE_SCHNORRSIG "enable schnorrsig module" ${ENABLE_DEV_MODE})
  23 | +
  24 | +option(ALLOW_EXPERIMENTAL "allow experimental configure options" ${ENABLE_DEV_MODE})
  25 | +option(USE_EXTERNAL_DEFAULT_CALLBACKS "enable external default callback functions" OFF)
  26 | +option(COVERAGE "enable compiler flags to support kcov coverage analysis" ${ENABLE_DEV_MODE})

real-or-random commented at 9:27 AM on June 29, 2022:

option(COVERAGE "enable compiler flags to support coverage analysis" ${ENABLE_DEV_MODE})

I think kcov is the tool special to the linux kernel. I don't think we'll need to mention the tool here.

hebasto commented at 7:24 PM on June 29, 2022:

Thanks! Updated.

in CMakeLists.txt:47 in ec372dc0b8 outdated

  27 | +
  28 | +set(ECMULT_WINDOW_SIZE "auto" CACHE STRING "window size for ecmult precomputation for verification, specified as integer in range [2..24]. \"auto\" is a reasonable setting for desktop machines (currently 15). [default=auto]")
  29 | +if(ECMULT_WINDOW_SIZE STREQUAL auto)
  30 | +  set(ECMULT_WINDOW_SIZE 15)
  31 | +endif()
  32 | +if(NOT ECMULT_WINDOW_SIZE MATCHES ^[1-9][0-9]*$ OR ECMULT_WINDOW_SIZE LESS 2 OR ECMULT_WINDOW_SIZE GREATER 24)

real-or-random commented at 9:30 AM on June 29, 2022:

LESS and GREATER are only true if the arguments are a number, see https://cmake.org/cmake/help/latest/command/if.html#comparisons, so this could be simplified

if(ECMULT_WINDOW_SIZE GREATER 1 AND ECMULT_WINDOW_SIZE LESS 25)

hebasto commented at 11:16 AM on June 29, 2022:

Your suggestion checks for value validity, rather invalidity :)

If you mean to drop regexp, for some reasons (I did not investigate them) it will accept -DECMULT_WINDOW_SIZE=8a.

real-or-random commented at 1:43 PM on June 29, 2022:

Hm, I see. It probably uses something like the C functions atoi() or atol() under the hood; I think they simply ignoring trailing garbage. We could also convert it to a number via math() but yeah, this is not elegant either.

Your suggestion checks for value validity, rather invalidity :)

ok yes but this could be fixed :)

real-or-random commented at 10:03 AM on June 30, 2022:

Maybe it would make sense to extract MATCHES ^[1-9][0-9]*$ into a separate macro/function IS_NUM or similar and then use it for all numeric options.

Just a suggestion, I don't have strong opinions, as long as the code does what it's supposed to do.

hebasto commented at 3:00 PM on June 30, 2022:

I'd leave it as is for now. Code still concise and readable, imo :)

in CMakeLists.txt:57 in ec372dc0b8 outdated

  35 | +
  36 | +set(ECMULT_GEN_PREC_BITS "auto" CACHE STRING "Precision bits to tune the precomputed table size for signing, specified as integer 2, 4 or 8. \"auto\" is a reasonable setting for desktop machines (currently 4). [default=auto]")
  37 | +if(ECMULT_GEN_PREC_BITS STREQUAL auto)
  38 | +  set(ECMULT_GEN_PREC_BITS 4)
  39 | +endif()
  40 | +if(NOT ECMULT_GEN_PREC_BITS STREQUAL 2 AND NOT ECMULT_GEN_PREC_BITS STREQUAL 4 AND NOT ECMULT_GEN_PREC_BITS STREQUAL 8)

real-or-random commented at 9:32 AM on June 29, 2022:

maybe

if(NOT ECMULT_GEN_PREC_BITS EQUAL 2 AND NOT ECMULT_GEN_PREC_BITS EQUAL 4 AND NOT ECMULT_GEN_PREC_BITS EQUAL 8)

hebasto commented at 11:08 AM on June 29, 2022:

For some reasons (I did not investigate them) it will accept -DECMULT_GEN_PREC_BITS=8a.

in CMakeLists.txt:111 in ec372dc0b8 outdated

  43 | +
  44 | +option(USE_FORCE_WIDEMUL_INT128 "force the use of the (unsigned) __int128 based wide multiplication implementation" OFF)
  45 | +option(USE_FORCE_WIDEMUL_INT64 "force the use of the (u)int64_t based wide multiplication implementation" OFF)
  46 | +if (USE_FORCE_WIDEMUL_INT128 AND USE_FORCE_WIDEMUL_INT64)
  47 | +  message(FATAL_ERROR "USE_FORCE_WIDEMUL_INT128 and USE_FORCE_WIDEMUL_INT64 cannot be enabled simultaneously.")
  48 | +endif()

real-or-random commented at 9:33 AM on June 29, 2022:

Is there a way to "hide" these options a little bit? These are really "if you know what you're doing" and intended for CI and maintainers. Maybe this here: https://cmake.org/cmake/help/latest/command/mark_as_advanced.html ?

hebasto commented at 7:25 PM on June 29, 2022:

Thanks! Updated.

Also included ENABLE_DEV_MODE.

in CMakeLists.txt:210 in ec372dc0b8 outdated

  67 | +secp_try_append_cflag(-Wcast-align=strict)
  68 | +secp_try_append_cflag(-Wconditional-uninitialized)
  69 | +
  70 | +if(CMAKE_VERSION VERSION_GREATER 3.2)
  71 | +  cmake_policy(SET CMP0063 NEW)
  72 | +endif()

real-or-random commented at 9:50 AM on June 29, 2022:

Want to add a comment here?

It may be good to add also CMP0092:, /W3 has thousands of annoying/stupid warnings for our code IIRC https://gitlab.kitware.com/cmake/cmake/-/merge_requests/3250/diffs?commit_id=1baf122cd41d6500139649661052236b8e684565

hebasto commented at 7:25 PM on June 29, 2022:

Thanks! Updated.

real-or-random commented at 9:56 AM on June 30, 2022:

Want to add a comment here?

still open, I think the new one should also get a comment. I guess searching the web for CMP00XX will quickly give you a result but a one-line comment will help the reader

hebasto commented at 3:00 PM on June 30, 2022:

CMake policies have been commented.

in README.md:107 in ec372dc0b8 outdated

 102 | +To alleviate cross compiling, preconfigured toolchain files are available in the `toolchain` directory.
 103 | +For example, to cross compile for Windows:
 104 | +
 105 | +    $ cmake -DCMAKE_TOOLCHAIN_FILE=../toolchain/x86_64-w64-mingw32.cmake ..
 106 | +
 107 | +To compile for Android with NDK (not using a toolchain file, and assuming the `ANDROID_HOME` environment variable has been set):

real-or-random commented at 10:00 AM on June 29, 2022:

I think the correct variable is ANDROID_NDK_ROOT, see https://github.com/openssl/openssl/issues/11205 and the docs https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling-for-android . Maybe it makes sense to have a link to the docs, or only have that link. Not sure what others think.

hebasto commented at 7:25 PM on June 29, 2022:

Thanks! Updated.

hebasto force-pushed on Jun 29, 2022

hebasto commented at 7:23 PM on June 29, 2022: member

Updated ec372dc0b8a95053a771614931982e29291b6dd8 -> d98be43d08fcfaef756309bb3048a57274b0ccb6 (pr1113.04 -> pr1113.05, diff):

addressed @real-or-random's comments

hebasto force-pushed on Jun 29, 2022

in CMakeLists.txt:1 in ff836b6199 outdated

   0 | @@ -0,0 +1,119 @@
   1 | +# Copyright 2022

real-or-random commented at 10:05 AM on June 30, 2022:

You should probably add your name here and everywhere else. (I think that's an annoyance here in the library. It would be good to switch to "libsecp256k1 contributors" but last time I brought that up, it wasn't clear if this is legally okay.)

hebasto commented at 3:01 PM on June 30, 2022:

Updated.

hebasto renamed this:
~~[WIP] build: Add CMake-based build system~~
build: Add CMake-based build system
on Jun 30, 2022

in CMakeLists.txt:28 in ff836b6199 outdated

  23 | +option(ENABLE_MODULE_EXTRAKEYS "enable extrakeys module" ${ENABLE_DEV_MODE})
  24 | +option(ENABLE_MODULE_SCHNORRSIG "enable schnorrsig module" ${ENABLE_DEV_MODE})
  25 | +
  26 | +option(ALLOW_EXPERIMENTAL "allow experimental configure options" ${ENABLE_DEV_MODE})
  27 | +option(USE_EXTERNAL_DEFAULT_CALLBACKS "enable external default callback functions" OFF)
  28 | +option(COVERAGE "enable compiler flags to support coverage analysis" ${ENABLE_DEV_MODE})

real-or-random commented at 2:35 PM on June 30, 2022:

COVERAGE shouldn't be enabled by ENABLE_DEV_MODE

hebasto commented at 3:01 PM on June 30, 2022:

Thanks! Fixed.

real-or-random commented at 2:42 PM on June 30, 2022: contributor

It's correct to add the config header template libsecp256k1-config.h.in because it replicates what autoconf does but I think maintaining this file will be annoying, I guess. It will mean we'll have the duplicate the help strings.

I'm not exactly sure what's the best way forward.

We could either try to get rid of the config header entirely, also in autoconf. That simplifies things (generated source files are always annoying in some way. The autoconf manual gives the following rationale for preferring a config header over -D:

When a package contains more than a few tests that define C preprocessor symbols, the command lines to pass -D options to the compiler can get quite long. This causes two problems. One is that the make output is hard to visually scan for errors. More seriously, the command lines can exceed the length limits of some operating systems. [...] The fact that the symbols are documented is important in order to check that config.h makes sense. The fact that there is a well-defined list of symbols that should be defined (or not) is also important for people who are porting packages to environments where configure cannot be run: they just have to fill in the blanks.

This first part is not entirely convincing for us. The path limitation seems to a problem only on ancient system and anyway, the number of options is limited: With #929 solved, we'd have preprocessor defaults for the two numeric options (-DECMULT_GEN_PREC_BITS=4 -D ECMULT_WINDOW_SIZE=15), and so the values a typical user needs to set are of type ENABLE_MODULE_FOO and -DUSE_EXTERNAL_ASM.

On the other hand, one can argue that a config header is actually a good thing for people who build without a build system because it lists and documents all the options. Unfortunately the .in formats for autotools and cmake are not really compatible, so we can't use the same file for both.

Maybe it doesn't matter all that much. In the end, we need a least one place (and as few as possible) places where the options are listed and documented, and some duplication will be unavoidable if we want to support more than one of building even if it's just "autotools" and "manual". But I still I tend to think that we should get rid of the config header entirely. The only real advantage is that it helps people without a build system to fill in the gaps but that's a rare case, and we should simply document the options somewhere else. Maybe we could have a single table in the README with multiple columns: one for the autotool argument, one for manual build, and (if this PR here finds acceptance), for cmake:

<table> <tr> <td>Option <td>./configure argument <td>cmake argument <td>manual argument <tr> <td>Enable the Schnorrsig module <td><pre>--enable-module-schnorrsig</pre> <td><pre>-DENABLE_MODULE_SCHNORRSIG=ON</pre> <td><pre>-DENABLE_MODULE_SCHNORRSIG</pre> </table>

There may even be a way to unify the last two columns.

hebasto force-pushed on Jun 30, 2022

hebasto commented at 2:56 PM on June 30, 2022: member

Updated ff836b6199d81e63f61afee1b8385e815ca25c47 -> f587a8cfe16704d29ad0d4409a90807ef8320147 (pr1113.06 -> pr1113.07, diff):

addressed @fanquake's and @real-or-random's comments
moved toolchain fails into cmake directory

in src/CMakeLists.txt:7 in f587a8cfe1 outdated

   0 | @@ -0,0 +1,71 @@
   1 | +# Copyright 2022 Hennadii Stepanov
   2 | +# Distributed under the MIT software license, see the accompanying
   3 | +# file COPYING or https://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +add_definitions(-DHAVE_CONFIG_H)
   6 | +include_directories(${PROJECT_SOURCE_DIR} ${PROJECT_SOURCE_DIR}/include)
   7 | +include_directories(${PROJECT_BINARY_DIR}/src)

real-or-random commented at 3:29 PM on June 30, 2022:

These lines can (and should) be removed once #1116 has been merged.

The only paths that needs to be set then is ${PROJECT_SOURCE_DIR}/include for the code examples.

edit: well no, it's more complicated, see below.

hebasto commented at 4:29 PM on June 30, 2022:

While the libsecp256k1-config.h is still present in the project, including of PROJECT_BINARY_DIR is required because it is the place where all build artifacts, including the libsecp256k1-config.h, reside.

hebasto commented at 9:16 PM on July 1, 2022:

include_directories(${PROJECT_SOURCE_DIR} ${PROJECT_SOURCE_DIR}/include) has been removed.

hebasto cross-referenced this on Jun 30, 2022 from issue build: Fix #include "..." paths to get rid of further -I arguments by real-or-random

real-or-random commented at 4:08 PM on June 30, 2022: contributor

Hm, this is currently very confusing... so /src/util.h (where / is the project root) has this: https://github.com/bitcoin-core/secp256k1/blob/43756da819a235d813e7ecd53eae6df073b53247/src/util.h#L10-L12

Though this is implementation-defined, basically all compilers look in the current directory first, so they look for /src/libsecp256k1-config.h, which exists on my system because autotools generated it.

Only if this file doesn't exist, the compiler will try the -I paths, which -- when set correctly -- could include /src/build, so then it would find the /src/build/libsecp256k1-config.h, which cmake generated in its out-of-tree build directory...

That's pretty annoying. So we could change the include to #include <src/libsecp256k1-config.h> and then let the build system set -I accordingly. That's okayish, but getting rid of the config header (see above) is probably even easier.

hebasto force-pushed on Jul 1, 2022

hebasto commented at 9:15 PM on July 1, 2022: member

Updated f587a8cfe16704d29ad0d4409a90807ef8320147 -> 5dda6460657778e52b89a7920a8b63a032f5183f (pr1113.07 -> pr1113.08):

rebased on top of the #1116
addressed @real-or-random's comment

hebasto force-pushed on Jul 1, 2022

in CMakeLists.txt:107 in 6b65314195 outdated

 102 | +message("  ecmult window size ............... ${ECMULT_WINDOW_SIZE}")
 103 | +message("  ecmult gen precision bits ........ ${ECMULT_GEN_PREC_BITS}")
 104 | +message("Optional features:")
 105 | +message("  external callbacks ............... ${USE_EXTERNAL_DEFAULT_CALLBACKS}")
 106 | +if(BUILD_TESTS OR BUILD_EXHAUSTIVE_TESTS)
 107 | +  message("  support kcov coverage analysis ... ${COVERAGE}")

real-or-random commented at 5:55 PM on July 2, 2022:

  message("  support coverage analysis ... ${COVERAGE}")

hebasto commented at 3:08 PM on July 8, 2022:

Thanks! Updated.

real-or-random commented at 4:35 PM on July 8, 2022:

I just realized that this says "kcov" because the configure --help uses the same wording. I guess we should should update this when tidying up the configuration options as outlaid above. Then it would be helpful to reduce the doc strings in autotools and here to a minimum, and add larger explanations to the README. (This won't avoid duplication entirely but at least makes it less worse...)

in CMakeLists.txt:21 in 6b65314195 outdated

  16 | +option(ENABLE_DEV_MODE "enable all binaries and modules by default but individual options can still be overridden explicitly" OFF)
  17 | +
  18 | +option(BUILD_BENCHMARK "build benchmarks" ON)
  19 | +option(BUILD_TESTS "build tests" ON)
  20 | +option(BUILD_EXHAUSTIVE_TESTS "build exhaustive tests" ON)
  21 | +option(BUILD_EXAMPLES "build examples" ${ENABLE_DEV_MODE})

real-or-random commented at 5:56 PM on July 2, 2022:

BUILD_EXAMPLES is without function currently.

hebasto commented at 3:09 PM on July 8, 2022:

Implemented.

in CMakeLists.txt:16 in 6b65314195 outdated

  11 | +project(secp256k1 VERSION 0.1.0 LANGUAGES C)
  12 | +
  13 | +set(CMAKE_C_STANDARD 90)
  14 | +set(CMAKE_C_EXTENSIONS OFF)
  15 | +
  16 | +option(ENABLE_DEV_MODE "enable all binaries and modules by default but individual options can still be overridden explicitly" OFF)

real-or-random commented at 6:02 PM on July 2, 2022:

Hm, I think having this as an option doesn't really work with CMake's "cache-based" config system. When ENABLE_MODULE_RECOVERY is OFF in the cache, setting ENABLE_DEV_MODE won't enable it.

Also there's no "order" on the config options. In ./configure, we process options from left-to-right, so later options can override the dev mode defaults. But this doesn't make sense in CMake.

Maybe there's another way to express this . https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html looks promising. Though this could be done in a separate PR. dev-mode is not a crucial feature and anyway we'd still have autotools.

hebasto commented at 12:53 PM on July 8, 2022:

Hm, I think having this as an option doesn't really work with CMake's "cache-based" config system. When ENABLE_MODULE_RECOVERY is OFF in the cache, setting ENABLE_DEV_MODE won't enable it.

In such cases a CMakeCache.txt file in the top directory of the build tree must be deleted.

For example, "File" --> "Delete Cache" in the cmake-gui menu :)

real-or-random commented at 4:37 PM on July 8, 2022:

In such cases a CMakeCache.txt file in the top directory of the build tree must be deleted.

Right that works but I doubt it's a great interface. I think if this is hard to get right, then it shouldn't be in a way of this PR and we could introduce a dev mode later.

hebasto commented at 8:20 PM on July 8, 2022:

The ENABLE_DEV_MODE option has been removed.

in CMakeLists.txt:40 in 6b65314195 outdated

  35 | +endif()
  36 | +if(NOT ECMULT_WINDOW_SIZE MATCHES ^[1-9][0-9]*$ OR ECMULT_WINDOW_SIZE LESS 2 OR ECMULT_WINDOW_SIZE GREATER 24)
  37 | +  message(FATAL_ERROR "ECMULT_WINDOW_SIZE value is \"${ECMULT_WINDOW_SIZE}\", but must an integer in range [2..24] or \"auto\".")
  38 | +endif()
  39 | +
  40 | +set(ECMULT_GEN_PREC_BITS "auto" CACHE STRING "Precision bits to tune the precomputed table size for signing, specified as integer 2, 4 or 8. \"auto\" is a reasonable setting for desktop machines (currently 4). [default=auto]")

real-or-random commented at 6:22 PM on July 2, 2022:

set(ECMULT_GEN_PREC_BITS "auto" CACHE STRING "Precision bits to tune the precomputed table size for signing, specified as integer 2, 4 or 8. \"auto\" is a reasonable setting for desktop machines (currently 4). [default=auto]")
set_property(CACHE ECMULT_GEN_PREC_BITS PROPERTY STRINGS "auto" 2 4 8)

real-or-random commented at 6:23 PM on July 2, 2022:

This makes ccmake and cmake-gui offer a nicer selection. We should probably do the same above. Listing the numbers from 2 to 24 looks strange but why not. (We'd like to restrict this in the future anyway.)

hebasto commented at 3:09 PM on July 8, 2022:

Thanks! Updated.

in CMakeLists.txt:50 in 6b65314195 outdated

  45 | +  message(FATAL_ERROR "ECMULT_GEN_PREC_BITS value is \"${ECMULT_GEN_PREC_BITS}\", but must an integer 2, 4, 8, or \"auto\".")
  46 | +endif()
  47 | +
  48 | +option(USE_FORCE_WIDEMUL_INT128 "force the use of the (unsigned) __int128 based wide multiplication implementation" OFF)
  49 | +option(USE_FORCE_WIDEMUL_INT64 "force the use of the (u)int64_t based wide multiplication implementation" OFF)
  50 | +if (USE_FORCE_WIDEMUL_INT128 AND USE_FORCE_WIDEMUL_INT64)

real-or-random commented at 6:27 PM on July 2, 2022:

nit: all the others ifs don't have a space after them. (but maybe it's nicer with a space?)

hebasto commented at 3:10 PM on July 8, 2022:

Thanks! Fixed.

in README.md:78 in 6b65314195 outdated

  73 | +
  74 | +To maintain a pristine source tree, CMake encourages to perform an out-of-source build by using a separate dedicated build tree.
  75 | +
  76 | +### Building on POSIX systems
  77 | +
  78 | +    $ rm -rf build && mkdir build && cd build

real-or-random commented at 6:28 PM on July 2, 2022:

    $ mkdir build && cd build

just a nit but I wouldn't suggest invasive commands

hebasto commented at 3:11 PM on July 8, 2022:

Thanks! Updated.

in README.md:105 in 6b65314195 outdated

 100 | +The following example assumes using of Visual Studio 2022 and CMake v3.21+.
 101 | +
 102 | +In "Developer Command Prompt for VS 2022":
 103 | +
 104 | +    >rd /s /q build && mkdir build && cd build
 105 | +    >cmake .. -G "Visual Studio 17 2022" -A x64 -DBUILD_BENCHMARK=OFF

real-or-random commented at 6:30 PM on July 2, 2022:

Is the reason for disabling the benchmarks that they don't work? If yes, have you tried on master? #1084 introduced benchmark support on Windows.

hebasto commented at 8:20 PM on July 8, 2022:

Thanks! Updated.

real-or-random commented at 6:32 PM on July 2, 2022: contributor

This is working pretty well already, I think. Have you tried adding the tests?

sipa cross-referenced this on Jul 5, 2022 from issue [secp256k1] Update secp256k1 from 2017 to 2022 by R0g3r10LL31t3

sipa commented at 8:11 PM on July 5, 2022: contributor

I tried the following:

mkdir build
cd build
cmake .. -DENABLE_MODULE_SCHNORRSIG=ON
make
./src/bench

The result seems to indicate that:

ECDH & recovery modules were built (despite configure telling me they weren't)
schnorrsig being built (despite being experimental, and experimental modules not enabled)
performance is very low (compiler optimizations off?)

EDIT: seems -DCMAKE_BUILD_TYPE=Release is needed to get optimizations enabled. This should perhaps be documented? (I guess this is standard, but I'm not familiar with this style of configuring things)

real-or-random commented at 8:20 PM on July 5, 2022: contributor

@sipa

ECDH & recovery modules were built (despite configure telling me they weren't)

This is probably the issue I already discovered (https://github.com/bitcoin-core/secp256k1/pull/1113#issuecomment-1171407985) ... can you try removing src/libsecp256k1-config.h, which is probably still around in your source tree from the autotools build, before trying this PR?

sipa commented at 8:23 PM on July 5, 2022: contributor

@real-or-random

can you try removing src/libsecp256k1-config.h, which is probably still around in your source tree from the autotools build, before trying this PR?

Done, that seems to help, though now I get errors because the extrakeys module doesn't seem enabled (which schnorrsig depends on). If I enable extrakeys explicitly, it works, and with -DCMAKE_BUILD_TYPE=Release added, I get believable bench output.

real-or-random commented at 8:33 PM on July 5, 2022: contributor

Oh module dependencies...

sipa commented at 8:35 PM on July 5, 2022: contributor

Those are not serious issues of course, and probably easy to resolve. It's nice to see that the hard part (it building/running in the first place, apparently including for MSVC) already seems to work.

real-or-random cross-referenced this on Jul 6, 2022 from issue config: Set preprocessor defaults for ECMULT_* config values by real-or-random

hebasto force-pushed on Jul 8, 2022

hebasto commented at 3:07 PM on July 8, 2022: member

Updated 6b65314195baeee982c7d5213377d493d073f119 -> 13c63e79b56ab00bffb0ff40d8a6a0f4b03ed620 (pr1113.09 -> pr1113.10):

rebased
added examples/CMakeLists.txt
option docstrings style now follows CMake's docstring style
addressed @real-or-random's and @sipa's comments
fixed module dependecies

hebasto commented at 3:19 PM on July 8, 2022: member

EDIT: seems -DCMAKE_BUILD_TYPE=Release is needed to get optimizations enabled.

By default (at least on Linux), -DCMAKE_BUILD_TYPE=Release adds -O3 -DNDEBUG flags, and -DCMAKE_BUILD_TYPE=Debug does -g.

Therefore, I'd suggest to do not use this option at all. Instead, Autotools' behavior has been re-imlemented, i.e., -O2, if no COVERAGE requested. And -O0 otherwise.

real-or-random commented at 4:31 PM on July 8, 2022: contributor

Thanks for constantly making updates, I think we're almost there. Though we may want to simplify the config system first and only then add CMake.

Therefore, I'd suggest to do not use this option at all.

I'm not sure if this is the way to go. CMAKE_BUILD_TYPE seems to be pretty idiomatic in CMake, so maybe that's what people would expect. But I need to do more reading, I haven't really checked.

So Autotools always enable -g -O2 and so far this was pretty useful for us... So as I understand it, RelWithDebInfo does this. Could we just set this as a default?

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

edit: removed leftover draft text

hebasto force-pushed on Jul 8, 2022

hebasto commented at 8:19 PM on July 8, 2022: member

Updated 13c63e79b56ab00bffb0ff40d8a6a0f4b03ed620 -> e9cebaaadfa3ba1a0f233d125fe2eed50f3993cb (pr1113.10 -> pr1113.11, diff):

improved documentation
removed the ENABLE_DEV_MODE option

hebasto commented at 1:50 PM on July 12, 2022: member

@real-or-random

So Autotools always enable -g -O2 and so far this was pretty useful for us... So as I understand it, RelWithDebInfo does this. Could we just set this as a default?

The default value of the CMAKE_C_FLAGS_RELWITHDEBINFO variable is -O2 -g -DNDEBUG. I think, we want to strip out -DNDEBUG. Or to define our own custom configuration, along with Coverage?

real-or-random commented at 11:00 PM on July 12, 2022: contributor

Sorry, I guess my suggestion wasn't clear. I meant

make RelWithDebInfo the default build type (is this even possible?)
and independently, introduce a separate build type Coverage.

The default value of the CMAKE_C_FLAGS_RELWITHDEBINFO variable is -O2 -g -DNDEBUG. I think, we want to strip out -DNDEBUG.

Yes, we probably would want to strip it. (Though it won't hurt, we never check for NDEBUG it in the code.)

hebasto commented at 7:13 AM on July 13, 2022: member

Though it won't hurt, we never check for NDEBUG it in the code.

But we use assert.

hebasto force-pushed on Jul 13, 2022

hebasto commented at 2:56 PM on July 13, 2022: member

Updated e9cebaaadfa3ba1a0f233d125fe2eed50f3993cb -> 2ecf0dc437db7c38d4428eca9d17b0168287983a (pr1113.11 -> pr1113.12):

rebased

CMAKE_BUILD_TYPE seems to be pretty idiomatic in CMake

introduced two new build types: "RelWithAsserts" and "Coverage"; the latter should be used instead of the COVERAGE option
removed the COVERAGE option

All default build types, i.e., "Debug", "Release", "MinSizeRel" and "RelWithDebInfo", are still available.

The default build type is "RelWithAsserts" which is equivalent to the Autotools' default.

sipa commented at 6:47 PM on July 13, 2022: contributor

There are only asserts in the API examples. For those it's probably better to by default leave the asserts in.

hebasto commented at 2:16 PM on July 21, 2022: member

@sipa

schnorrsig being built (despite being experimental, and experimental modules not enabled)

schnorrsig has not been treated as experimental since #995.

hebasto force-pushed on Jul 21, 2022

hebasto commented at 2:27 PM on July 21, 2022: member

From the recent meeting on IRC:

<real_or_random> hebasto: so you think we'd still have a config file for cmake? <hebasto> if build, in general, will support config-less routine, cmake won't require it

I was wrong. The cmake/libsecp256k1-config.h.in has been dropped in the latest push.

hebasto commented at 7:52 PM on July 31, 2022: member

#1113 (review):

You should probably add your name here and everywhere else. (I think that's an annoyance here in the library. It would be good to switch to "libsecp256k1 contributors" but last time I brought that up, it wasn't clear if this is legally okay.)

Wrt the recent discussion on IRC, should I switch to "libsecp256k1 contributors" in this PR?

hebasto force-pushed on Aug 2, 2022

hebasto commented at 10:15 AM on August 2, 2022: member

According to yesterday's IRC meeting, copyright headers have been removed from the new added files.

hebasto cross-referenced this on Aug 7, 2022 from issue build: Add CMake-based build system by hebasto

hebasto force-pushed on Aug 17, 2022

hebasto commented at 10:48 AM on August 17, 2022: member

Updated dc9124b2b23c5cfe4f3524b1e7defb528b62b839 -> 8d019748b599193510a61975371805c2841d7990 (pr1113.14 -> pr1113.15):

rebased
improved building with MSVC
improved hygiene of user-provided CFLAGS

hebasto force-pushed on Aug 21, 2022

hebasto commented at 9:45 AM on August 21, 2022: member

Updated 8d019748b599193510a61975371805c2841d7990 -> d77e4c89de78c2b2a523e9e56a931db80104a569 (pr1113.15 -> pr1113.16, diff):

added make test which is CMake's analogue of Autotools' make check
various improvements based on feedback from bitcoin/bitcoin#25797

The PR description has been updated with detailed Autotools -- CMake Feature Parity Tables.

hebasto force-pushed on Aug 21, 2022

hebasto commented at 12:57 PM on August 21, 2022: member

Updated d77e4c89de78c2b2a523e9e56a931db80104a569 -> 3dc4de05299835aa4ff39275507858ebd2ecb8f9 (pr1113.16 -> pr1113.17, diff):

fixed accidentally broken compatibility with CMake 3.1

Tested on Debian 8 with non-system CMake installation:

$ cmake --version
cmake version 3.1.3

CMake suite maintained and supported by Kitware (kitware.com/cmake).

hebasto force-pushed on Aug 21, 2022

hebasto commented at 8:52 PM on August 21, 2022: member

Updated 3dc4de05299835aa4ff39275507858ebd2ecb8f9 -> a891c91f5215dadb883b88ffa0108f38e4d2b23d (pr1113.17 -> pr1113.18, diff):

a prefix SECP_ has been added to all secp256k1-specific variables to avoid potential name clashes when this library build system being used as a sub-project in another build system
added SECP_BUILD_SHARED and SECP_BUILD_STATIC configuration options

hebasto marked this as a draft on Aug 22, 2022

hebasto cross-referenced this on Aug 23, 2022 from issue Add `tools/symbol-check.py` by hebasto

hebasto marked this as ready for review on Aug 23, 2022

hebasto force-pushed on Aug 23, 2022

hebasto commented at 10:48 PM on August 23, 2022: member

Updated a891c91f5215dadb883b88ffa0108f38e4d2b23d -> 1eca0329db3898cbbd0824803112441ae74d9acd (pr1113.18 -> pr1113.19, diff):

fixed all bugs and regressions found during extensive testing including cross building for Windows and macOS, and native building using MSVC

real-or-random commented at 2:33 PM on August 25, 2022: contributor

related, maybe there's some gold: https://github.com/Bitcoin-ABC/secp256k1/blob/master/CMakeLists.txt

sipa cross-referenced this on Aug 25, 2022 from issue Add vcpkg installation instructions by JonLiu1993

hebasto commented at 3:23 PM on August 25, 2022: member

related, maybe there's some gold: https://github.com/Bitcoin-ABC/secp256k1/blob/master/CMakeLists.txt

When I started to work on bringing CMake into Bitcoin Core and in here, I have skimmed that repository.

Did look into that listfile in particular again:

maybe dropping the "auto" value for the SECP_ECMULT_WINDOW_SIZE and SECP_ECMULT_GEN_PREC_BITS options is cleaner, but it slightly deviates from Autotools' configure implementation
they use SECP256K1_ prefix for option names, which is longer than ours SECP_ but follows the library name

What do you think?

real-or-random commented at 4:39 PM on August 25, 2022: contributor

Did look into that listfile in particular again:

* maybe dropping the "auto" value for the `SECP_ECMULT_WINDOW_SIZE` and `SECP_ECMULT_GEN_PREC_BITS` options is cleaner, but it slightly deviates from Autotools' `configure` implementation

In the end both is fine because we have the default values in the preprocessor (#1121) and later we probably want to set these only via the preprocessor. I think the current version of the PR is good because then the two build systems are in sync.

* they use `SECP256K1_` prefix for option names, which is longer than ours `SECP_` but follows the library name

What do you think?

I think among these two choices, the full name is better. I mean we prefix every symbol with the full secp256k1_, so it feels somewhat arbitrary to have a shorter prefix in the build system. But there's anyway some bikeshedding necessary for the prefixes of the macros. ( #1121 (comment) ).

Maybe now is the time to think about this. @sipa made a point that a general prefix SECP256K1_could be useful for all macros in the case that users will build the library by simply including the code. And this would be consistent with most of our function names already (except a few names in util.h). So if we want this, we could additionally think whether we want a prefix to distinguish external macros (supposed to be set by user/build system) from internal macros. But something like SECP256K1_CONFIG_ECMULT_WINDOW_SIZE is very long and not elegant. Maybe let's use only SECP256K1_ only as a prefix for external macros and then we could later still use something like SECP256K1_INTERNAL_ for internal macros if we want to name space them?

By the way:

a prefix SECP_ has been added to all secp256k1-specific variables to avoid potential name clashes when this library build system being used as a sub-project in another build system

Can this happen at all with CMake? (I have no idea about subbuilds with CMake).

hebasto commented at 4:47 PM on August 25, 2022: member

By the way:

a prefix SECP_ has been added to all secp256k1-specific variables to avoid potential name clashes when this library build system being used as a sub-project in another build system

Can this happen at all with CMake? (I have no idea about subbuilds with CMake).

If a subproject (e.g., libsecp256k1) being added to the parent project (e.g., Bitcoin Core) using the add_subdirectory command, all variables from the parent project scope are copied to the subproject scope.

ZenulAbidin commented at 2:14 PM on October 4, 2022: contributor

I support having a parallel CMake system in addition to the autoconf one. I might even help with maintaining it as I have a use for libsecp256k1 for cross-platform projects. @hebasto Is your CMakeLists.txt still usable with the latest commit of bitcoin-core/secp256k1 as-is?

I can try to get rid of src/libsecp256k1-config.h in a separate PR if there's still demand for that.

real-or-random cross-referenced this on Oct 4, 2022 from issue Add section on configuration flags to README by ZenulAbidin

hebasto commented at 2:20 PM on October 28, 2022: member

@ZenulAbidin

@hebasto Is your CMakeLists.txt still usable with the latest commit of bitcoin-core/secp256k1 as-is?

It is. Why?

hebasto force-pushed on Oct 30, 2022

hebasto commented at 9:24 PM on October 30, 2022: member

Updated 1eca0329db3898cbbd0824803112441ae74d9acd -> dcf19d6a0332f24120af5e7cd4707435dbcf2076 (pr1113.19 -> pr1113.20, diff).

The SECP256K1_ is used for user-defined CMake options now.

Now integration with downstream projects works as easy as follows:

if secp256k1 is a subtree (including Bitcoin Core project) -- add_subdirectory(secp256k1)
if secp256k1 has been installed -- find_package(secp256k1)

A new "x86_64: Windows (VS 2022)" CI task has been added: https://cirrus-ci.com/task/5007933141417984.

Also some other small improvement were done based on work on the related projects (https://github.com/bitcoin/bitcoin/pull/25797, https://github.com/sipa/minisketch/pull/75) and their integration.

hebasto force-pushed on Nov 1, 2022

hebasto commented at 1:33 PM on November 1, 2022: member

Updated dcf19d6a0332f24120af5e7cd4707435dbcf2076 -> 896229922fd85425db1d005a2ba8839e8fdffdd1 (pr1113.20 -> pr1113.21, diff):

fixed minor issues for various non-standard installation scenarios
fixed compatibility with CMake 3.1
a few other minor improvements

hebasto force-pushed on Nov 30, 2022

hebasto commented at 12:25 PM on November 30, 2022: member

Updated 896229922fd85425db1d005a2ba8839e8fdffdd1 -> bf0512788b4d8611136d55c224f10ae67cf5d002 (pr1113.21 -> pr1113.22):

rebased and synced with changes which were introduced in #993

hebasto force-pushed on Dec 14, 2022

hebasto commented at 1:23 PM on December 14, 2022: member

Updated bf0512788b4d8611136d55c224f10ae67cf5d002 -> 6d8b8dc666637343c69dd34ddce509f17639b7d9 (pr1113.22 -> pr1113.23):

rebased on top of the recent changes related to the v0.2.0 release

real-or-random cross-referenced this on Dec 14, 2022 from issue secp256k1-zkp with CMakeLists? by mattiaferrari02

hebasto force-pushed on Dec 20, 2022

hebasto commented at 9:11 AM on December 20, 2022: member

Updated 6d8b8dc666637343c69dd34ddce509f17639b7d9 -> efc15ad1a3ff6b25a68f7a2985ed2d2816cd9991 (pr1113.23 -> pr1113.24):

rebased on top of the recently merged #1178

This PR is ready for a final (?) review now.

hebasto force-pushed on Jan 17, 2023

hebasto commented at 3:35 PM on January 17, 2023: member

Updated efc15ad1a3ff6b25a68f7a2985ed2d2816cd9991 -> 858be0e97b05253a6274ec31e2eb321bc24dfcb4 (pr1113.24 -> pr1113.25):

rebased
incorporated noverify_tests (#1188) and ctime_tests (#1169)

in CMakeLists.txt:81 in 858be0e97b outdated

  76 | +if(NOT SECP256K1_ECMULT_GEN_PREC_BITS STREQUAL 2 AND NOT SECP256K1_ECMULT_GEN_PREC_BITS STREQUAL 4 AND NOT SECP256K1_ECMULT_GEN_PREC_BITS STREQUAL 8)
  77 | +  message(FATAL_ERROR "SECP256K1_ECMULT_GEN_PREC_BITS value is \"${SECP256K1_ECMULT_GEN_PREC_BITS}\", but must an integer 2, 4, 8, or \"auto\".")
  78 | +endif()
  79 | +add_definitions(-DECMULT_GEN_PREC_BITS=${SECP256K1_ECMULT_GEN_PREC_BITS})
  80 | +
  81 | +option(SECP256K1_USE_FORCE_WIDEMUL_INT128 "Force the use of the (unsigned) __int128 based wide multiplication implementation." OFF)

sipa commented at 6:52 PM on January 17, 2023:

On the configure side they're called --with-test-override-...; maybe it's useful to use a similar name on the cmake side (for the cmake flag, not the corresponding C macro), to make it clear they're not for normal usage?

hebasto commented at 3:59 PM on January 18, 2023:

Updated.

in CMakeLists.txt:85 in 858be0e97b outdated

  80 | +
  81 | +option(SECP256K1_USE_FORCE_WIDEMUL_INT128 "Force the use of the (unsigned) __int128 based wide multiplication implementation." OFF)
  82 | +if(SECP256K1_USE_FORCE_WIDEMUL_INT128)
  83 | +  add_definitions(-DUSE_FORCE_WIDEMUL_INT128)
  84 | +endif()
  85 | +option(SECP256K1_USE_FORCE_WIDEMUL_INT64 "Force the use of the (u)int64_t based wide multiplication implementation." OFF)

sipa commented at 6:56 PM on January 17, 2023:

I believe this is missing an equivalent to the configure option --with-test-override-widemul=int128_struct, added in 2914bccbc0913806ee64425a27d38cdc27b288e8 (#1000).

hebasto commented at 3:59 PM on January 18, 2023:

Thanks! Updated.

in CMakeLists.txt:104 in 858be0e97b outdated

  99 | +option(SECP256K1_BUILD_CTIME_TESTS "Build constant-time tests." OFF)
 100 | +option(SECP256K1_BUILD_EXHAUSTIVE_TESTS "Build exhaustive tests." ON)
 101 | +option(SECP256K1_BUILD_EXAMPLES "Build examples." OFF)
 102 | +
 103 | +# Redefine configuration flags.
 104 | +set(CMAKE_C_FLAGS_DEBUG "-O0 -g3")

sipa commented at 6:59 PM on January 17, 2023:

Does this work on MSVC?

hebasto commented at 3:58 PM on January 18, 2023:

Thanks! Fixed.

in CMakeLists.txt:207 in 858be0e97b outdated

 202 | +elseif(SECP256K1_USE_FORCE_WIDEMUL_INT64)
 203 | +  set(widemul_status "int64")
 204 | +else()
 205 | +  set(widemul_status OFF)
 206 | +endif()
 207 | +message("  override wide multiplication ........ ${widemul_status}")

sipa commented at 7:00 PM on January 17, 2023:

Perhaps this should not be reported unless overridden (as it's a test-only option).

hebasto commented at 3:59 PM on January 18, 2023:

Thanks! Updated.

in src/CMakeLists.txt:67 in 858be0e97b outdated

  62 | +  )
  63 | +endif()
  64 | +
  65 | +if(SECP256K1_BUILD_BENCHMARK)
  66 | +  add_executable(bench bench.c)
  67 | +  target_link_libraries(bench bench_or_test_binary)

sipa commented at 8:32 PM on January 17, 2023:

I don't think these link specifications are correct.

The examples, bench, and ctime_tests are supposed to be externally linked to libsecp256k1 (they're users of the library, and should just link to the built library, not to the objects inside of it).
bench_internal, bench_ecmult, tests, noverify_tests are supposed to be internally linked (so they're linked with the .o files from which the library is built from as well).

So for examples, when building with shared library (and no static library), I'd expect ldd tests to not show libsecp256k1.so, but ldd ctime_tests should show it.

In the other direction, nm ctime_tests should only show names of functions exported by the API, while nm tests should also show internal functions.

It appears that the current cmake code builds everything by linking with the internal objects. That will always work, however:

the ctime tests aren't exactly testing the release library file that's built.
the bench benchmarks similarly will not be benchmarking the release library.
the examples will be built in a way that's not available to actual users (as they only have the static/shared library, not the .o files it was built from).

hebasto commented at 4:00 PM on January 18, 2023:

Should work as expected now.

hebasto force-pushed on Jan 18, 2023

hebasto commented at 3:58 PM on January 18, 2023: member

Updated 858be0e97b05253a6274ec31e2eb321bc24dfcb4 -> 84f1fd27a07eb7f54bd6454019384660b6b4a108 (pr1113.25 -> pr1113.26):

rebased
addressed @sipa's recent comments

in src/CMakeLists.txt:72 in 84f1fd27a0 outdated

  67 | +  ${PROJECT_NAME}_static
  68 | +)
  69 | +
  70 | +if(SECP256K1_BUILD_BENCHMARK)
  71 | +  add_executable(bench bench.c)
  72 | +  target_link_libraries(bench binary_interface link_shared)

sipa commented at 6:15 PM on January 18, 2023:

I don't think this approach is correct either; it'll always link bench with the shared library, even when the shared library isn't being built. It should just link with whatever is being built - probably shared if shared is available, and static otherwise.

And below the opposite problem occurs: it always links with the static library, even when the static library isn't being built. In fact, it shouldn't be linked with either the static or shared library; it just needs to link the common object files (precomputed stuff, asm if included).

hebasto commented at 6:26 PM on January 18, 2023:

it'll always link bench with the shared library, even when the shared library isn't being built. It should just link with whatever is being built - probably shared if shared is available, and static otherwise.

Going to implement this suggestion.

And below the opposite problem occurs: it always links with the static library, even when the static library isn't being built. In fact, it shouldn't be linked with either the static or shared library; it just needs to link the common object files (precomputed stuff, asm if included).

I believe there is no difference between linking against object files and linking against a static library, which is an archive of object files, no?

sipa commented at 6:30 PM on January 18, 2023:

Sure, but the static library, as a build product, contains too much (it contains the entire library API code, which tests/exhaustive_tests/bench_ecmult/bench_internal don't need).

The point is that these binaries are at the same "level" as the library itself, not users of it.

hebasto commented at 7:32 PM on January 18, 2023:

Sure, but the static library, as a build product, contains too much (it contains the entire library API code, which tests/exhaustive_tests/bench_ecmult/bench_internal don't need).

FWIW, for example, the resulted bench_internal binary has the same nm output as it has being compiled on the master branch using Autotools-based build system.

Linking object files directly ends with linker errors like that:

/usr/bin/ld: CMakeFiles/secp256k1_obj.dir/secp256k1.c.o: in function `secp256k1_selftest':
/home/hebasto/git/secp256k1/src/secp256k1.c:85: multiple definition of `secp256k1_selftest'; CMakeFiles/bench_internal.dir/bench_internal.c.o:/home/hebasto/git/secp256k1/src/secp256k1.c:85: first defined here

sipa commented at 7:33 PM on January 18, 2023:

You can't include the object built from secp256k1.c, because that's included in the tests.c code already (that's exactly the point; the built static library does include these).

Only link with the objects from precomputed_*, and if need be, external asm objects.

hebasto commented at 8:19 PM on January 18, 2023:

Easy to reproduce with:

$ gcc src/bench_internal.c src/precomputed_ecmult.c src/precomputed_ecmult_gen.c src/secp256k1.c

sipa commented at 8:20 PM on January 18, 2023:

Don't include src/secp256k1.c, that's the point. The static library includes it, but the "internal" binaries don't need it.

hebasto commented at 9:12 PM on January 18, 2023:

Thanks! Updated.

hebasto force-pushed on Jan 18, 2023

hebasto commented at 9:11 PM on January 18, 2023: member

Updated 84f1fd27a07eb7f54bd6454019384660b6b4a108 -> 9c2295be845ec99446be8a894fa09f5191baa1b0 (pr1113.26 -> pr1113.27):

addressed @sipa's comment

hebasto force-pushed on Jan 18, 2023

hebasto commented at 9:39 PM on January 18, 2023: member

Updated 9c2295be845ec99446be8a894fa09f5191baa1b0 -> 4dad13e030bb557cf49b394bf79cb31ffe4043bb (pr1113.27 -> pr1113.28, diff):

fixed accidentally broken compatibility with CMake 3.1

hebasto force-pushed on Jan 19, 2023

hebasto commented at 3:29 PM on January 19, 2023: member

Updated 4dad13e030bb557cf49b394bf79cb31ffe4043bb -> aa1c233299ab6798d714a3ed6fc2a625dc3896eb (pr1113.28 -> pr1113.29, diff):

noverify_tests.exe and *_example.exe runs have been added to the native Windows CI task

hebasto force-pushed on Jan 22, 2023

hebasto commented at 2:16 PM on January 22, 2023: member

Updated aa1c233299ab6798d714a3ed6fc2a625dc3896eb -> 5c92794e3e23c10d5bf944c612687cabae935594 (pr1113.29 -> pr1113.30):

rebased
a minor fix applied

sipa commented at 7:19 PM on January 23, 2023: contributor

@hebasto So it seems that only the exernal ARM32 asm, and valgrind detection support are missing here. I don't think those are necessarily required before merging, but I'd like to hear what your plans are around adding support for those.

hebasto commented at 9:48 AM on January 24, 2023: member

@sipa

So it seems that only the exernal ARM32 asm, and valgrind detection support are missing here. I don't think those are necessarily required before merging, but I'd like to hear what your plans are around adding support for those.

I'm working on them. I promise to maintain CMake-based build system on par with Autotools-based one. Or better :)

hebasto force-pushed on Jan 24, 2023

hebasto commented at 7:55 PM on January 24, 2023: member

Updated 5c92794e3e23c10d5bf944c612687cabae935594 -> fbacf464ff1a8e1b46508aa9b4fba8d0a3052304 (pr1113.30 -> pr1113.31, diff):

added support for -DSECP256K1_ASM option

hebasto force-pushed on Jan 24, 2023

hebasto force-pushed on Jan 25, 2023

hebasto commented at 2:54 PM on January 25, 2023: member

Updated b09bfbfe7330795757f85c44fe3b204d76b60e93 -> 1b0e0ba27a57cf74fdd0f3c06e1441c44119883d (pr1113.32 -> pr1113.33, diff):

added support for -DSECP256K1_VALGRIND option
the PR description has been amended
a few tiny improvements

in CMakeLists.txt:129 in 1b0e0ba27a outdated

 124 | +  if(SECP256K1_ASM STREQUAL "arm")
 125 | +    message(FATAL_ERROR "ARM assembly optimization is experimental. Use -DSECP256K1_EXPERIMENTAL=ON to allow.")
 126 | +  endif()
 127 | +endif()
 128 | +
 129 | +option(SECP256K1_VALGRIND "Build with extra checks for running inside Valgrind." OFF)

sipa commented at 7:12 PM on January 25, 2023:

Can this be auto-detected and on by default if detected?

hebasto commented at 9:45 PM on January 25, 2023:

Sure! Done.

in CMakeLists.txt:134 in 1b0e0ba27a outdated

 137 | +  endif()
 138 | +endif()
 139 | +
 140 | +option(SECP256K1_BUILD_BENCHMARK "Build benchmarks." ON)
 141 | +option(SECP256K1_BUILD_TESTS "Build tests." ON)
 142 | +option(SECP256K1_BUILD_CTIME_TESTS "Build constant-time tests." ${SECP256K1_VALGRIND})

sipa commented at 7:12 PM on January 25, 2023:

It should be possible to configure enabling the ctime tests even when valgrind is not available (specifically, for building with msan).

hebasto commented at 7:28 PM on January 25, 2023:

The code defines the default value only. Consider:

$ cmake -S . -B build -DSECP256K1_VALGRIND=OFF -DSECP256K1_BUILD_CTIME_TESTS=ON
...
secp256k1 configure summary
===========================
...
  ctime_tests ......................... ON
...
Valgrind .............................. OFF

sipa commented at 7:29 PM on January 25, 2023:

Apologies, I should have tried!

hebasto force-pushed on Jan 25, 2023

hebasto commented at 9:42 PM on January 25, 2023: member

Updated 1b0e0ba27a57cf74fdd0f3c06e1441c44119883d -> 56678206f179046c3fc95569d08037127b03eac4 (pr1113.33 -> pr1113.35, diff):

addressed @sipa's comment
minor improvements

hebasto commented at 9:50 PM on January 25, 2023: member

A question for discussion:

Can the default value of the SECP256K1_TEST_OVERRIDE_WIDE_MULTIPLY option, which is "AUTO", be replaced with "OFF"? It would mean "Override is OFF. i.e, no override is applied".

If answer is "yes", it would simplify code a bit.

real-or-random commented at 10:12 AM on January 26, 2023: contributor

Can the default value of the SECP256K1_TEST_OVERRIDE_WIDE_MULTIPLY option, which is "AUTO", be replaced with "OFF"? It would mean "Override is OFF. i.e, no override is applied".

Sounds reasonable.

hebasto force-pushed on Jan 26, 2023

hebasto commented at 10:54 AM on January 26, 2023: member

Can the default value of the SECP256K1_TEST_OVERRIDE_WIDE_MULTIPLY option, which is "AUTO", be replaced with "OFF"? It would mean "Override is OFF. i.e, no override is applied".

Sounds reasonable.

Thanks! Implemented.

hebasto force-pushed on Jan 26, 2023

real-or-random commented at 11:23 AM on January 26, 2023: contributor

Note: I'm pretty busy this week and the next week, but I will certainly come back to this afterward. (Of course, feel free to merge this without me if you feel it's ready!)

hebasto commented at 3:07 PM on January 26, 2023: member

An example of the installed library integration into another project has been added to the PR description:

find_package(secp256k1 0.2.1 EXACT REQUIRED CONFIG)

hebasto commented at 10:52 AM on January 27, 2023: member

Another question to discuss:

Currently, the config files, which are used by other projects to integrated secp256k1 library, are named as follows:

$ ls -1 lib/cmake/secp256k1/
secp256k1Config.cmake
secp256k1ConfigVersion.cmake
secp256k1Targets.cmake
secp256k1Targets-release.cmake

Considering that the project name is lowercase only, i.e., secp256k1, would it better to stick to lowercase for config files as well? Such a case is well documented by CMake. That means config file names would be as follows:

secp256k1-config.cmake
secp256k1-config-version.cmake
secp256k1-targets.cmake
secp256k1-targets-release.cmake  # or secp256k1-targets-debug.cmake

real-or-random commented at 12:48 PM on January 27, 2023: contributor

I guess, lowercase looks more natural to me for a C library, but I don't think I have a real preference here.

hebasto force-pushed on Jan 27, 2023

hebasto commented at 3:04 PM on January 27, 2023: member

Updated b90a743f0ce937fc1cf843017e68070ef5db8a07 -> 2540c6e055278e21e6d944725dbac52da2cb3faf (pr1113.36 -> pr1113.37, diff):

config files are lowercase now
minor improvements of installation code

real-or-random commented at 10:26 PM on January 27, 2023: contributor

At the moment, automake will add -DENABLE_MODULE_EXTRAKEYS=1 and cmake will add -DENABLE_MODULE_EXTRAKEYS (and similarly for similar variables). Both definitions work fine with our current source, but we should probably make sure that the definitions are equivalent to avoid subtle issues. I don't think I have a strong preference. I think the simplest fix is to make cmake define those variables to be explicitly 1.

hebasto force-pushed on Jan 28, 2023

hebasto commented at 9:06 AM on January 28, 2023: member

Updated 2540c6e055278e21e6d944725dbac52da2cb3faf -> 0f996c7ee0fcf19121f68db2b46cbe2fb043a390 (pr1113.37 -> pr1113.38, diff):

addressed @real-or-random's comment
fixed the make test target when ctime_tests was compiled with Valgrind support

hebasto cross-referenced this on Jan 28, 2023 from issue Add x-only ECDH support to ecdh module by sipa

real-or-random commented at 2:27 PM on January 30, 2023: contributor

Can we add a make check that does the same as make test? I think it will be confusing to switch between system and having to remember whether it's test or check.

Maybe this does the job. I haven't tried it.

  add_custom_target(check COMMAND ${CMAKE_CTEST_COMMAND})

Also, is it possible to exclude ctime_tests from make test? They're not run by autotool's make check. (See #723 for background.)

real-or-random commented at 4:19 PM on January 30, 2023: contributor

We discussed in IRC that we want to mark CMake experimental in the README until we have full docs of all the options plus CI parity (i.e, we test roughly the same amount of configurations through CI for CMake as for Autotools). Both should be done after this PR.

hebasto force-pushed on Jan 30, 2023

hebasto commented at 11:35 PM on January 30, 2023: member

Updated 0f996c7ee0fcf19121f68db2b46cbe2fb043a390 -> 3b32cc5928f68429af3d3a5b1a985d7d88597e18 (pr1113.38 -> pr1113.39):

rebased
addressed @real-or-random's comment:

Can we add a make check that does the same as make test?

Also, is it possible to exclude ctime_tests from make test?

addressed comments from today's IRC meeting:

mark CMake experimental in the README until we have full docs of all the options plus CI parity

fixed linking of exhaustive_tests
a few minor improvements

A note: linking of bench and ctime_tests binaries has been implemented wrt #1203.

hebasto force-pushed on Jan 30, 2023

hebasto cross-referenced this on Jan 31, 2023 from issue [POC] cmake: Add option to run `include-what-you-use` with compiler by hebasto

hebasto commented at 9:08 AM on February 7, 2023: member

Anything to add / remove / amend? :)

hebasto force-pushed on Feb 7, 2023

hebasto commented at 11:49 AM on February 7, 2023: member

Updated c90ee43658fa61a420ed14bd803a026d79bca4b7 -> 6ec8d6f4c2b24568e4d132d428900d837d402cbc (pr1113.40 -> pr1113.41):

rebased on top of the #1206
mirrored changes from #1206

hebasto cross-referenced this on Feb 14, 2023 from issue Introduce `clang-tidy` and optimize code by hebasto

real-or-random referenced this in commit cbd2555934 on Feb 21, 2023

hebasto force-pushed on Feb 22, 2023

hebasto commented at 9:27 AM on February 22, 2023: member

Updated 6ec8d6f4c2b24568e4d132d428900d837d402cbc -> 812cc7e248074f568da85306395ce8a423867885 (pr1113.41 -> pr1113.42):

rebased on top of the #1209
mirrored changes from #1209

Particularly, the following commands won't fire LNK4217 warnings on Windows:

>cmake -G "Visual Studio 17 2022" -A x64 -S . -B build -DSECP256K1_BUILD_SHARED=OFF -DSECP256K1_BUILD_EXAMPLES=ON
>cmake --build build --config Release

hebasto cross-referenced this on Feb 22, 2023 from issue build: Add SECP256K1_API_VAR to fix importing variables from DLLs by real-or-random

mattiaferrari02 cross-referenced this on Feb 27, 2023 from issue Add functions implementations by mattiaferrari02

hebasto cross-referenced this on Feb 28, 2023 from issue build: Add CMake-based build system (3 of N) by hebasto

in CMakeLists.txt:13 in 776ef08f1e outdated

   8 | +endif()
   9 | +
  10 | +# The package (a.k.a. release) version is based on semantic versioning 2.0.0 of
  11 | +# the API. All changes in experimental modules are treated as
  12 | +# backwards-compatible and therefore at most increase the minor version.
  13 | +project(secp256k1 VERSION 0.2.1 LANGUAGES NONE)

real-or-random commented at 10:05 AM on March 3, 2023:

nit: Is there a reason why you set LANGUAGES NONE instead of C and use enable_language(C) below?

theuni commented at 5:10 PM on March 6, 2023:

I'd like to know this as well.

hebasto commented at 10:24 PM on March 6, 2023:

Updated.

in CMakeLists.txt:27 in 776ef08f1e outdated

  22 | +set(${PROJECT_NAME}_LIB_VERSION_AGE 0)
  23 | +
  24 | +enable_language(C)
  25 | +set(CMAKE_C_STANDARD 90)
  26 | +set(CMAKE_C_EXTENSIONS OFF)
  27 | +set(CMAKE_POSITION_INDEPENDENT_CODE ON)

real-or-random commented at 10:17 AM on March 3, 2023:

Is there a reason not rely to the default here? AFAIU we also don't set this explicitly in the autotools build.

theuni commented at 4:16 PM on March 3, 2023:

Agree. Given the build targets, this seems like one of the teeny tiny subset of projects that may actually care. Like @real-or-random says here: #1113 (review) , I think we should probably skip this for now and discuss it in a (likely one of many of such) follow-up PR.

FWIW downstream Core sets this, so there's some precedent at least: https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L1992

real-or-random commented at 4:25 PM on March 3, 2023:

@theuni (Ignoring that this discussion may be long to a separate PR), what's the stories of enabling PIC for non-shared libraries? Are there any advantages or disadvantages? Why does Core set this for libsecp256k1? (This goes back to 2014 https://github.com/bitcoin/bitcoin/commit/07a99017033b23f840f602d768efa87e0e914e90 ) Should we enable this in our test and example binaries? (I guess that doesn't matter at all.)

theuni commented at 4:35 PM on March 3, 2023:

@real-or-random Core builds a libsecp and links it into a shared libconsensus/libkernel, as well as hardened (-pie) binaries. For that reason, we always want a pic libsecp, even though we're building it statically.

For some platforms (x86 for ex) there may be a significant overhead, but we accept it because of the hardening (aslr) that comes with it. Some projects on niche and/or resource-starved architectures may not be so willing to accept the overhead.

theuni commented at 5:16 PM on March 6, 2023:

Just revisiting this to confirm...

From POSITION_INDEPENDENT_CODE's docs

This property is True by default for SHARED and MODULE library targets and False otherwise. This property is initialized by the value of the CMAKE_POSITION_INDEPENDENT_CODE variable if it is set when a target is created.

This matches the current libtool behavior, so leaving it alone seems correct to me. Core should be able to set it at the top level in order to get PIC in our static libsecp.

hebasto commented at 10:25 PM on March 6, 2023:

Thanks! Updated.

in CMakeLists.txt:29 in 776ef08f1e outdated

  26 | +set(CMAKE_C_EXTENSIONS OFF)
  27 | +set(CMAKE_POSITION_INDEPENDENT_CODE ON)
  28 | +
  29 | +list(APPEND CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake)
  30 | +
  31 | +# We do not use CMake's BUILD_SHARED_LIBS option.

real-or-random commented at 10:18 AM on March 3, 2023:

The reasoning should be documented.

theuni commented at 4:17 PM on March 3, 2023:

Yes, I'm curious to know why :)

hebasto commented at 12:53 PM on March 6, 2023:

BUILD_SHARED_LIBS is a global flag. I can imagine that a top project, which includes secp256k1 as a subproject, can use it on its own purpose.

real-or-random commented at 2:07 PM on March 6, 2023:

That's a good reason, yes.

theuni commented at 5:10 PM on March 6, 2023:

Thanks. Yes, it does seem that this is the convention.

I wonder if the convention will begin change down the road after the introduction of PROJECT_IS_TOP_LEVEL .

real-or-random commented at 8:54 AM on March 7, 2023:

I wonder if the convention will begin change down the road after the introduction of PROJECT_IS_TOP_LEVEL .

AFAIU from the docs, PROJECT_IS_TOP_LEVEL will not help here. Since we have our own project() call, PROJECT_IS_TOP_LEVEL will still be true for libsecp256k1 in its root directory, even if it's included from another CMakeLists.txt.

in CMakeLists.txt:118 in 776ef08f1e outdated

 115 | +set(SECP256K1_VALGRIND "AUTO" CACHE STRING "Build with extra checks for running inside Valgrind. [default=AUTO]")
 116 | +set_property(CACHE SECP256K1_VALGRIND PROPERTY STRINGS "AUTO" "OFF" "ON")
 117 | +check_string_option_value(SECP256K1_VALGRIND)
 118 | +if(SECP256K1_VALGRIND)
 119 | +  find_package(Valgrind MODULE)
 120 | +  if(Valgrind_FOUND)

real-or-random commented at 10:22 AM on March 3, 2023:

nit: Is there a reason why you use a different capitalization for valgrind sometimes?

hebasto commented at 1:09 PM on March 6, 2023:

SECP256K1_VALGRIND is a full capitalized user-configurable option.
Our package search module is called FindValgrind.cmake which follows the pattern Find<PackageName>.cmake. This assumes a capitalized package name Valgrind, which, in turn, is used as a part of standard variable names.
valgrind_brew_prefix is a lowercase local variable.

real-or-random commented at 2:07 PM on March 6, 2023:

2. Our package search module is called FindValgrind.cmake which follows the pattern Find<PackageName>.cmake. This assumes a capitalized package name Valgrind, which, in turn, is used as a part of standard variable names.

That's the part I was missing. Thanks, makes a lot of sense.

in CMakeLists.txt:206 in 776ef08f1e outdated

 201 | +  # Honor visibility properties for all target types.
 202 | +  # See: https://cmake.org/cmake/help/latest/policy/CMP0063.html
 203 | +  cmake_policy(SET CMP0063 NEW)
 204 | +endif()
 205 | +set(CMAKE_C_VISIBILITY_PRESET hidden)
 206 | +set(CMAKE_VISIBILITY_INLINES_HIDDEN TRUE)

real-or-random commented at 10:26 AM on March 3, 2023:

Here we deviate from autotools. It may indeed be a good idea to set -fvisibility-inlines-hidden, also in autotools, but I think this should be the concern of a different PR.

I think it's a good idea to change this in lockstep in both build systems to keep the differences between them small.

hebasto commented at 10:26 PM on March 6, 2023:

Thanks! Updated.

in CMakeLists.txt:182 in 776ef08f1e outdated

 177 | +  set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
 178 | +  try_add_compile_option(/W3)
 179 | +  try_add_compile_option(/wd4146)
 180 | +  try_add_compile_option(/wd4244)
 181 | +  try_add_compile_option(/wd4267)
 182 | +  try_add_compile_option(/wd4334)

real-or-random commented at 10:29 AM on March 3, 2023:

Same here, I think these are indeed better warning options than in configure.ac (awesome that you took the time to figure them out!), but this PR should keep them in sync with configure.ac.

theuni commented at 4:21 PM on March 3, 2023:

After seeing a few of these very reasonable differences, I think a small follow-up TODO list would be really helpful. There are plenty of things that should be addressed after merge, but imo matching behaviors should generally be the goal of the initial pr.

hebasto commented at 10:26 PM on March 6, 2023:

Updated.

in README.md:85 in 812cc7e248 outdated

  80 | +
  81 | +    $ mkdir build && cd build
  82 | +    $ cmake ..
  83 | +    $ make
  84 | +    $ make check  # run the test suite
  85 | +    $ sudo make install  # optional

real-or-random commented at 10:53 AM on March 3, 2023:

This obviously works, but I think the proper cmake way of doing this is like this:

    $ cmake --build .
    $ cmake --build . --target check  # run the test suite
    $ cmake --build . --target install  # optional

Then this works with every cmake backend, not only GNU make (as you describe below for windows). E.g. install ninja and try cmake -G ninja .. and then the above.

See https://cmake.org/cmake/help/latest/guide/tutorial/A%20Basic%20Starting%20Point.html and "Invoking make" here: https://web.archive.org/web/20190722182127/http://voices.canonical.com/jussi.pakkanen/2013/03/26/a-list-of-common-cmake-antipatterns/

hebasto commented at 2:11 PM on March 6, 2023:

My intention was to demonstrate 100% compatibility of the build/install steps with the Autotools based build system. If anyone uses them in their scripts, only the configuration step needs to be updated.

real-or-random commented at 8:24 PM on March 6, 2023:

Ok, yes, I don't have a strong preference here, and there are good arguments for either side. Please don't hesitate to omit my corresponding commit, and I'm happy to put this on the "maybe later" list.

hebasto commented at 10:27 PM on March 6, 2023:

Please don't hesitate to omit my corresponding commit

Thanks! Omitted :)

in cmake/FindValgrind.cmake:13 in 812cc7e248 outdated

   8 | +  )
   9 | +endif()
  10 | +
  11 | +find_path(Valgrind_INCLUDE_DIR
  12 | +  NAMES valgrind/memcheck.h
  13 | +  HINTS ${valgrind_brew_prefix}/include

real-or-random commented at 10:55 AM on March 3, 2023:

  HINTS $<valgrind_brew_prefix,{valgrind_brew_prefix}/include>$

I need this to get rid of a warning when I run cmake with the lint --warn-uninitialized.

hebasto commented at 2:39 PM on March 6, 2023:

Generator expressions do not work in this context. One could easily observe actual paths using --debug-find option.

real-or-random commented at 8:25 PM on March 6, 2023:

I see, sorry. Not sure if there is another good/obvious way of getting rid of the warning. If not, let's not worry.

hebasto commented at 10:29 PM on March 6, 2023:

A working implementation has been suggested.

in CMakeLists.txt:147 in 812cc7e248 outdated

 138 | +if(MSVC)
 139 | +  string(REPLACE "/DNDEBUG" "" CMAKE_C_FLAGS_RELEASE ${CMAKE_C_FLAGS_RELEASE})
 140 | +else()
 141 | +  set(CMAKE_C_FLAGS_DEBUG "-O0 -g3")
 142 | +  set(CMAKE_C_FLAGS_RELEASE "-O2 -g")
 143 | +endif()

real-or-random commented at 1:18 PM on March 3, 2023:

I'm still not entirely convinced about this.

I think users familiar with CMake have an assumption that predefined targets do more or less what they're supposed to do. So, if we want debugging output by debug we should rather set our default target to RelWithDebugInfo.
CMake may have initialized the flags variables already, so we should probably not set it, but merely to replace them carefully. For example, CMake supports countless compilers and for some -O2 is not a valid option at all.

I suggest this:

# Redefine configuration flags.
if(MSVC)
  string(REPLACE " /DNDEBUG "  " " CMAKE_C_FLAGS_RELEASE " ${CMAKE_C_FLAGS_RELEASE} ")
  string(REPLACE " /DNDEBUG "  " " CMAKE_C_FLAGS_RELWITHDEBINFO " ${CMAKE_C_FLAGS_RELWITHDEBINFO} ")
  string(REPLACE " /DNDEBUG "  " " CMAKE_C_FLAGS_MINSIZEREL " ${CMAKE_C_FLAGS_MINSIZEREL} ")
else()
  string(REPLACE " -DNDEBUG "  " " CMAKE_C_FLAGS_RELEASE " ${CMAKE_C_FLAGS_RELEASE} ")
  string(REPLACE " -DNDEBUG "  " " CMAKE_C_FLAGS_RELWITHDEBINFO " ${CMAKE_C_FLAGS_RELWITHDEBINFO} ")
  string(REPLACE " -DNDEBUG "  " " CMAKE_C_FLAGS_MINSIZEREL " ${CMAKE_C_FLAGS_MINSIZEREL} ")
  # Prefer -O2 optimization level
  string(REPLACE " -O3 "  " -O2 " CMAKE_C_FLAGS_RELEASE " ${CMAKE_C_FLAGS_RELEASE} ")
endif()

Notes:

I tried to add some spacing to make sure we replace -DNDEBUG but not -DNDEBUGGGG.
As an even less aggressive variant, we could strip -DNDEBUG only in the examples directory... But not sure. That may be overkill. Stripping it everywhere is certainly okay and makes the compiler invocation a bit cleaner for non-exotic compilers. What do you think?
We could actually guard the replacing of O2 by a check for gcc/clang, but I'm not convinced. We don't do this in autotools either...
setting is totally fine for the coverage build below. This is a developer thing anyway.

theuni commented at 7:17 PM on March 3, 2023:

I've been hacking around and the above is basically what I came up with as well. It's a shame we can't do make-style variable expansion like:

string(REPLACE -DNDEBUG "" CMAKE_${LANG}_FLAGS_${CONFIG} "${CMAKE_${LANG}_FLAGS_${CONFIG}_INIT}")

real-or-random commented at 8:36 PM on March 4, 2023:

It's a shame we can't do make-style variable expansion like:
string(REPLACE -DNDEBUG "" CMAKE_${LANG}_FLAGS_${CONFIG} "${CMAKE_${LANG}_FLAGS_${CONFIG}_INIT}")

Thinking about this more, this can't work here. At the risk of explaining something you may already know (but others here may not): CMake supports not only single-config generators for systems like make, where CMake hardcodes the config in the generated Makefile. It also supports multi-config generators for build systems like Visual Studio, which natively support multiple configs/built types. (Multi-config is used when CMAKE_CONFIGURATION_TYPES is set). In that mode, what should ${CONFIG} be expanded, too? The used configuration is not yet determined when CMake runs and writes the Visual Studio build files. Generator expressions like $<CONFIG> are CMake's mechanism to deal with this, but don't make sense here in this case of setting variables. See also https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#build-configurations.

Okay, now that I wrote that paragraph, 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.

hebasto commented at 10:31 PM on March 6, 2023:

Updated with improved REGEX REPLACE.

in CMakeLists.txt:292 in 812cc7e248 outdated

 282 | +  message(" - LDFLAGS for executables ............ ${CMAKE_EXE_LINKER_FLAGS_DEBUG}")
 283 | +  message(" - LDFLAGS for shared libraries ....... ${CMAKE_SHARED_LINKER_FLAGS_DEBUG}")
 284 | +  message("Release configuration:")
 285 | +  message(" - CFLAGS ............................. ${CMAKE_C_FLAGS_RELEASE}")
 286 | +  message(" - LDFLAGS for executables ............ ${CMAKE_EXE_LINKER_FLAGS_RELEASE}")
 287 | +  message(" - LDFLAGS for shared libraries ....... ${CMAKE_SHARED_LINKER_FLAGS_RELEASE}")

real-or-random commented at 1:30 PM on March 3, 2023:

  message("RelWithDebInfo configuration:")
  message(" - CFLAGS ............................. ${CMAKE_C_FLAGS_RELWITHDEBINFO}")
  message(" - LDFLAGS for executables ............ ${CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO}")
  message(" - LDFLAGS for shared libraries ....... ${CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO}")

I don't think we'll typically -O0, so let's drop the "Debug" output here entirely

Anyway, this is only for multi-config generators like -G "Ninja Multi-Config"...

hebasto commented at 10:38 PM on March 6, 2023:

As configuration is not defined at the end of the configuring for multi-config generators, it seems important to present to a user flags for often used ones (output for all configurations could be noisy). I left RelWithDebInfo and Release. Although, when using MSVC, the default configuration is "Debug".

real-or-random commented at 12:56 PM on March 7, 2023:

Although, when using MSVC, the default configuration is "Debug".

Oh indeed, and we can't change this right now: https://gitlab.kitware.com/cmake/cmake/-/issues/20820

I'm fine with either way, but you bring up an excellent point and printing RelWithDebInfo and Debug is probably slightly better than what we do now. Outputting all would also make sense, but yes, this is perhaps too noisy.

hebasto commented at 1:57 PM on March 7, 2023:

... printing RelWithDebInfo and Debug is probably slightly better than what we do now.

Done in #1113 (comment).

in CMakeLists.txt:276 in 812cc7e248 outdated

 266 | +message("C compiler ............................ ${CMAKE_C_COMPILER}")
 267 | +message("CFLAGS ................................ ${CMAKE_C_FLAGS}")
 268 | +get_directory_property(compile_options COMPILE_OPTIONS)
 269 | +string(REPLACE ";" " " compile_options "${compile_options}")
 270 | +message("Compile options ....................... " ${compile_options})
 271 | +if(DEFINED CMAKE_BUILD_TYPE)

real-or-random commented at 1:33 PM on March 3, 2023:

Maybe add a note that this can be replaced by NOT GENERATOR_IS_MULTI_CONFIG (https://cmake.org/cmake/help/latest/prop_gbl/GENERATOR_IS_MULTI_CONFIG.html) once we can require cmake 3.9

in CMakeLists.txt:169 in 812cc7e248 outdated

 164 | +if(cached_cmake_build_type)
 165 | +  set_property(CACHE CMAKE_BUILD_TYPE PROPERTY
 166 | +    STRINGS "Debug" "Release" "MinSizeRel" "RelWithDebInfo" "Coverage"
 167 | +  )
 168 | +endif()
 169 | +set(default_build_type "Release")

real-or-random commented at 1:34 PM on March 3, 2023:

set(default_build_type "RelWithDebInfo")

hebasto commented at 10:40 PM on March 6, 2023:

Updated.

in CMakeLists.txt:214 in 812cc7e248 outdated

 209 | +if(SECP256K1_BUILD_BENCHMARK OR SECP256K1_BUILD_TESTS OR SECP256K1_BUILD_EXHAUSTIVE_TESTS OR SECP256K1_BUILD_CTIME_TESTS OR SECP256K1_BUILD_EXAMPLES)
 210 | +  # We do not use CMake CTest's BUILD_TESTING option.
 211 | +  mark_as_advanced(BUILD_TESTING)
 212 | +  enable_testing()
 213 | +  add_custom_target(check
 214 | +    COMMAND ${CMAKE_CTEST_COMMAND} -C $<$<CONFIG:Release>:Release>$<$<CONFIG:Debug>:Debug>

real-or-random commented at 1:53 PM on March 3, 2023:

    COMMAND ${CMAKE_CTEST_COMMAND} $<$<CONFIG:RelWithDebInfo>,-C RelWithDebInfo>

I think just testing RelWithDebInfo is good enough. (As I said, we use -O0 really rarely).

This should probably check if RelWithDebInfo is active. If yes, set -C RelWithDebInfo to have a sane default. Otherwise, we can simply omit -C entirely because it's not clear what the user wants.

in .cirrus.yml:403 in 812cc7e248 outdated

 398 | +  configure_script:
 399 | +    - '%x64_NATIVE_TOOLS%'
 400 | +    - cmake -G "Visual Studio 17 2022" -A x64 -S . -B build -DSECP256K1_ENABLE_MODULE_RECOVERY=ON -DSECP256K1_BUILD_EXAMPLES=ON
 401 | +  build_script:
 402 | +    - '%x64_NATIVE_TOOLS%'
 403 | +    - cmake --build build --config Release -j 5 -- /p:CL_MPcount=5

real-or-random commented at 2:10 PM on March 3, 2023:

    - cmake --build build --config RelWithDebInfo -j 5 -- /p:CL_MPcount=5

hebasto commented at 10:40 PM on March 6, 2023:

Updated.

in src/CMakeLists.txt:87 in 812cc7e248 outdated

  80 | +  add_test(noverify_tests noverify_tests)
  81 | +  if(NOT CMAKE_BUILD_TYPE STREQUAL "Coverage")
  82 | +    add_executable(tests tests.c ${internal_obj})
  83 | +    target_link_libraries(tests binary_interface)
  84 | +    target_compile_definitions(tests PRIVATE VERIFY)
  85 | +    add_test(tests tests)

real-or-random commented at 2:28 PM on March 3, 2023:

This violates the advice in https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#build-configurations and should probably use generator expressions to check for DEBUG:Coverage, at least for adding VERIFY in target_compile_definitions(tests PRIVATE VERIFY).

For actually adding the tests, we can add_test(NAME tests COMMAND tests CONFIGURATIONS RelWithDebInfo Release MinSizeRel Debug to exclude Coverage.

Maybe then just drop the if check? Running add_executable is not too bad, I guess.

Same below for exhaustive tests.

hebasto commented at 10:41 PM on March 6, 2023:

Thanks! Updated.

in CMakeLists.txt:166 in 812cc7e248 outdated

 161 | +  CMAKE_SHARED_LINKER_FLAGS_COVERAGE
 162 | +)
 163 | +get_property(cached_cmake_build_type CACHE CMAKE_BUILD_TYPE PROPERTY TYPE)
 164 | +if(cached_cmake_build_type)
 165 | +  set_property(CACHE CMAKE_BUILD_TYPE PROPERTY
 166 | +    STRINGS "Debug" "Release" "MinSizeRel" "RelWithDebInfo" "Coverage"

real-or-random commented at 2:30 PM on March 3, 2023:

    STRINGS "RelWithDebInfo" "Release" "MinSizeRel" "Debug" "Coverage"

(just a more meaningful order with the default on the left)

I think we should CMAKE_CONFIGURATION_TYPES to the same values (incl. Coverage), in that order. I've seen instances in the CMake manual where the first value of CMAKE_CONFIGURATION_TYPES is taken as default. (Hin:t: This variable expects that types are separated by ;)

hebasto commented at 10:42 PM on March 6, 2023:

Updated.

real-or-random commented at 2:35 PM on March 3, 2023: contributor

I again have a bunch of comments, which look like the PR is not mature, but the opposite is the case. These are really "advanced" comments and corner cases, and none of the comments are particularly important.

I'd really like to see this merged within the next days, with or without these comments addresses. This is just an experimental build system. If a build fails, nothing bad happens and we still have the other system. But having this in the next release will be wonderful to get more feedback from using this in the wild.

real-or-random commented at 4:29 PM on March 3, 2023: contributor

edit: moved to #1224

theuni commented at 7:23 PM on March 3, 2023: contributor

@real-or-random Very helpful, thanks.

Since you probably didn't see over where we're reviewing, here's the current approach we're considering for the CMake switchover in Core, in case it's helpful for you here: https://github.com/hebasto/bitcoin/pull/7#discussion_r1120165874

tl;dr: We're starting by self-building libsecp (in addition to leveldb/crc32c) during the initial CMake merge, in order to keep the review as simple as possible. We'll then switch over to invoking the downstream libsecp CMake once it's merged and ready. That way there's no pressure in either direction and both steps are simpler.

real-or-random commented at 8:07 PM on March 4, 2023: contributor

I pushed a bunch of fixup commits here: https://github.com/real-or-random/secp256k1/tree/220628-cmake-tim These address all of my comments (except #1113 (review), but that's rather a question than a comment :) ).

From my side, this is ready to merge after these fixes

I think some of this could still be simplified and made a bit more idiomatic, but this should be done after merging this. In particular, making things more cmake-ish will mean making them less autotools-ish. And this will only increase the differences between the two build systems, which isn't a good idea for the initial PR.

in src/CMakeLists.txt:28 in 776ef08f1e outdated

  23 | +)
  24 | +target_include_directories(${PROJECT_NAME} INTERFACE
  25 | +  $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
  26 | +)
  27 | +target_compile_definitions(${PROJECT_NAME} PRIVATE
  28 | +  $<$<PLATFORM_ID:Windows>:DLL_EXPORT>

theuni commented at 5:29 PM on March 6, 2023:

~Why only Windows?~ Edit: nevermind, matches header behavior.

Also, as a follow-up, we should change this define to SECP256k1_DLL_EXPORT or so, to get away from the libtool naming confusion once and for all.

real-or-random commented at 8:21 PM on March 6, 2023:

Also, as a follow-up, we should change this define to SECP256k1_DLL_EXPORT or so, to get away from the libtool naming confusion once and for all.

added to the list

theuni commented at 5:33 PM on March 6, 2023: contributor

Looking good to me % the outstanding comments and a few new ones.

A request: Would you mind doing a preview of this plugged into Core and replacing our initial internal implementation? I'd like to have at least some confidence that downstream can still build correctly.

hebasto force-pushed on Mar 6, 2023

hebasto commented at 10:16 PM on March 6, 2023: member

@real-or-random @theuni

Thank you so much for thorough reviewing!

Updated 812cc7e248074f568da85306395ce8a423867885 -> f45c4fb921efc310f8a4c3bee4253c19ac997d55 (pr1113.42 -> pr1113.43).

Implemented changes suggested by @real-or-random in his branch with a few exceptions:

https://github.com/real-or-random/secp256k1/commit/090e9acd629ddfe20d6ef80a243e76ccf8b2acbc not implemented as no justification provided (sorry if I missed it)
documentation remains unchanged, see this discussion
https://github.com/real-or-random/secp256k1/commit/d709495e56e2859f2b925b5b3c798c75ecec254e rewritten correctly
https://github.com/real-or-random/secp256k1/commit/772894d44a365026ccec47364fcd03a9ff13d0e0 become more robust
https://github.com/real-or-random/secp256k1/commit/aa97fbd75e76e028775a1ce878190eea325890db not implemented because it can not defined both CMAKE_BUILD_TYPE and CMAKE_CONFIGURATION_TYPES

Fixed a bug in FindValgrind.cmake -- s/APPLE/CMAKE_HOST_SYSTEM_NAME STREQUAL "Darwin"/

hebasto force-pushed on Mar 6, 2023

real-or-random commented at 10:36 PM on March 6, 2023: contributor

real-or-random@090e9ac not implemented as no justification provided (sorry if I missed it)

Oh yes, I didn't comment here. My rationale for removing this was that it's another difference to autotools. Or does autotools set /MT or /MD? If no, I guess we should keep the defaults here and put it on the list of things to care about later.

hebasto commented at 11:11 PM on March 6, 2023: member

real-or-random@090e9ac not implemented as no justification provided (sorry if I missed it)

Oh yes, I didn't comment here. My rationale for removing this was that it's another difference to autotools. Or does autotools set /MT or /MD?

No, it doesn't.

If no, I guess we should keep the defaults here and put it on the list of things to care about later.

Otherwise, CMake will use its default value. My intention is to make choosing a MSVC runtime library explicit. Even, if we prefer the default value, which is not the case now.

hebasto commented at 11:32 PM on March 6, 2023: member

@theuni

A request: Would you mind doing a preview of this plugged into Core and replacing our initial internal implementation? I'd like to have at least some confidence that downstream can still build correctly.

Going to work on it tomorrow.

real-or-random commented at 11:28 AM on March 7, 2023: contributor

Otherwise, CMake will use its default value. My intention is to make choosing a MSVC runtime library explicit. Even, if we prefer the default value, which is not the case now.

I'm not sure if making it explicit is better. Defaults exists for a reason, and we shouldn't try to be more clever unless we have a good reason to believe that we're more clever.

After having thought about this more, I think the primary concern is that users should be able to choose CMAKE_MSVC_RUNTIME_LIBRARY. Since we don't distribute binaries, the actual value of CMAKE_MSVC_RUNTIME_LIBRARY that we set doesn't matter that much. But it seems that there's no value that is optimal in all cases, so users should be able to ask for their preference.

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 https://github.com/jedisct1/libsodium/discussions/1215 ), and I guess there's a reason why the CMake default is MultiThreadedDLL (where the DLL part corresponds to /MD)

That means I suggest we should do the following:

Respect whatever the user passes for CMAKE_MSVC_RUNTIME_LIBRARY on the command line, and probably even make it a CACHE variable.
If the user doesn't pass anything, do one of the following, in descending personal preference: Stick with CMake default (i.e., don't set anything) or set CMake's /MD default explicitly. (Both options are effectively the same until CMake changes its defaults, which is unlikely...)

Does this make sense to you?

hebasto force-pushed on Mar 7, 2023

hebasto commented at 12:27 PM on March 7, 2023: member

Updated 8793b1b3e48116ddba48c6fec5ef68997466bfdd -> b47a5e3e9defcd0caffdd44c0c5b0c56e6596466 (pr1113.44 -> pr1113.45, diff):

applied https://github.com/real-or-random/secp256k1/commit/090e9acd629ddfe20d6ef80a243e76ccf8b2acbc

real-or-random commented at 12:51 PM on March 7, 2023: contributor

applied real-or-random@090e9ac

Ok, this gives the user full control, nice.

in src/CMakeLists.txt:84 in b47a5e3e9d outdated

  79 | +  target_link_libraries(noverify_tests binary_interface)
  80 | +  add_test(noverify_tests noverify_tests)
  81 | +  add_executable(tests tests.c ${internal_obj})
  82 | +  target_compile_definitions(tests PRIVATE VERIFY)
  83 | +  target_link_libraries(tests binary_interface)
  84 | +  add_test(NAME tests COMMAND tests CONFIGURATIONS RelWithDebInfo Release MinSizeRel Debug)

real-or-random commented at 1:28 PM on March 7, 2023:

if(NOT CMAKE_BUILD_TYPE STREQUAL "Coverage")
  add_test(tests tests)
endif()

I think CONFIGURATIONS this was a mistake, sorry. While the docs say that this is supposed to work, tests is then omitted if one simply runs ctest or make check. See also https://cmake.org/pipermail/cmake/2016-September/064229.html where someone reported the same but got no reply, and see src/CTestTestfile.cmake in the build dir to see why this doesn't work.

The above does the right thing for single-config generators. It also adds a meaningless tests binary for Coverage mode on multi-config generators, but I guess we need to live with that for now. (We could address this issue in the source actually by printing out a warning when VERIFY and COVERAGE are enabled at the same time.)

(make check now simply runs ctest. We could switch to the old behavior of running ctest -C ..., which works, but I doubt that will address the root cause. It's a reasonable requirement that just invoking ctest will do the right thing.)

real-or-random commented at 1:28 PM on March 7, 2023: contributor

I was about to ACK this, but my testing found one more issue...

hebasto force-pushed on Mar 7, 2023

hebasto commented at 1:53 PM on March 7, 2023: member

Updated b47a5e3e9defcd0caffdd44c0c5b0c56e6596466 -> 95779fd91f0b59f4affc19012335b6791320719d (pr1113.45 -> pr1113.46, diff):

addressed #1113 (review)
addressed #1113 (review)

real-or-random commented at 2:18 PM on March 7, 2023: contributor

ACK 95779fd91f0b59f4affc19012335b6791320719d diff looks good, and I tested this with different CMake generators and many different settings

All comments have been addressed or are tracked for possible future PRs, and the ones which are tracked should really be dealt with in separate PRs.

real-or-random approved

hebasto commented at 3:29 PM on March 7, 2023: member

@theuni

A request: Would you mind doing a preview of this plugged into Core and replacing our initial internal implementation? I'd like to have at least some confidence that downstream can still build correctly.

Going to work on it tomorrow.

Here is a branch which switches the recent https://github.com/hebasto/bitcoin/pull/10 to this implementation (configure with -DCMAKE_BUILD_TYPE=Debug).

in README.md:91 in 572ef69767 outdated

  86 | +
  87 | +To compile optional modules (such as Schnorr signatures), you need to run `cmake` with additional flags (such as `-DSECP256K1_ENABLE_MODULE_SCHNORRSIG=ON`). Run `cmake .. -LH` to see the full list of available flags.
  88 | +
  89 | +### Cross compiling
  90 | +
  91 | +To alleviate cross compiling, preconfigured toolchain files are available in the `cmake` directory.

sipa commented at 8:42 PM on March 7, 2023:

The verb "alleviate" is strange here. Do you mean something like "To alleviate issues with cross compiling", or "To support cross compiling"?

hebasto commented at 9:10 PM on March 7, 2023:

Thanks! Updated.

in cmake/FindValgrind.cmake:1 in 572ef69767 outdated

   0 | @@ -0,0 +1,41 @@
   1 | +if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Darwin")

sipa commented at 8:44 PM on March 7, 2023:

Support for valgrind through brew was removed in #1152. I think most logic here can be dropped.

hebasto commented at 9:00 PM on March 7, 2023:

#1152 changes CI only.

The build system still has support for valgrind through brew: https://github.com/bitcoin-core/secp256k1/blob/9d1b458d5fb3ea45c1a038aae4be35f828ffed31/configure.ac#L45-L56

hebasto commented at 9:02 PM on March 7, 2023:

I think most logic here can be dropped.

FWIW, I tested valgrind on x86_64 macOS.

If you still thinking that dropping this piece of code is best, let me know.

sipa commented at 9:05 PM on March 7, 2023:

Ah, fair enough - it makes sense to mimic what the existing build system supports.

I do think that we should probably drop support for macos valgrind altogether, as macos is increasing ARM64-only, and valgrind doesn't support macos-ARM64. But that can be done in a separate PR later.

real-or-random commented at 10:46 PM on March 7, 2023:

Ah, fair enough - it makes sense to mimic what the existing build system supports.

Indeed.

I do think that we should probably drop support for macos valgrind altogether, as macos is increasing ARM64-only, and valgrind doesn't support macos-ARM64. But that can be done in a separate PR later.

ARM64 support is on the roadmap here https://github.com/LouisBrunner/valgrind-macos/issues/56... but yep, I added this to the list of things to reconsider after the PR.

in cmake/TryAddCompileOption.cmake:1 in 572ef69767 outdated

   0 | @@ -0,0 +1,23 @@
   1 | +include(CheckCCompilerFlag)

sipa commented at 8:46 PM on March 7, 2023:

@hebasto Did you copy this from somewhere, or did you write it yourself?

hebasto commented at 8:57 PM on March 7, 2023:

The latter.

hebasto commented at 9:06 PM on March 7, 2023:

Also feedback from reviewing of the similar project in Bitcoin Core was used.

hebasto force-pushed on Mar 7, 2023

hebasto commented at 9:09 PM on March 7, 2023: member

Updated 95779fd91f0b59f4affc19012335b6791320719d -> 752ae70e1d77aff39498dcb6e0e7e162c392f6cb (pr1113.46 -> pr1113.47, diff):

addressed @sipa's #1113 (review)

in src/CMakeLists.txt:14 in 752ae70e1d outdated

   9 | +  set(common_obj "$<TARGET_OBJECTS:common>")
  10 | +else()
  11 | +  set(common_obj "")
  12 | +endif()
  13 | +
  14 | +add_library(precomputed OBJECT

sipa commented at 9:16 PM on March 7, 2023:

Is this library used anywhere? I may be misunderstanding the syntax, but it seems its objects are only referenced through ${internal_obj}?

Same for common.

hebasto commented at 9:24 PM on March 7, 2023:

Is this library...

This is not a real library, it is just a collection of object files, which indeed

are only referenced through ${internal_obj}

hebasto commented at 9:26 PM on March 7, 2023:

That is a way on CMake 3.1 to build a shared and a static libraries simultaneously.

UPD. In CMake 3.12+, a more convenient syntax is available.

sipa commented at 9:31 PM on March 7, 2023:

Thanks, I see!

theuni commented at 10:48 PM on March 7, 2023: contributor

Building with: cmake . -B build2/ -DCMAKE_BUILD_TYPE=Debug -DSECP256K1_BUILD_STATIC=0 -DSECP256K1_BUILD_SHARED=1 I get:

/usr/bin/ld: CMakeFiles/precomputed.dir/precomputed_ecmult.c.o: relocation R_X86_64_PC32 against symbol `stderr@@GLIBC_2.2.5' can not be used when making a shared object; recompile with -fPIC

Fixed with:

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index f07855da..31dbb9b3 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -23,2 +23,5 @@ add_library(${PROJECT_NAME} SHARED EXCLUDE_FROM_ALL
 )
+get_target_property(USE_PIC ${PROJECT_NAME} POSITION_INDEPENDENT_CODE)
+set_target_properties(precomputed PROPERTIES POSITION_INDEPENDENT_CODE ${USE_PIC})
+
 target_include_directories(${PROJECT_NAME} INTERFACE

in src/CMakeLists.txt:20 in 752ae70e1d outdated

  15 | +  precomputed_ecmult.c
  16 | +  precomputed_ecmult_gen.c
  17 | +)
  18 | +set(internal_obj "$<TARGET_OBJECTS:precomputed>" "${common_obj}")
  19 | +
  20 | +add_library(${PROJECT_NAME} SHARED EXCLUDE_FROM_ALL

theuni commented at 10:50 PM on March 7, 2023:

This static/shared logic seems needlessly complicated to me, and from what I can tell largely bypasses CMake's typical machinery and customs.

Is the extra work here primarily to support building a shared/static lib at the same time? If so, is that something we really need to carry forward?

real-or-random commented at 10:58 PM on March 7, 2023:

Is the extra work here primarily to support building a shared/static lib at the same time? If so, is that something we really need to carry forward?

I agree. If this is only done to support both types at the same time, this is something we could drop for sure, in particular if it helps with issues such as the PIC issue in the previous comment. No need to mimic that specific ability of autotools. If one needs both, one can invoke the build system twice.

theuni commented at 11:35 PM on March 7, 2023:

The PIC issue above is that the object lib doesn't know whether or not it should be built as PIC, so it defaults to no. For static libs, that's the correct answer. For shared libs, that's wrong.

But since it's only built once no matter what, there's no correct answer to "how should the object lib be built if we're building for both shared and static libs?". The result is the hacky patch above.

So while I appreciate the attempt to do both at once, I think the only real "clean" way is to build objects twice. Which as @real-or-random said, means simply invoking twice.

In case it's helpful, yesterday I pushed a draft CMake port of Minisketch which I believe handles this correctly (though it currently uses BUILD_SHARED_LIBS directly). Notice how it gets/sets the POSITION_INDEPENDENT_CODE properly for the object lib so that it does the right thing whether it's building shared or static.

hebasto commented at 9:37 AM on March 8, 2023:

This static/shared logic seems needlessly complicated to me, and from what I can tell largely bypasses CMake's typical machinery and customs.

Is the extra work here primarily to support building a shared/static lib at the same time?

Yes, it is.

The PIC issue above is that the object lib doesn't know whether or not it should be built as PIC, so it defaults to no. For static libs, that's the correct answer. For shared libs, that's wrong.

That is why there was set(CMAKE_POSITION_INDEPENDENT_CODE ON) initially.

hebasto commented at 9:40 AM on March 8, 2023:

In case it's helpful, yesterday I pushed a draft CMake port of Minisketch...

FWIW, https://github.com/sipa/minisketch/pull/75

hebasto commented at 10:44 AM on March 8, 2023:

So while I appreciate the attempt to do both at once, I think the only real "clean" way is to build objects twice. Which as @real-or-random said, means simply invoking twice.

Agree.

FWIW, here are some related thoughts from one of CMake developers.

real-or-random commented at 10:48 AM on March 8, 2023:

That is why there was set(CMAKE_POSITION_INDEPENDENT_CODE ON) initially.

Makes sense! Sorry, I wasn't aware of all these issues when I suggested dropping that line.

I think for now, we should either

apply the patch suggested by @theuni
or simply set(CMAKE_POSITION_INDEPENDENT_CODE ON) again -- sure it's a deviation from autotools then, but it's justified

If you ask me, I'd probably go with 1, but either is fine.

It's totally reasonable to say that dropping the ability to build shared and static libs at the same time belongs to the category of making things more CMake-ish and is probably better addressed in a separate PR.

The big advantage of getting this PR merged soon that it will be easier to refine separate aspects of the CMake build separately. This PR here is huge already now (with 240 comments and 47 revisions), and discussions will get much easier if they can happen in separate PRs which address only a single concern.

real-or-random commented at 10:49 AM on March 8, 2023:

Oh. you already implemented suggestion 1 while I was writing this. :)

hebasto force-pushed on Mar 8, 2023

hebasto commented at 10:40 AM on March 8, 2023: member

Updated 752ae70e1d77aff39498dcb6e0e7e162c392f6cb -> a846de12bae294f8f2877c585e57708949bec7b4 (pr1113.47 -> pr1113.48, diff):

addressed @theuni's #1113 (comment)

in CMakeLists.txt:150 in a846de12ba outdated

 145 | +  # Prefer -O2 optimization level. (-O3 is CMake's default for Release for many compilers.)
 146 | +  string(REGEX REPLACE "-O3[ \t\r\n]*" "-O2" CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE}")
 147 | +endif()
 148 | +
 149 | +# Define custom "Coverage" build type.
 150 | +set(CMAKE_C_FLAGS_COVERAGE "${CMAKE_C_FLAGS_RELWITHDEBINFO} -DCOVERAGE=1 --coverage -Wno-unused-parameter" CACHE STRING

real-or-random commented at 1:28 PM on March 8, 2023:

set(CMAKE_C_FLAGS_COVERAGE "${CMAKE_C_FLAGS_RELWITHDEBINFO} -O0 -DCOVERAGE=1 --coverage -Wno-unused-parameter" CACHE STRING

hebasto commented at 1:37 PM on March 8, 2023:

I thought that -O0 was skipped for a reason in https://github.com/real-or-random/secp256k1/commit/3fde3022e6247ba07e5e5090834737fd3a13d65e, but forgot to ask about it :)

Updated.

real-or-random commented at 1:48 PM on March 8, 2023:

Yes, but the reason is my stupidity. :)

real-or-random approved

real-or-random commented at 1:28 PM on March 8, 2023: contributor

ACK mod nit

build: Add CMake-based build system

Co-authored-by: Tim Ruffing <crypto@timruffing.de>

5468d70964

cmake: Export config files 10602b0030

ci: Add "x86_64: Windows (VS 2022)" task e1eb33724c

hebasto force-pushed on Mar 8, 2023

hebasto commented at 1:35 PM on March 8, 2023: member

Updated a846de12bae294f8f2877c585e57708949bec7b4 -> e1eb33724c2ca47855a8c1dada421cabdb717fe7 (pr1113.48 -> pr1113.49, diff):

addressed @real-or-random's #1113 (review)

real-or-random approved

real-or-random commented at 1:37 PM on March 8, 2023: contributor

ACK e1eb33724c2ca47855a8c1dada421cabdb717fe7

theuni commented at 2:49 PM on March 8, 2023: contributor

It's totally reasonable to say that dropping the ability to build shared and static libs at the same time belongs to the category of making things more CMake-ish and is probably better addressed in a separate PR.

The big advantage of getting this PR merged soon that it will be easier to refine separate aspects of the CMake build separately. This PR here is huge already now (with 240 comments and 47 revisions), and discussions will get much easier if they can happen in separate PRs which address only a single concern.

Agreed. I definitely think we should make some changes, but agree that the discussion doesn't need to drag out the initial PR.

Since my only remaining concern is the above static/shared lib situation which will be addressed in a follow-up...

ACK e1eb33724c2ca47855a8c1dada421cabdb717fe7.

theuni approved

sipa commented at 3:04 PM on March 8, 2023: contributor

ACK e1eb33724c2ca47855a8c1dada421cabdb717fe7

I've played around with this, am starting to understand the structure/syntax a bit, and the parts I understand look correct. The follow-ups in #1113 (comment) seem good future steps, especially the discussion above makes me think we should just cmake-ify and only support either static or shared builds, if that simplifies some of the hacks needed. Some other follow-ups:

Release note entry
Overall documentation for all configure flags/defines (already useful without cmake, but with cmake the situation does get even more complicated)
Add ability to detect whether arm assembly is available (this is already an issue with configure today; I just noticed looking at cmake that there is no detection for it).

sipa merged this on Mar 8, 2023

sipa closed this on Mar 8, 2023

real-or-random cross-referenced this on Mar 8, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by real-or-random

hebasto deleted the branch on Mar 8, 2023

hebasto cross-referenced this on Mar 8, 2023 from issue release: prepare for 0.3.0 by jonasnick

in README.md:109 in e1eb33724c

 104 | +The following example assumes using of Visual Studio 2022 and CMake v3.21+.
 105 | +
 106 | +In "Developer Command Prompt for VS 2022":
 107 | +
 108 | +    >cmake -G "Visual Studio 17 2022" -A x64 -S . -B build
 109 | +    >cmake --build build --config Release

real-or-random commented at 5:58 PM on March 8, 2023:

Now that I've read this again this in the merged README: :see_no_evil:

    >cmake --build build --config RelWithDebInfo

hebasto commented at 8:02 PM on March 8, 2023: member

@sipa

Add ability to detect whether arm assembly is available (this is already an issue with configure today; I just noticed looking at cmake that there is no detection for it).

FWIW, the following command https://github.com/bitcoin-core/secp256k1/blob/ef4f8bd025960b5226f9fddebe1864e4afcb719a/CMakeLists.txt#L90-L91 is supposed to fail when arm assembly is not available.

Additionaly, we can use check_language(ASM) command.

sipa commented at 8:04 PM on March 8, 2023: contributor

@hebasto That can't work, because it isn't testing ARM-specific assembly being available.

E.g. this "works":

$ cmake .. -DSECP256K1_ASM=arm -DSECP256K1_EXPERIMENTAL=ON

on Linux x86_64, but causes build failure.

sipa referenced this in commit 763079a3f1 on Mar 8, 2023

hebasto commented at 11:10 PM on March 8, 2023: member

That can't work, because it isn't testing ARM-specific assembly being available.

Well, it can be like that

if(CMAKE_SYSTEM_PROCESSOR MATCHES "^arm*" AND CMAKE_SIZEOF_VOID_P EQUAL 4)
  include(CheckLanguage)
  check_language(ASM)
endif()

real-or-random cross-referenced this on Mar 10, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by hebasto

theuni cross-referenced this on Mar 10, 2023 from issue cmake: Set `POSITION_INDEPENDENT_CODE` property properly by hebasto

hebasto cross-referenced this on Mar 10, 2023 from issue Build: allow static or shared but not both by theuni

div72 referenced this in commit 945b094575 on Mar 14, 2023

hebasto cross-referenced this on Apr 10, 2023 from issue cmake: Make installation optional by CyberTailor

giahuy98 cross-referenced this on Apr 14, 2023 from issue Fix 64-bit Android ABIs & allow manual ABI selection by PeteClubSeven

hebasto cross-referenced this on Apr 29, 2023 from issue cmake: Use `SECP256K1_COVERAGE` option instead of `CMAKE_BUILD_TYPE=Coverage` by hebasto

hebasto cross-referenced this on Apr 30, 2023 from issue refactor: Make 64-bit shift explicit by hebasto

hebasto cross-referenced this on May 30, 2023 from issue test: Warn if both `VERIFY` and `COVERAGE` are defined by hebasto

real-or-random referenced this in commit d75dc59b58 on May 31, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

real-or-random cross-referenced this on Jun 28, 2023 from issue build: Improvements to symbol visibility logic on Windows by real-or-random

vmta referenced this in commit 8f03457eed on Jul 1, 2023

jonasnick cross-referenced this on Jul 21, 2023 from issue Upstream PRs 1113, 1225, 1227, 1229, 1223 by jonasnick

build: Add CMake-based build system #1113

Autotools -- CMake Feature Parity Tables

1. Configuration options

2. make targets

2. `make` targets