build: Introduce CMake-based build system #75

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

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

Additionally, a GitHub Actions CI task for MSVC and clang-cl has been added, following this feedback.

Autotools vs CMake configuration option parity

Autotools	CMake
`--disable-ccache`	N/A
`--enable-tests`	`-DMINISKETCH_BUILD_TESTS`
`--enable-benchmark`	`-DMINISKETCH_BUILD_BENCHMARK`
`--enable-fields=,-separated list`	`-DMINISKETCH_FIELDS=;-separated list`
	`-DMINISKETCH_INSTALL`

Notes on the implementation

The default CMAKE_BUILD_TYPE is left unset.
CMake's per-configuration compiler flags are not modified. As a result, they may include -DNDEBUG.
The target name test is reserved. Therefore, the testing binary has been renamed to test-noverify. Alternatively, following the naming used in the libsecp256k1 repo, the test binaries could be renamed as:
- test --> noverify_tests
- test-verify --> verify_tests

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.

sipa · 2025-05-13T18:03:10Z

What is the status here?

hebasto · 2025-05-13T18:05:06Z

What is the status here?

I'll push an update shortly.

hebasto · 2025-06-02T15:55:00Z

.github/workflows/ci.yml

+            # TODO: Resolve the issue and re-enable benchmark.
+            skip_benchmark: true


The issue has been fixed in #96.

theuni · 2025-06-02T22:20:57Z

src/CMakeLists.txt

+
+add_subdirectory(fields)
+
+add_compile_definitions($<$<BOOL:${HAVE_CLZ}>:HAVE_CLZ>)


Instead of attaching CLMUL to the fields interface, how about handling it here?

set_source_files_properties(minisketch.cpp PROPERTIES COMPILE_DEFINITIONS $<$<BOOL:${HAVE_CLMUL}>:HAVE_CLMUL>)

The idea was to keep all HAVE_CLMUL-related code local.

Actually, it seems a good bit more straightforward to me to drop the fields/ subdir and just include that logic here:

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index d8e4fde..cae8228 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -1,13 +1,42 @@ include(GNUInstallDirs) -add_subdirectory(fields) - add_compile_definitions($<$<BOOL:${HAVE_CLZ}>:HAVE_CLZ>) add_library(minisketch minisketch.cpp) target_include_directories(minisketch INTERFACE $<BUILD_INTERFACE:$<$<NOT:$<BOOL:${PROJECT_IS_TOP_LEVEL}>>:${PROJECT_SOURCE_DIR}/include>> ) + +add_library(minisketch_field_sources INTERFACE) +target_sources(minisketch_field_sources + INTERFACE + fields/generic_1byte.cpp + fields/generic_2bytes.cpp + fields/generic_3bytes.cpp + fields/generic_4bytes.cpp + fields/generic_5bytes.cpp + fields/generic_6bytes.cpp + fields/generic_7bytes.cpp + fields/generic_8bytes.cpp +) + +set(clmul_sources + fields/clmul_1byte.cpp + fields/clmul_2bytes.cpp + fields/clmul_3bytes.cpp + fields/clmul_4bytes.cpp + fields/clmul_5bytes.cpp + fields/clmul_6bytes.cpp + fields/clmul_7bytes.cpp + fields/clmul_8bytes.cpp +) + +if(HAVE_CLMUL) + set_source_files_properties(minisketch.cpp PROPERTIES COMPILE_DEFINITIONS HAVE_CLMUL) + set_source_files_properties(${clmul_sources} PROPERTIES COMPILE_OPTIONS ${CLMUL_CXXFLAGS}) + target_sources(minisketch_field_sources INTERFACE ${clmul_sources}) +endif() + target_link_libraries(minisketch PRIVATE minisketch_field_sources) add_library(minisketch_verify EXCLUDE_FROM_ALL minisketch.cpp)

Thanks! Your suggestion has been taken with reordering to keep code specific to each target as local as possible.

