build: Add CMake buildsystem #75

hebasto · 2022-10-20T00:02:25Z

This PR adds a new CMake-based build system, which can be reused in downstream projects, including Bitcoin Core.

Also an MSVC CI task has been added per feedback.

Autotools vs CMake configuration option parity:

Autotools	CMake
`--disable-ccache`	N/A
`--enable-tests`	`-DMINISKETCH_TESTS`
`--enable-benchmark`	`-DMINISKETCH_BENCHMARK`
`--enable-fields`	`-DMINISKETCH_FIELDS`

Here is a examples of output when configuring:

for the "Ninja Multi-Config" generator:

$ cmake -B build -G "Ninja Multi-Config" -DMINISKETCH_BENCHMARK=ON -DMINISKETCH_FIELDS=42
-- The CXX compiler identification is GNU 13.2.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test HAVE_CLMUL
-- Performing Test HAVE_CLMUL - Success
-- Performing Test HAVE_CLZ
-- Performing Test HAVE_CLZ - Success


minisketch configure summary
============================
Library type .......................... Static
Build options:
  clmul fields ........................ ENABLED
  clz implementation .................. compiler builtin
Optional binaries:
  benchmark ........................... ON
  tests ............................... ON

Cross compiling ....................... FALSE
C++ compiler .......................... GNU 13.2.0, /usr/bin/c++
Available build configurations ........ Debug, Release, RelWithDebInfo
Default build configuration ........... Debug
'Debug' build configuration:
  C++ flags ........................... -g -Wall
'Release' build configuration:
  C++ flags ........................... -O3 -DNDEBUG -Wall
'RelWithDebInfo' build configuration:
  C++ flags ........................... -O2 -g -DNDEBUG -Wall

CMake Warning at CMakeLists.txt:117 (message):
  Only compiling in support for field sizes: 42

  This means the library will lack support for other field sizes entirely.



-- Configuring done (0.5s)
-- Generating done (0.0s)
-- Build files have been written to: /home/hebasto/git/minisketch/build

for the "Unix Makefiles" generator (the default on Linux):

$ cmake -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo -DMINISKETCH_BENCHMARK=ON -DMINISKETCH_FIELDS=42
-- The CXX compiler identification is GNU 13.2.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test HAVE_CLMUL
-- Performing Test HAVE_CLMUL - Success
-- Performing Test HAVE_CLZ
-- Performing Test HAVE_CLZ - Success


minisketch configure summary
============================
Library type .......................... Static
Build options:
  clmul fields ........................ ENABLED
  clz implementation .................. compiler builtin
Optional binaries:
  benchmark ........................... ON
  tests ............................... ON

Cross compiling ....................... FALSE
C++ compiler .......................... GNU 13.2.0, /usr/bin/c++
Build configuration ................... RelWithDebInfo
C++ flags ............................. -O2 -g -DNDEBUG -Wall

CMake Warning at CMakeLists.txt:117 (message):
  Only compiling in support for field sizes: 42

  This means the library will lack support for other field sizes entirely.



-- Configuring done (0.6s)
-- Generating done (0.0s)
-- Build files have been written to: /home/hebasto/git/minisketch/build

hebasto · 2022-10-20T00:03:02Z

Chasing for Concept (N)ACKs...

hebasto · 2022-10-23T09:42:45Z

The currently suggesting minimal CMake-based build system implementation is enough for:

building binaries on Windows natively, including CI
integrating with Bitcoin Core CMake-based build system as easy as

add_subdirectory(minisketch EXCLUDE_FROM_ALL)
target_compile_definitions(minisketch
  PRIVATE
    DISABLE_DEFAULT_FIELDS
    ENABLE_FIELD_32
)

To became a full fledged one, the current minimal CMake-based build system requires some additional features:

more robustness (e.g., checking compiler/linker flags before applying them)
ability to install the built artifacts
ability to export the built artifacts and, probably, packaging of them
documentation

Although, those features are not required for this PR goal, i.e., providing a native Windows CI task.

hebasto · 2022-10-31T13:19:35Z

Updated 5851544 -> d2408e0 (pr75.03 -> pr75.04, diff):

policy CMP0063 set explicitly to avoid CMake warnings
the default CMake build directory added to the .gitignore file

theuni · 2023-03-17T20:23:38Z

Imo this should be renamed "Add CMake buildsystem". I looked for this a while back and didn't find it because CMake isn't mentioned in the title.

Concept ACK. Will review next week.

theuni · 2023-03-31T14:31:58Z

@sipa Are you interested in CMake for this project?

sipa · 2023-03-31T20:17:37Z

@theuni Yeah, will look soon.

theuni · 2023-03-31T20:32:41Z

@sipa No rush. I was just looking for a concept ACK.

I worked up a CMake impl as well before seeing this PR. I think we'd benefit from parts of each, so I'll get with @hebasto on creating a combined version for review.

hebasto · 2023-04-16T12:11:21Z

Imo this should be renamed "Add CMake buildsystem".

Done.

sipa · 2024-04-11T10:11:50Z

I'm happy to add a CMake build system for minisketch. I will need review from people more experienced with build systems, though.

hebasto · 2024-04-11T11:09:34Z

I'm happy to add a CMake build system for minisketch. I will need review from people more experienced with build systems, though.

