Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add 1:1 CMake File #549

Closed
wants to merge 13 commits into from
Closed

Conversation

DesWurstes
Copy link
Contributor

@DesWurstes DesWurstes commented Aug 12, 2018

Currently lacks of:

  • Module ECDH
  • JNI
    but is very stable.

@DesWurstes DesWurstes force-pushed the master branch 2 times, most recently from e8d90fe to 4cf4e41 Compare August 12, 2018 09:09
@afk11
Copy link
Contributor

afk11 commented Aug 12, 2018

#315 aimed to add this before, and some points were raised about testing, and keeping the configuration in sync with new features. Perhaps you could address these points so others can assess how viable this is?

@gmaxwell
Copy link
Contributor

I'm really so super not enthused by having yet another build system. (and last I checked cmake still didn't support a lot of systems, or cross compiling... though I know its well liked for windows support)

gmaxwell added a commit that referenced this pull request May 23, 2019
… basic config

dbed75d Undefine `STATIC_PRECOMPUTATION` if using the basic config (DesWurstes)
310111e Keep LDFLAGS if `--coverage` (DesWurstes)

Pull request description:

  Update: **This is a trimmed pull request with strong rationale.**

  - Adding `--coverage` shouldn't reset `LDFLAGS`, this is definitely a typo
  - The basic configuration should undefine `STATIC_PRECOMPUTATION`, as generating it is not supported and it complicates #549

Tree-SHA512: 29f0dd4c870ec60d535346446b453da459ca843ed1265c2bc966bf0fcbdf3c5c79f9e48a419662e81d790a7003f8877a16e2a5a74aa5c0b79645e15ad56a0f66
@gmaxwell
Copy link
Contributor

Unfortunately, we lack the resources or interest to adequately maintain another parallel build system on an ongoing basis. To the extent that build-system concerns would be a good use of development time, we believe that they'd better be spent making it easier to build the library without any build system and documenting how that's done. I believe we're likely to close any further cmake or vcproject pull requests unless something changes-- nothing wrong with your efforts and thank you for the attempt, they just don't reflect this libraries focus at this time.

@gmaxwell gmaxwell closed this May 29, 2019
@DesWurstes
Copy link
Contributor Author

Thanks anyway.

@real-or-random real-or-random mentioned this pull request Jun 25, 2020
@xloem
Copy link

xloem commented Apr 10, 2021

Just a comment that cmake appears valuable enough to me to find some way to facilitate collaboration among the people making pull requests.

I see that libsecp256k1 gets a pull request adding cmake about every year or so, so if there were some community-maintained buildfile in a 'contrib' or 'unsupported' folder or somesuch, it might be maintainable simply by those who use it.

I'm working with code that embeds libsecp256k1 using cmake, and when doing so it becomes clear how fragmented maintenance effort around that is.

Maintaining documentation for building libsecp256k1 without autotools would of course also meet a significant portion of this concern.

@real-or-random
Copy link
Contributor

I see that libsecp256k1 gets a pull request adding cmake about every year or so, so if there were some community-maintained buildfile in a 'contrib' or 'unsupported' folder or somesuch, it might be maintainable simply by those who use it.

Hm I'd be skeptical to accept this, because I feel that would still create a maintenance burden for us. Would it be possible to create an external "overlay" that adds CMake support? I admit that's probably not exactly elegant.

Maintaining documentation for building libsecp256k1 without autotools would of course also meet a significant portion of this concern.

Yes, we have #622 for this.

@zone117x
Copy link

zone117x commented Feb 7, 2022

Would be nice to have an open issue for discussing cmake support. I maintain a C# wrapper for this lib and cmake is (for me) the easiest way to build cross-platform binaries. I've recently looked at adding linux-arm64 and Apple Silicon support, and cmake support would have been helpful here as well.

@real-or-random
Copy link
Contributor

Would be nice to have an open issue for discussing cmake support. I maintain a C# wrapper for this lib and cmake is (for me) the easiest way to build cross-platform binaries. I've recently looked at adding linux-arm64 and Apple Silicon support, and cmake support would have been helpful here as well.

autotools have good support for cross-compilation. For example, use ./configure --host=aarch64-linux-gnu for ARM64.

https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.71/html_node/Specifying-Target-Triplets.html#Specifying-Names

@zone117x
Copy link

zone117x commented Feb 8, 2022

I was able to update my cmake setup to build universal binaries for macos (x86 + arm64) with CMAKE_OSX_ARCHITECTURES=arm64;x86_64.

