build: Add CMake-based build system (2 of N)

[hebasto]

The parent PR: bitcoin#25797.
The previous PR in the staging branch: #5.

This PR details:

1. Added project-wide system introspection.

Autoconf macro	Symbols in bitcoin-config.h	CMake implementation
AC_INIT	PACKAGE_NAME PACKAGE_TARNAME PACKAGE_VERSION PACKAGE_STRING PACKAGE_BUGREPORT PACKAGE_URL	Done in #5, except for PACKAGE_TARNAME and PACKAGE_STRING, which are not currently used
AX_CXX_COMPILE_STDCXX	HAVE_CXX17 HAVE_CXX20	CMake uses its own implementation
AC_C_BIGENDIAN	AC_APPLE_UNIVERSAL_BUILD WORDS_BIGENDIAN and C headers	CMake uses its own implementation
AX_PTHREAD	HAVE_PTHREAD HAVE_PTHREAD_PRIO_INHERIT PTHREAD_CREATE_JOINABLE	CMake uses its own implementation
AC_SYS_LARGEFILE	_FILE_OFFSET_BITS _LARGE_FILES	Not implemented yet
AC_FUNC_STRERROR_R	HAVE_DECL_STRERROR_R HAVE_STRERROR_R STRERROR_R_CHAR_P and C headers	Implemented STRERROR_R_CHAR_P, others are not used
C headers	HAVE_INTTYPES_H HAVE_STDINT_H HAVE_STDIO_H HAVE_STDLIB_H HAVE_STRINGS_H HAVE_STRING_H HAVE_SYS_STAT_H HAVE_SYS_TYPES_H HAVE_UNISTD_H STDC_HEADERS	Only HAVE_STRING_H, HAVE_SYS_TYPES_H and HAVE_UNISTD_H are evaluated for internal needs

---

2. Added the leveldb build target (including its crc32c dependency):

• on Linux / *BSD / macOS:

cmake -S . -B build
cmake --build build --target leveldb

• on Windows:

cmake -G "Visual Studio 17 2022" -A x64 -S . -B build
cmake --build build --config Debug --target leveldb  # or --config Release

NOTE. On Windows (MSVC), the leveldb library now uses crc32c one. That is not the case when using the current build system from the build_msvc directory:

bitcoin/build_msvc/libleveldb/libleveldb.vcxproj

Line 53 in fe1b325

<PreprocessorDefinitions>HAVE_CRC32C=0;HAVE_SNAPPY=0;LEVELDB_IS_BIG_ENDIAN=0;_UNICODE;UNICODE;_CRT_NONSTDC_NO_DEPRECATE;LEVELDB_PLATFORM_WINDOWS;%(PreprocessorDefinitions)</PreprocessorDefinitions>

[hebasto]

The PR description has been updated with details about CMake's implementation of Autoconf's system introspection macros.

@theuni wrote:

and have the next PR be the big one that brings in the structure and at least one build target.

This PR delivers a few build targets. But I couldn't move to actual building without tiding up system introspection. Sorry if it didn't meet your expectations. But I promise the next PR will deliver bitcoind target.

[hebasto]