Replaced set_source_files_properties back with set_property(SOURCE ...) as the latter accepts empty property value, which is the case for CLMUL_CXXFLAGS on Windows.

Otherwise, see https://github.com/hebasto/minisketch/actions/runs/15404712059.

hebasto · 2025-06-18T12:35:44Z

The PR has been updated to improve integration with downstream projects and now makes use of PUBLIC_HEADER (see bitcoin-core/secp256k1#1679).

The corresponding Bitcoin Core branch that integrates the new build system is available here.

Please note that the current approach does not involve using a project-specific variable to override CMake's BUILD_SHARED_LIBS (this is done via SECP256K1_DISABLE_SHARED in the libsecp256k1 project):

function(add_minisketch subdir)
  message("")
  message("Configuring minisketch subtree...")
  set(BUILD_SHARED_LIBS OFF)
  set(CMAKE_EXPORT_COMPILE_COMMANDS OFF)
  # Hard-code subtree's options.
  set(MINISKETCH_INSTALL OFF)
  set(MINISKETCH_BUILD_TESTS OFF)
  set(MINISKETCH_BUILD_BENCHMARK OFF)
  set(MINISKETCH_FIELDS 32)
  add_subdirectory(${subdir} EXCLUDE_FROM_ALL)
  target_link_libraries(minisketch
    PRIVATE
      core_interface
  )
endfunction()

In integration scenarios similar to bitcoin-core/secp256k1#1678, the $<TARGET_OBJECTS:minisketch> generator expression expands to all associated object files.

cc @theuni

hebasto · 2025-06-27T13:40:45Z

CMakeLists.txt

+# Prevent include directories from parent project from leaking into this one.
+set_property(DIRECTORY PROPERTY INCLUDE_DIRECTORIES "")


See similar code in the libmultiprocess project:

# Prevent include directories from parent project from leaking into this one. set_property(DIRECTORY PROPERTY INCLUDE_DIRECTORIES "")

hebasto · 2025-06-27T13:43:47Z

Pushed some improvements basing on @ryanofsky's bitcoin-core/libmultiprocess@6adbb1d and offline discussions with @purpleKarrot.

~~The corresponding Bitcoin Core branch that integrates the new build system is available here.~~

src/CMakeLists.txt

hebasto · 2025-06-27T19:34:04Z

The feedback from @purpleKarrot has been addressed.

~~The corresponding Bitcoin Core branch that integrates the new build system is available here.~~

purpleKarrot · 2025-06-27T19:45:16Z

@hebasto, I think you accidentally based your latest change on an older revision. A few changes got reverted.

purpleKarrot · 2025-06-27T19:49:22Z

CMakeLists.txt

+#=============================
+# Build Options
+#=============================
+set(CMAKE_CXX_VISIBILITY_PRESET hidden)


This feature actually requires a bit more work to get right. It is currently not working correctly in the autotools build either, so it can be done in a separate PR.

hebasto · 2025-06-27T20:15:12Z

Further feedback from @purpleKarrot has been addressed.

The corresponding Bitcoin Core branch that integrates the new build system is available here.

I think you accidentally based your latest change on an older revision. A few changes got reverted.

Should be OK now.

purpleKarrot · 2025-06-27T20:22:05Z

ACK 850cc86

theuni

ACK 850cc86.

Once integrated into Core, I propose that we nuke the old buildsystem. I doubt sipa has much interest in supporting both here.

sipa · 2025-07-01T21:05:23Z

Weakly-tested ACK 850cc86

f74b7e2bc2 Merge bitcoin-core/minisketch#75: build: Introduce CMake-based build system 850cc868d5 ci: Add GHA workflow with native Windows job 40d56708c8 doc: Add "Building with CMake" section to `README.md` a0c86c79a7 build: Add CMake-based build system git-subtree-dir: src/minisketch git-subtree-split: f74b7e2bc2e5e1a1360927934521216bee5f345c

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

hebasto marked this pull request as draft March 5, 2025 14:14

hebasto force-pushed the 221019-ci-msvc branch from 300dd9a to 900d0c8 Compare June 1, 2025 01:07

hebasto marked this pull request as ready for review June 1, 2025 01:09

hebasto commented Jun 2, 2025

View reviewed changes

theuni reviewed Jun 2, 2025

View reviewed changes

hebasto force-pushed the 221019-ci-msvc branch 3 times, most recently from 3170353 to 77bfe3b Compare June 5, 2025 13:50

hebasto force-pushed the 221019-ci-msvc branch from 77bfe3b to 5592227 Compare June 18, 2025 11:57

hebasto mentioned this pull request Jun 19, 2025

cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS bitcoin-core/secp256k1#1688

Open

hebasto force-pushed the 221019-ci-msvc branch from 5592227 to f403189 Compare June 27, 2025 13:38

hebasto commented Jun 27, 2025

View reviewed changes

purpleKarrot reviewed Jun 27, 2025

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

purpleKarrot reviewed Jun 27, 2025

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

hebasto force-pushed the 221019-ci-msvc branch 2 times, most recently from 85d6f32 to 0d6e8ac Compare June 27, 2025 19:24

purpleKarrot reviewed Jun 27, 2025

View reviewed changes

hebasto added 3 commits June 27, 2025 21:12

build: Add CMake-based build system

a0c86c7

doc: Add "Building with CMake" section to README.md

40d5670

ci: Add GHA workflow with native Windows job

850cc86

hebasto force-pushed the 221019-ci-msvc branch from 0d6e8ac to 850cc86 Compare June 27, 2025 20:12

theuni approved these changes Jul 1, 2025

View reviewed changes

sipa merged commit f74b7e2 into bitcoin-core:master Jul 1, 2025
21 checks passed

hebasto deleted the 221019-ci-msvc branch July 2, 2025 08:30

hebasto mentioned this pull request Jul 2, 2025

Update minisketch subtree and switch to its build script bitcoin/bitcoin#32856

Open

		# TODO: Resolve the issue and re-enable benchmark.
		skip_benchmark: true


		add_subdirectory(fields)

		add_compile_definitions($<$<BOOL:${HAVE_CLZ}>:HAVE_CLZ>)

		# Prevent include directories from parent project from leaking into this one.
		set_property(DIRECTORY PROPERTY INCLUDE_DIRECTORIES "")

build: Introduce CMake-based build system #75

build: Introduce CMake-based build system #75

Uh oh!

Conversation

hebasto commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Autotools vs CMake configuration option parity

Notes on the implementation

Uh oh!

hebasto commented Oct 20, 2022

Uh oh!

hebasto commented Oct 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Oct 31, 2022

Uh oh!

theuni commented Mar 17, 2023

Uh oh!

theuni commented Mar 31, 2023

Uh oh!

sipa commented Mar 31, 2023

Uh oh!

theuni commented Mar 31, 2023

Uh oh!

hebasto commented Apr 16, 2023

Uh oh!

sipa commented Apr 11, 2024

Uh oh!

hebasto commented Apr 11, 2024

Uh oh!

hebasto commented Apr 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Apr 26, 2024

Uh oh!

sipa commented May 13, 2025

Uh oh!

hebasto commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

hebasto commented Oct 20, 2022 •

edited

Loading

hebasto commented Oct 23, 2022 •

edited

Loading

hebasto Apr 26, 2024 •

edited

Loading

hebasto Jun 2, 2025 •

edited

Loading

hebasto Jun 2, 2025 •

edited

Loading

hebasto commented Jun 18, 2025 •

edited

Loading

hebasto commented Jun 27, 2025 •

edited

Loading

hebasto commented Jun 27, 2025 •

edited

Loading

hebasto commented Jun 27, 2025 •

edited

Loading