Another useful feature is export symbols in the Windows dll binary with set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON), and it builds on Windows systems with regular cmake+msvc tooling which has some benefits compared to cross-compiling from Linux to Windows.

I'm sure those are both possible using autotools as well but it looks more painful. Cmake seems to have emerged as the de facto cross platform build system.

One thing I had trouble with was trying to rebase my cmake config onto the latest master branch due to the changes around USE_ECMULT_STATIC_PRECOMPUTATION. So the library is still building off a fork from 2018. If this repo had a cmakelists.txt I'm betting it would have been maintained. Maybe the next PR to this repo adding cmake support will figure it out for me :)

@real-or-random
Copy link
Contributor

Unfortunately, we lack the resources or interest to adequately maintain another parallel build system on an ongoing basis.

I think this is still true. Of course, we could switch to CMake entirely, and I believe it's probably the better build system. But even if we have consensus, I'm not sure if it's worth the effort given that autotools work pretty well.

One thing I had trouble with was trying to rebase my cmake config onto the latest master branch due to the changes around USE_ECMULT_STATIC_PRECOMPUTATION. So the library is still building off a fork from 2018. If this repo had a cmakelists.txt I'm betting it would have been maintained. Maybe the next PR to this repo adding cmake support will figure it out for me :)

Any specific questions?

@zone117x
Copy link

zone117x commented Feb 8, 2022

Unfortunately, we lack the resources or interest to adequately maintain another parallel build system on an ongoing basis.

I think this is still true. Of course, we could switch to CMake entirely, and I believe it's probably the better build system. But even if we have consensus, I'm not sure if it's worth the effort given that autotools work pretty well.

Yep fair enough -- makes sense. I do think there's room for including a basic cmakelists.txt that produces a typical output (e.g. only the non-experimental modules, using the pre-generated context), without replicating the various tests and other functionality in autotools. It could use the "experimental" label/docs so folks understand it could in theory build a broken output and not get tested.

Here's the one I'm using https://github.com/MeadowSuite/secp256k1/blob/master/CMakeLists.txt, which seems like it would be even smaller if adjusted to work the latest codebase because the pre-generated context changes, and some of those directives being automatic or removed.

Any specific questions?

It seems like this should work:

cmake_minimum_required(VERSION 3.4)
project(secp256k1 LANGUAGES C)

set(COMMON_COMPILE_FLAGS, ENABLE_MODULE_ECDH, ENABLE_MODULE_RECOVERY, USE_BASIC_CONFIG)
set(COMPILE_OPTIONS -O3 -W -std=c89 -pedantic -Wall -Wextra -Wcast-align -Wnested-externs -Wshadow -Wstrict-prototypes -Wno-unused-function -Wno-long-long -Wno-overlength-strings)

add_library(secp256k1 SHARED src/secp256k1.c)

target_compile_definitions(secp256k1 PRIVATE ${COMMON_COMPILE_FLAGS} ${COMPILE_FLAGS})
target_include_directories(secp256k1 PRIVATE ${CMAKE_SOURCE_DIR} ${CMAKE_SOURCE_DIR}/src)
target_compile_options(secp256k1 PRIVATE ${COMPILE_OPTIONS})

But results in errors from ECMULT_WINDOW_SIZE and ECMULT_GEN_PREC_BITS being undeclared -- shouldn't the USE_BASIC_CONFIG option specify those?

[ 50%] Building C object CMakeFiles/secp256k1.dir/src/secp256k1.c.o
In file included from /Users/matt/Projects/secp256k1/src/secp256k1.c:17:
In file included from /Users/matt/Projects/secp256k1/src/ecmult_impl.h:16:
/Users/matt/Projects/secp256k1/src/ecmult.h:26:4: error: Set ECMULT_WINDOW_SIZE to an integer in range [2..24].
#  error Set ECMULT_WINDOW_SIZE to an integer in range [2..24].

Then if I specify those myself like this:

cmake_minimum_required(VERSION 3.4)
project(secp256k1 LANGUAGES C)

set(COMMON_COMPILE_FLAGS, ENABLE_MODULE_ECDH, ENABLE_MODULE_RECOVERY, USE_BASIC_CONFIG)
set(COMPILE_OPTIONS -O3 -W -std=c89 -pedantic -Wall -Wextra -Wcast-align -Wnested-externs -Wshadow -Wstrict-prototypes -Wno-unused-function -Wno-long-long -Wno-overlength-strings)

add_library(secp256k1 SHARED src/secp256k1.c)