I'm going to update this PR shortly.

hebasto · 2024-04-16T17:00:07Z

Reworked.

The PR description has been updated.

The approach from hebasto/bitcoin#93 was used for printing summary.

theuni · 2024-04-26T14:05:29Z

CMakeLists.txt

+endif()
+
+if(MINGW)
+  add_link_options(-static)


I don't think we should port this functionality. Instead we should use the CMake shared/static selection machinery.

This affects only benchmark/test binaries and allows to run them on the build system via Wine when building either a static library or a shared one.

UPD. It also links libstdc++-6 to libminisketch.dll statically when cross-compiling for Windows and -DBUILD_SHARED_LIBS=YES is provided.

Consider

cmake -B build \ -DCMAKE_BUILD_TYPE=Release \ -DCMAKE_SYSTEM_NAME=Windows \ -DCMAKE_CXX_COMPILER=x86_64-w64-mingw32-g++-posix cmake --build build --target test wine ./build/src/test.exe

or

cmake -B build \ -DCMAKE_BUILD_TYPE=Release \ -DCMAKE_SYSTEM_NAME=Windows \ -DCMAKE_CXX_COMPILER=x86_64-w64-mingw32-g++-posix \ -DBUILD_SHARED_LIBS=YES cmake --build build --target test wine ./build/src/test.exe

src/CMakeLists.txt

CMakeLists.txt

theuni · 2024-04-26T14:30:14Z

CMakeLists.txt

+)
+
+if(DEFINED CMAKE_CXX_STANDARD)
+  if(CMAKE_CXX_STANDARD EQUAL 98 OR CMAKE_CXX_STANDARD LESS 11)


Hah! Stupid CMake :)

Y2K problem :)

hebasto · 2024-04-26T16:02:17Z

Addressed @theuni's comments.

hebasto force-pushed the 221019-ci-msvc branch from bd57926 to 14503b1 Compare October 23, 2022 08:00

hebasto marked this pull request as ready for review October 23, 2022 08:02

hebasto force-pushed the 221019-ci-msvc branch from 14503b1 to 30fdfb3 Compare October 23, 2022 09:40

hebasto force-pushed the 221019-ci-msvc branch from 30fdfb3 to 5851544 Compare October 30, 2022 20:41

hebasto mentioned this pull request Oct 30, 2022

build: Add CMake-based build system bitcoin-core/secp256k1#1113

Merged

hebasto force-pushed the 221019-ci-msvc branch from 5851544 to d2408e0 Compare October 31, 2022 13:14

hebasto changed the title ~~ci: Add "x86_64: Windows (VS 2022)" task~~ build: Add CMake buildsystem Apr 16, 2023

hebasto force-pushed the 221019-ci-msvc branch 2 times, most recently from 66b4133 to b486457 Compare April 16, 2024 16:44

theuni reviewed Apr 26, 2024

View reviewed changes

hebasto force-pushed the 221019-ci-msvc branch from b486457 to cbbfdae Compare April 26, 2024 16:01

hebasto added 2 commits April 26, 2024 17:26

build: Add CMake-based build system

be68da4

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

300dd9a

hebasto force-pushed the 221019-ci-msvc branch from cbbfdae to 300dd9a Compare April 26, 2024 16:26

theuni mentioned this pull request Apr 26, 2024

Fix or disable conversion warnings? #84

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Add CMake buildsystem #75

build: Add CMake buildsystem #75

hebasto commented Oct 20, 2022 •

edited

Loading

hebasto commented Oct 20, 2022

hebasto commented Oct 23, 2022 •

edited

Loading

hebasto commented Oct 31, 2022

theuni commented Mar 17, 2023

theuni commented Mar 31, 2023

sipa commented Mar 31, 2023

theuni commented Mar 31, 2023

hebasto commented Apr 16, 2023

sipa commented Apr 11, 2024

hebasto commented Apr 11, 2024

hebasto commented Apr 16, 2024

theuni Apr 26, 2024

hebasto Apr 26, 2024 •

edited

Loading

hebasto Apr 26, 2024

theuni Apr 26, 2024

hebasto Apr 26, 2024

hebasto commented Apr 26, 2024

build: Add CMake buildsystem #75

Are you sure you want to change the base?

build: Add CMake buildsystem #75

Conversation

hebasto commented Oct 20, 2022 • edited Loading

hebasto commented Oct 20, 2022

hebasto commented Oct 23, 2022 • edited Loading

hebasto commented Oct 31, 2022

theuni commented Mar 17, 2023

theuni commented Mar 31, 2023

sipa commented Mar 31, 2023

theuni commented Mar 31, 2023

hebasto commented Apr 16, 2023

sipa commented Apr 11, 2024

hebasto commented Apr 11, 2024

hebasto commented Apr 16, 2024

theuni Apr 26, 2024

Choose a reason for hiding this comment

hebasto Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

hebasto Apr 26, 2024

Choose a reason for hiding this comment

theuni Apr 26, 2024

Choose a reason for hiding this comment

hebasto Apr 26, 2024

Choose a reason for hiding this comment

hebasto commented Apr 26, 2024

hebasto commented Oct 20, 2022 •

edited

Loading

hebasto commented Oct 23, 2022 •

edited

Loading

hebasto Apr 26, 2024 •

edited

Loading