sipa / minisketch

Minisketch: an optimized library for BCH-based set reconciliation
MIT License
310 stars 52 forks source link

build: Add CMake buildsystem #75

Open hebasto opened 2 years ago

hebasto commented 2 years ago

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:

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 commented 2 years ago

Chasing for Concept (N)ACKs...

hebasto commented 2 years ago

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

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

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

hebasto commented 2 years ago

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

theuni commented 1 year ago

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

@sipa Are you interested in CMake for this project?

sipa commented 1 year ago

@theuni Yeah, will look soon.

theuni commented 1 year ago

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

Imo this should be renamed "Add CMake buildsystem".

Done.

sipa commented 7 months ago

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

hebasto commented 7 months ago

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

Reworked.

The PR description has been updated.

The approach from https://github.com/hebasto/bitcoin/pull/93 was used for printing summary.

hebasto commented 7 months ago

Addressed @theuni's comments.