target_compile_definitions(secp256k1 PRIVATE ECMULT_GEN_PREC_BITS=4)
target_compile_definitions(secp256k1 PRIVATE ECMULT_WINDOW_SIZE=15)

target_compile_definitions(secp256k1 PRIVATE ${COMMON_COMPILE_FLAGS} ${COMPILE_FLAGS})
target_include_directories(secp256k1 PRIVATE ${CMAKE_SOURCE_DIR} ${CMAKE_SOURCE_DIR}/src)
target_compile_options(secp256k1 PRIVATE ${COMPILE_OPTIONS})

I get:

[ 50%] Building C object CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[100%] Linking C shared library libsecp256k1.dylib
Undefined symbols for architecture arm64:
  "_secp256k1_ecmult_gen_prec_table", referenced from:
      _secp256k1_ecmult_gen in secp256k1.c.o
  "_secp256k1_pre_g", referenced from:
      _secp256k1_ecmult in secp256k1.c.o
  "_secp256k1_pre_g_128", referenced from:
      _secp256k1_ecmult in secp256k1.c.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[3]: *** [libsecp256k1.dylib] Error 1
make[2]: *** [CMakeFiles/secp256k1.dir/all] Error 2
make[1]: *** [CMakeFiles/secp256k1.dir/rule] Error 2
make: *** [secp256k1] Error 2

My guess is that I need to provide specific header inclusions and/or there's some order in which they need to be included.

This comment seems to be pointing me towards the right direction:

/* Autotools creates libsecp256k1-config.h, of which ECMULT_WINDOW_SIZE is needed.
ifndef guard so downstream users can define their own if they do not use autotools. */
#if !defined(ECMULT_WINDOW_SIZE)
#include "libsecp256k1-config.h"
#endif

But I'm not quite familiar enough with C or these build systems to figure it out myself.

@real-or-random
Copy link
Contributor

Try add_library(secp256k1 SHARED src/secp256k1.c src/precomputed_ecmult_gen.c src/precomputed_ecmult.c)