Friendly ping (as GitHub, probably, won't) people who showed some interest in CMake:

@achow101 @adam2k @dongcarl @fanquake @jarolrod @luke-jr @MarcoFalke @ryanofsky @sipsorcery @TheCharlatan @theStack @theuni @vasild @willcl-ark

[theuni]

At a high-level, I think we need some temporary place to stash autotools parity checks like introspection_compat.cmake.

Reason being: autoconf includes lots of ancient checks that aren't worth the time and effort anymore. Good example is HAVE_STRING_H.

So imo, it'd be helpful for you to be able to make the distinction of "I'm adding this to keep parity with autotools and keep the cmake/bitcoin-config.h identical, but it's not longer necessary and we can remove it all at some point after CMake is merged"

[fanquake]

autoconf includes lots of ancient checks that aren't worth the time and effort anymore. Good example is HAVE_STRING_H.

Yep. I see little reason to port all of this cruft over into CMake (or keep it for the long term), especially when we don't even do anything based on the output of most tests.

[theuni]

Yep. I see little reason to port all of this cruft over into CMake (or keep it for the long term), especially when we don't even do anything based on the output of most tests.

I suspect @hebasto's motivation here is trying to exactly re-create bitcoin-config.h exactly as before. IMO that's quite reasonable, as without that we'd never be able to compare object output before/after.

But yes, I very much agree that keeping it for the long term is unnecessary and/or harmful.

[hebasto]

@theuni

At a high-level, I think we need some temporary place to stash autotools parity checks like introspection_compat.cmake.

Reason being: autoconf includes lots of ancient checks that aren't worth the time and effort anymore. Good example is HAVE_STRING_H.

So imo, it'd be helpful for you to be able to make the distinction of "I'm adding this to keep parity with autotools and keep the cmake/bitcoin-config.h identical, but it's not longer necessary and we can remove it all at some point after CMake is merged"

Agree. For now, only required / used in our code definitions are evaluated and passed into the bitcoin-config.h header.

HAVE_STRING_H is used just internally for evaluating of STRERROR_R_CHAR_P:

bitcoin/cmake/introspection.cmake

Lines 134 to 147 in eeb8465

	CHECK_INCLUDE_FILE_CXX(string.h HAVE_STRING_H)
	if(HAVE_STRING_H)
	check_cxx_source_compiles("
	#include <string.h>
	int main()
	{
	char buf[100];
	char x = *strerror_r(0, buf, sizeof buf);
	char* p = strerror_r(0, buf, sizeof buf);
	return !p || x;
	}
	" STRERROR_R_CHAR_P)
	endif()

The same way, internally only, some other definitions are used.

[hebasto]

I suspect @hebasto's motivation here is trying to exactly re-create bitcoin-config.h exactly as before.

Exactly, as it was explicitly requested.

But I'm not going to pull unused crap. That is why a table in the PR description explains which definition is absent and why.

[theuni]

I suspect @hebasto's motivation here is trying to exactly re-create bitcoin-config.h exactly as before.

Exactly, as it was explicitly requested.

But I'm not going to pull unused crap. That is why a table in the PR description explains which definition is absent and why.

Yes, this is very helpful and I appreciate the approach. A diff between the old/new bitcoin-config.h looks as expected for me given the chart above.

[theuni]

For other reviewers: this is correctly under the if(HAVE_UNISTD_H) condition, even though it doesn't appear that way in autotools.

There, AC_CHECK_DECLS sets some defaults, including:

#ifdef HAVE_UNISTD_H
#include <unistd.h>
#endif

[theuni]

Gross global :P

I see it matches what we do currently though. Also gross :)

[theuni]

Another high-level comment: it might be easier to copy our existing way of building crc32/leveldb (basically reinventing their buildsystem internally) than to battle all of the new changes that using the upstream CMake buildsystem drags in.

In fact, yeah, thinking about it some more I'd much prefer it that way for our initial CMake merge. Then we could switch to the upstream buildsystems at a later date and with more review scrutiny.

[theuni]

Another high-level comment: it might be easier to copy our existing way of building crc32/leveldb (basically reinventing their buildsystem internally) than to battle all of the new changes that using the upstream CMake buildsystem drags in.

I'm currently working on converting leveldb to be like this to see how it looks, I'll push it up when done.

[hebasto]

Another high-level comment: it might be easier to copy our existing way of building crc32/leveldb (basically reinventing their buildsystem internally) than to battle all of the new changes that using the upstream CMake buildsystem drags in.

I'm currently working on converting leveldb to be like this to see how it looks, I'll push it up when done.

Maybe my old branch can be helpful:

• cmake/subtree-crc32c.cmake
• cmake/subtree-leveldb.cmake

?

[hebasto]

@furszy @0xB10C @w0xlt

Mind joining us here for testing and reviewing?

[theuni]

Maybe my old branch can be helpful:

Hah, thanks but too late. Here's my attempt:
https://github.com/theuni/bitcoin/commits/cmake2-selfleveldb

I like this approach MUCH better for now. I think we should work on switching to the upstream buildsystem eventually, but this is such a simpler change to deal with.

[hebasto]

@theuni

Maybe my old branch can be helpful:

Hah, thanks but too late. Here's my attempt: https://github.com/theuni/bitcoin/commits/cmake2-selfleveldb

I like this approach MUCH better for now. I think we should work on switching to the upstream buildsystem eventually, but this is such a simpler change to deal with.

Thank you very much for your suggestion! It has been implemented now with a few differences as follows:

• a structure of a target follows the pattern used in the parent build: Add CMake-based build system bitcoin/bitcoin#25797. i.e, source files, with no headers - definitions - include directories - compile options - link libraries - link options
• added a missed src/leveldb/util/histogram.cc source file
• generator expressions are used for brevity
• adjusted for MSVC
• include() command has been moved into the top CMakeLists.txt to avoid unneeded contamination by project-wide definitions and options which are set in the src/CMakeLists.txt
• check for HAVE_O_CLOEXEC is a part of cmake/introspection.cmake as it is used in src/util/tokenpipe.cpp

---

Other @theuni's comments have been addressed as well.

The PR description has been updated with MSVC-related details.

[hebasto]

A note for reviewers.

Consider compiling:

• with CMake:

cmake -S . -B build -DCMAKE_CXX_FLAGS="-g -O2"
cmake --build build --target leveldb

and

• with Autotools:

./autogen.sh
./configure --disable-hardening
make clean
make -C src leveldb/libleveldb.la leveldb/libmemenv.la

The resulted CMake's build/libleveldb.a is essentially the same as the combined Autotools' src/leveldb/.libs/libleveldb.a and src/leveldb/.libs/libmemenv.a.

[fanquake]

Can we atleast just turn off -Werror, there's no need to have that enabled.

[hebasto]

We can return to this comment when introducing default warnings and -DWERROR option.

[theuni]

Agreed. This is ugly for now, but I'm ok punting on this until we see how warning/error handling is intended to be... handled :)

[theuni]

I'd strongly prefer to integrate crc32c into our buildsystem like this instead for now: https://github.com/hebasto/bitcoin/blob/cmake/0615.4/cmake/subtree-crc32c.cmake (unreviewed in detail, but concept ACK :)

Again, I'm NOT advocating for keeping it that way long-term, I think your approach here is the way to go eventually. But for now, it's effectively forcing reviewers to review the entire crc32c buildsystem as well, leading to questions like: #6 (comment)

[hebasto]

@theuni

I'd strongly prefer to integrate crc32c into our buildsystem like this instead for now: https://github.com/hebasto/bitcoin/blob/cmake/0615.4/cmake/subtree-crc32c.cmake (unreviewed in detail, but concept ACK :)

Again, I'm NOT advocating for keeping it that way long-term, I think your approach here is the way to go eventually. But for now, it's effectively forcing reviewers to review the entire crc32c buildsystem as well, leading to questions like: #6 (comment)

Done.

Also addressed @fanquake's comments:

• build: Add CMake-based build system (2 of N)  #6 (comment)
• build: Add CMake-based build system (2 of N)  #6 (comment)

[hebasto]

@theuni

These are actually behavioral changes because CMake (surprisingly imo) invokes the linker for check_cxx_source_compiles.

We may want to investigate set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) at the top-level to disable that, then opting-in to linking as necessary (like for the socket check above).

Thanks! A dedicated check_cxx_source_links macro has been added.

[hebasto]

@theuni wrote:

and have the next PR be the big one that brings in the structure and at least one build target.

... I promise the next PR will deliver bitcoind target.

See #7 :)