The USE_BASIC_CONFIG thing is indeed an issue. It's confusing, sorry. The plan is to make sure that the library builds even without any config but we haven't made progress (#929). So for now it's best to set ECMULT_GEN_PREC_BITS and ECMULT_WINDOW_SIZE explicitly. USE_BASIC_CONFIG will probably be removed.

@zone117x
Copy link

zone117x commented Feb 9, 2022

@real-or-random
Copy link
Contributor

Nice! One final note: We usually get better performance with -O2 than with -O3, you may give it a try.

@zone117x
Copy link

zone117x commented Feb 9, 2022

Nice! One final note: We usually get better performance with -O2 than with -O3, you may give it a try.

Will do, thanks!

Btw, does ~1.2MB sound about right for the size of the shared library (using precomputed with the default config)?

1.2M	./linux-x64/libsecp256k1.so
1.2M	./linux-x86/libsecp256k1.so
1.2M	./linux-arm64/libsecp256k1.so
1.2M	./osx-x64/libsecp256k1.dylib
1.2M	./osx-arm64/libsecp256k1.dylib
1.3M	./win-x64/secp256k1.dll
1.2M	./win-x86/secp256k1.dll
1.3M	./win-arm64/secp256k1.dll

The older 2018 output was substantially smaller at around 200KB. I'm assuming it's just the precomputed source?

@real-or-random
Copy link
Contributor

The older 2018 output was substantially smaller at around 200KB. I'm assuming it's just the precomputed source?

Indeed, it's the precomputed source. And 1.2 MB looks about right.

@zone117x
Copy link

zone117x commented Feb 9, 2022

@real-or-random how would you feel about a PR that added something like a /doc/building-with-cmake.md which started off with something like the existing experimental disclaimer in the readme, e.g. "Building with CMake is experimental and has not received enough scrutiny to satisfy the standard of quality of this library but docs are made available for testing and review by the community. "

The doc would contain some minimal cmakelists.txt snippet like the one in the above fork. I think that would very clearly make it apparent that it's not explicitly supported and should not be depended on. While at the same time, a lot of folks like myself and the others who open a cmake PR every year or so would find it quite helpful, as well as help contribute towards ensuring it stays reasonably up-to-date.

Thoughts?

@real-or-random
Copy link
Contributor

Maybe then it's easier to create an external repo for this? Otherwise we'd still need to look at the PRs changing it, so it will still need our resources.

I personally think that we're not married to autotools. If a CMake file (e.g., maintained somewhere else) evolves to a reasonable alternative, and it turns out that it works better, is easier to maintain, or has other advantages (without huge disadvantages), I can imagine we'd be open to switch. (But I can't speak for the others here.) I just don't think that any of the current contributors is willing to spend time on porting to CMake given that we have a working build system.

sipa added a commit that referenced this pull request Mar 8, 2023
e1eb337 ci: Add "x86_64: Windows (VS 2022)" task (Hennadii Stepanov)
10602b0 cmake: Export config files (Hennadii Stepanov)
5468d70 build: Add CMake-based build system (Hennadii Stepanov)

Pull request description:

  This PR adds a [CMake](https://cmake.org/)-based build system.

  Added build instructions and examples to the [`README.md`](https://github.com/hebasto/secp256k1/blob/220628-cmake/README.md#building-with-cmake-experimental) 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:
  - https://gnusha.org/secp256k1/2022-06-23.log
  - https://gnusha.org/secp256k1/2022-06-24.log
  - https://gnusha.org/secp256k1/2022-06-27.log
  - https://gnusha.org/secp256k1/2023-01-30.log

  ---

  Related PRs:
  - #315
  - #549
  - #761

  ---

  **Implementation notes**

  Minimum required CMake version is 3.1. This was required to provide [`C_STANDARD`](https://cmake.org/cmake/help/latest/prop_tgt/C_STANDARD.html) 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:
  - [CMake Versions on Linux Distros](https://gitlab.kitware.com/cmake/community/-/wikis/CMake-Versions-on-Linux-Distros)
  - https://repology.org/project/cmake/versions

  ---

  # Autotools -- CMake Feature Parity Tables

  ## 1. Configuration options

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

  | Autotools | CMake |
  |---|---|
  | `--prefix` | `-DCMAKE_INSTALL_PREFIX`
  | `--enable-shared` | `-DSECP256K1_BUILD_SHARED` |
  | `--enable-static` | `-DSECP256K1_BUILD_STATIC` |
  | `--enable-dev-mode` _hidden_ | N/A, see #1113 (comment) |
  | `--enable-benchmark` | `-DSECP256K1_BUILD_BENCHMARK` |
  | `--enable-coverage` | `-DCMAKE_BUILD_TYPE=Coverage` |
  | `--enable-tests` | `-DSECP256K1_BUILD_TESTS` |
  | `--enable-ctime-tests` | `-DSECP256K1_BUILD_CTIME_TESTS` |
  | `--enable-experimental` | `-DSECP256K1_EXPERIMENTAL` |
  | `--enable-exhaustive-tests` | `-DSECP256K1_BUILD_EXHAUSTIVE_TESTS` |
  | `--enable-examples` | `-DSECP256K1_BUILD_EXAMPLES` |
  | `--enable-module-ecdh` | `-DSECP256K1_ENABLE_MODULE_ECDH` |
  | `--enable-module-recovery` | `-DSECP256K1_ENABLE_MODULE_RECOVERY` |
  | `--enable-module-extrakeys` | `-DSECP256K1_ENABLE_MODULE_EXTRAKEYS` |
  | `--enable-module-schnorrsig` | `-DSECP256K1_ENABLE_MODULE_SCHNORRSIG` |
  | `--enable-external-default-callbacks` | `-DSECP256K1_USE_EXTERNAL_DEFAULT_CALLBACKS` |
  | `--with-test-override-wide-multiply` _hidden_ | `-DSECP256K1_TEST_OVERRIDE_WIDE_MULTIPLY` |
  | `--with-asm` | `-DSECP256K1_ASM` |
  | `--with-ecmult-window` | `-DSECP256K1_ECMULT_WINDOW_SIZE` |
  | `--with-ecmult-gen-precision` | `-DSECP256K1_ECMULT_GEN_PREC_BITS` |
  | `--with-valgrind` | `-DSECP256K1_VALGRING` |

  A screenshot of grouped options from `cmake-gui`:
  ![image](https://user-images.githubusercontent.com/32963518/214821305-fc3ffe82-4d05-4dd7-b2c2-7ca2d5d12e86.png)

  ## 2. `make` targets

  | Autotools | CMake |
  |---|---|
  | `make` | `make` |
  | `make check` | `make check` |
  | `make install` | `make install` * |

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

ACKs for top commit:
  theuni:
    ACK e1eb337.
  sipa:
    ACK e1eb337
  real-or-random:
    ACK e1eb337

Tree-SHA512: ebe2772eeb1a430a0a7ae767fb1a9a82d52d5e9bf2306956cd08f7b442c862be2539774dd10d5555817353d37d1c6add78b8fe5a85bb71239304fb42c98ff337
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants