build: Add CMake-based build system (7 of N) #17

hebasto · 2023-07-10T14:37:14Z

The parent PR: bitcoin#25797.
The previous PRs in the staging branch: #5, #6, #7, #10, #13, #15.

This PR consists of fixups only to make reviewing easier :)

This change encompasses: - bitcoin#24558 - bitcoin#27724

The vcpkg package manager has a separated `boost-test` package, and it is more robust to test its headers explicitly.

hebasto · 2023-07-10T14:37:57Z

Friendly ping @TheCharlatan @theuni :)

fanquake · 2023-07-10T14:45:21Z

src/test/CMakeLists.txt

@@ -49,7 +49,7 @@ add_executable(test_bitcoin
  checkqueue_tests.cpp
  coins_tests.cpp
  coinstatsindex_tests.cpp
-  compilerbug_tests.cpp
+  $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:compilerbug_tests.cpp>


In de1e9c3: Why is MSVC unable to compile this?

Discussed offline. Would much rather add a dummy test case, as we do in other test cases (i.e raii_event_tests), rather than skipping compiling code based on the compiler.

Reworked. Used the same approach as we currently use in raii_event_tests and system_tests.

Otherwise, the ctest command fails when building with MSVC on Windows.

hebasto · 2023-07-10T15:33:56Z

Addressed @fanquake's comment.

theuni · 2023-07-10T18:08:53Z

Addressed @fanquake's comment.

May as well go ahead and upstream this one I guess as MSVC probably has an existing hack that we could get rid of as well?

hebasto · 2023-07-10T18:31:28Z

Addressed @fanquake's comment.

May as well go ahead and upstream this one I guess as MSVC probably has an existing hack that we could get rid of as well?

This can happen when we parallize tests using test_bitcoin --run_test=<TEST_SUITE>. Currently, MSVC builds do not have such capabilities.

theuni

ACK after adding the requested comment.

I'm starting to get nervous about all the if(MSVC) logic, as abstraction is supposed to be the goal here.

I'm going to plan on taking a high-level pass at those cases when we get to the end though.

theuni · 2023-07-17T16:42:24Z

cmake/optional.cmake

-    cross_pkg_check_modules(sqlite sqlite3>=3.7.17 IMPORTED_TARGET)
-    if(sqlite_FOUND)
+    if(MSVC)
+      find_package(unofficial-sqlite3 CONFIG)


Could you please explain (in the form of a comment :) what "unofficial-sqlite3" is and where it comes from and why we care?

It is just a namespace of vcpkg's package:

C:\>vcpkg --triplet=x64-windows-static install sqlite3 Computing installation plan... The following packages are already installed: sqlite3[core,json1]:x64-windows-static -> 3.42.0 sqlite3:x64-windows-static is already installed Total install time: 293 us sqlite3 provides pkgconfig bindings. sqlite3 provides CMake targets: find_package(unofficial-sqlite3 CONFIG REQUIRED) target_link_libraries(main PRIVATE unofficial::sqlite3::sqlite3)

UPD. https://github.com/microsoft/vcpkg/blob/fba81a6a545a66ef3de3a2d36c355f687f72a6b4/ports/sqlite3/CMakeLists.txt#L77:

install(EXPORT unofficial-sqlite3-targets NAMESPACE unofficial::sqlite3:: FILE unofficial-sqlite3-targets.cmake DESTINATION share/unofficial-sqlite3)

UPD2: microsoft/vcpkg#12516

UPD3: microsoft/vcpkg#12505

What wording do you prefer for the comment?

I guess my question is... is there an official one? If this is not intended to be the canonical release, what is?

I guess my question is... is there an official one? If this is not intended to be the canonical release, what is?

The reason of the unofficial:: namespace is:

vcpkg owns the CMake build of sqlite3 and exports the targets via a CMakeLists.txt file in the port folder

I understand the sentence above as the SQLite project does not support CMake for now.

And it is only a namespace.

Btw, we are currently using the same vcpkg's package:

bitcoin/build_msvc/vcpkg.json

Line 10 in d09c8bc

"sqlite3",

An important note. This PR does not change the SQLite dependency. It changes the way how the build system discovers it.

Ok, thanks for explaining. It sure seems that "vcpkg" would've been a better namespace, heh. It's odd that it's named for what it's not rather than what it is.

I suppose just a comment to the effect of "Use of the unofficial namespace is a vcpkg convention" would be helpful.

I suppose just a comment to the effect of "Use of the unofficial namespace is a vcpkg convention" would be helpful.

Done.

I guess my question is... is there an official one? If this is not intended to be the canonical release, what is?

The reason of the unofficial:: namespace is:

vcpkg owns the CMake build of sqlite3 and exports the targets via a CMakeLists.txt file in the port folder

I understand the sentence above as the SQLite project does not support CMake for now.

I have to amend my comment.

From vcpkg's Maintainer Guide:

A core design ideal of vcpkg is to not create "lock-in" for customers. In the build system, there should be no difference between depending on a library from the system, and depending on a library from vcpkg. To that end, we avoid adding CMake exports or targets to existing libraries with "the obvious name", to allow upstreams to add their own official CMake exports without conflicting with vcpkg.

To that end, any CMake configs that the port exports, which are not in the upstream library, should have unofficial- as a prefix. Any additional targets should be in the unofficial::<port>:: namespace.

hebasto · 2023-07-17T17:09:37Z

@theuni

I'm starting to get nervous about all the if(MSVC) logic, as abstraction is supposed to be the goal here.

Like FindSQLite3?

hebasto · 2023-07-17T17:30:37Z

@theuni's comment has been addressed.

Thank you!

LINK : warning LNK4098: defaultlib 'LIBCMTD' conflicts with use of other libs; use /NODEFAULTLIB:library

theuni

ACK a933ddf

ac7bc5b [FIXUP] Rename CCACHE_EXECUTABLE --> CCACHE_COMMAND for consistency (Hennadii Stepanov) 0476509 [FIXUP] Learn to work with recent ccache in MSVC builds (Hennadii Stepanov) 2fd67c7 [FIXUP] Do not disable `TrackFileAccess` in MSVC builds (Hennadii Stepanov) a53ae12 [FIXUP] Use Multi-ToolTask in MSVC builds by default (Hennadii Stepanov) Pull request description: The parent PR: bitcoin#25797. The previous PRs in the staging branch: #5, #6, #7, #10, #13, #15, #17, #18. This PR consists of fixups related to using [Ccache](https://ccache.dev/) in MSVC builds. Top commit has no ACKs. Tree-SHA512: dc01202b054aaeb5b46607bc93126bee0df523df3b4f2df54b566b2e2d6306d784a723fd4a999d0d84fdf31e926a72304790f94b9d57034db0c112ee9f52070e

TheCharlatan · 2023-07-21T14:52:44Z

Post-merge ACK

a65da0d cmake: Redefine configuration flags (Hennadii Stepanov) 1a1dda5 [FIXUP] Evaluate flags set in depends _after_ config-specific flags (Hennadii Stepanov) 482e844 cmake: Warn about not encapsulated build properties (Hennadii Stepanov) 3736470 cmake: Add platform-specific flags (Hennadii Stepanov) e0621e9 cmake: Add `TryAppendLinkerFlag` module (Hennadii Stepanov) ae430cf cmake: Add `TryAppendCXXFlags` module (Hennadii Stepanov) 7903bd5 [FIXUP] Encapsulate common build flags into `core` interface library (Hennadii Stepanov) Pull request description: The parent PR: bitcoin#25797. The previous PRs in the staging branch: #5, #6, #7, #10, #13, #15, #17, #19. --- What is NEW: - functions for checking compiler and linker flags - managing flags for different build types (configurations) EXAMPLES of configuration output on Ubuntu 22.04: - for a single-config generator: ``` $ cmake .. -G "Unix Makefiles" ... Cross compiling ....................... FALSE Preprocessor defined macros ........... C compiler ............................ /usr/bin/cc CFLAGS ................................ C++ compiler .......................... /usr/bin/c++ CXXFLAGS .............................. Common compile options ................ Common link options ................... Linker flags for executables .......... Linker flags for shared libraries ..... Build type (configuration): - CMAKE_BUILD_TYPE ................... RelWithDebInfo - Preprocessor defined macros ........ - CFLAGS ............................. -O2 -g - CXXFLAGS ........................... -O2 -g - LDFLAGS for executables ............ - LDFLAGS for shared libraries ....... Use assembly routines ................. ON Use ccache for compiling .............. ON ... ``` - for a multi-config generator: ``` $ cmake .. -G "Ninja Multi-Config" ... Cross compiling ....................... FALSE Preprocessor defined macros ........... C compiler ............................ /usr/bin/cc CFLAGS ................................ C++ compiler .......................... /usr/bin/c++ CXXFLAGS .............................. Common compile options ................ Common link options ................... Linker flags for executables .......... Linker flags for shared libraries ..... Available build types (configurations) RelWithDebInfo Debug Release 'RelWithDebInfo' build type (configuration): - Preprocessor defined macros ........ - CFLAGS ............................. -O2 -g - CXXFLAGS ........................... -O2 -g - LDFLAGS for executables ............ - LDFLAGS for shared libraries ....... 'Debug' build type (configuration): - Preprocessor defined macros ........ DEBUG DEBUG_LOCKORDER DEBUG_LOCKCONTENTION RPC_DOC_CHECK ABORT_ON_FAILED_ASSUME - CFLAGS ............................. -O0 -g3 - CXXFLAGS ........................... -O0 -g3 -ftrapv - LDFLAGS for executables ............ - LDFLAGS for shared libraries ....... 'Release' build type (configuration): - Preprocessor defined macros ........ - CFLAGS ............................. -O2 - CXXFLAGS ........................... -O2 - LDFLAGS for executables ............ - LDFLAGS for shared libraries ....... Use assembly routines ................. ON Use ccache for compiling .............. ON ... ``` - cross-compiling for Windows: ``` $ make -C depends HOST=x86_64-w64-mingw32 DEBUG=1 NO_QT=1 $ cmake -B build --toolchain depends/x86_64-w64-mingw32/share/toolchain.cmake -DCMAKE_BUILD_TYPE=Debug ... Cross compiling ....................... TRUE, for Windows, x86_64 Preprocessor defined macros ........... _WIN32_WINNT=0x0601 _WIN32_IE=0x0501 WIN32_LEAN_AND_MEAN NOMINMAX WIN32 _WINDOWS _MT _GLIBCXX_DEBUG _GLIBCXX_DEBUG_PEDANTIC C compiler ............................ /usr/bin/x86_64-w64-mingw32-gcc CFLAGS ................................ -pipe -std=c11 -O1 C++ compiler .......................... /usr/bin/x86_64-w64-mingw32-g++-posix CXXFLAGS .............................. -pipe -std=c++17 -O1 Common compile options ................ Common link options ................... -Wl,--major-subsystem-version,6 -Wl,--minor-subsystem-version,1 Linker flags for executables .......... -static Linker flags for shared libraries ..... Build type (configuration): - CMAKE_BUILD_TYPE ................... Debug - Preprocessor defined macros ........ DEBUG DEBUG_LOCKORDER DEBUG_LOCKCONTENTION RPC_DOC_CHECK ABORT_ON_FAILED_ASSUME - CFLAGS ............................. -O0 -g3 - CXXFLAGS ........................... -O0 -g3 -ftrapv - LDFLAGS for executables ............ - LDFLAGS for shared libraries ....... Use assembly routines ................. ON Use ccache for compiling .............. ON ... ``` **A cross-project note.** The `ProcessConfigurations.cmake` is based on the same module that was suggested in bitcoin-core/secp256k1#1291. So, cross-reviewing is welcome :) ACKs for top commit: theuni: ACK a65da0d to keep this moving. This has been sitting for too long :( Tree-SHA512: 57c5e91ddf9675c6a2b56c0cb70fd3f045af8076bee74c49390de38b8d514e130d2086fde6d83d2d1278b437d0a10cc721f0aa44934698110aeadb3a1aef9e64

…tifications fuzz target fab164f fuzz: Avoid signed-integer-overflow in wallet_notifications fuzz target (MarcoFalke) Pull request description: Should avoid ``` policy/feerate.cpp:29:63: runtime error: signed integer overflow: 77600710321911316 * 149 cannot be represented in type 'int64_t' (aka 'long') #0 0x563a1775ed66 in CFeeRate::GetFee(unsigned int) const src/policy/feerate.cpp:29:63 #1 0x563a15913a69 in wallet::COutput::COutput(COutPoint const&, CTxOut const&, int, int, bool, bool, bool, long, bool, std::optional<CFeeRate>) src/./wallet/coinselection.h:91:57 #2 0x563a16fa6a6d in wallet::FetchSelectedInputs(wallet::CWallet const&, wallet::CCoinControl const&, wallet::CoinSelectionParams const&) src/wallet/spend.cpp:297:17 #3 0x563a16fc4512 in wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1105:33 #4 0x563a16fbec74 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1291:16 #5 0x563a16fcf6df in wallet::FundTransaction(wallet::CWallet&, CMutableTransaction&, long&, int&, bilingual_str&, bool, std::set<int, std::less<int>, std::allocator<int>> const&, wallet::CCoinControl) src/wallet/spend.cpp:1361:16 #6 0x563a1597b7b9 in wallet::(anonymous namespace)::FuzzedWallet::FundTx(FuzzedDataProvider&, CMutableTransaction) src/wallet/test/fuzz/notifications.cpp:162:15 #7 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0::operator()() const src/wallet/test/fuzz/notifications.cpp:228:23 #8 0x563a15958240 in unsigned long CallOneOf<wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1>(FuzzedDataProvider&, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1) src/./test/fuzz/util.h:43:27 #9 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>) src/wallet/test/fuzz/notifications.cpp:196:9 #10 0x563a15fdef0c in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #11 0x563a15fdef0c in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5 #12 0x563a158032a4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19822a4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #13 0x563a15802999 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1981999) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #14 0x563a15804586 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983586) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #15 0x563a15804aa7 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983aa7) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #16 0x563a157f21fb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19711fb) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #17 0x563a1581c766 in main (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199b766) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #18 0x7f499e17b0cf (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #19 0x7f499e17b188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #20 0x563a157e70c4 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19660c4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow policy/feerate.cpp:29:63 in MS: 0 ; base unit: 0000000000000000000000000000000000000000 0x3f,0x0,0x2f,0x5f,0x5f,0x5f,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0xff,0xff,0xff,0xff,0xff,0x53,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x0,0x0,0x0,0x0,0x13,0x5e,0x5f,0x5f,0x8,0x25,0x0,0x5f,0x5f,0x5f,0x5f,0x5f,0x5f,0x8,0x25,0xca,0x7f,0x5f,0x5f,0x5f,0x13,0x13,0x5f,0x5f,0x5f,0x2,0xdb,0xca,0x0,0x0,0xe7,0xe6,0x66,0x65,0x0,0x0,0x0,0x0,0x44,0x3f,0xa,0xa,0xff,0xff,0xff,0xff,0xff,0x61,0x76,0x6f,0x69,0x0,0xb5,0x15, ?\000/___}}}}}}}}}}}}}}}}}}}}\377\377\377\377\377S\377\377\377\377\377\000\000\000\000\000\000\023^__\010%\000______\010%\312\177___\023\023___\002\333\312\000\000\347\346fe\000\000\000\000D?\012\012\377\377\377\377\377avoi\000\265\025 artifact_prefix='./'; Test unit written to ./crash-4d3bac8a64d4e58b2f0943e6d28e6e1f16328d7d Base64: PwAvX19ffX19fX19fX19fX19fX19fX19fX3//////1P//////wAAAAAAABNeX18IJQBfX19fX18IJcp/X19fExNfX18C28oAAOfmZmUAAAAARD8KCv//////YXZvaQC1FQ== ACKs for top commit: dergoegge: ACK fab164f brunoerg: ACK fab164f Tree-SHA512: f416828f4394aa7303ee437f141e9bbd23c0e0f1b830e4ef3932338858249ba68a811b9837c5b7ad8c6ab871b6354996434183597c1a910a8d8e8d829693e4b2

hebasto added 2 commits July 10, 2023 10:30

[FIXUP] Boost

91d7327

This change encompasses: - bitcoin#24558 - bitcoin#27724

[FIXUP] for "cmake: Build test_bitcoin executable"

f2dbb17

The vcpkg package manager has a separated `boost-test` package, and it is more robust to test its headers explicitly.

hebasto mentioned this pull request Jul 10, 2023

build: Add CMake-based build system (6 of N) #15

Merged

fanquake reviewed Jul 10, 2023

View reviewed changes

[FIXUP] for "cmake: Build test_bitcoin executable"

31e4e62

Otherwise, the ctest command fails when building with MSVC on Windows.

hebasto force-pushed the 230710-cmake-G branch from 562e04f to 4d1b5be Compare July 10, 2023 15:33

This was referenced Jul 12, 2023

build: Add CMake-based build system (8 of N) #18

Merged

build: Add CMake-based build system (9 of N) #19

Merged

theuni reviewed Jul 17, 2023

View reviewed changes

hebasto force-pushed the 230710-cmake-G branch from 4d1b5be to efc4cd7 Compare July 17, 2023 17:29

hebasto added 2 commits July 17, 2023 20:35

[FIXUP] for "cmake: Add wallet functionality"

769633e

LINK : warning LNK4098: defaultlib 'LIBCMTD' conflicts with use of other libs; use /NODEFAULTLIB:library

[FIXUP] Better document a workaround

a933ddf

hebasto force-pushed the 230710-cmake-G branch from efc4cd7 to a933ddf Compare July 17, 2023 19:35

theuni approved these changes Jul 18, 2023

View reviewed changes

hebasto merged commit e592ad1 into cmake-staging Jul 20, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Add CMake-based build system (7 of N) #17

build: Add CMake-based build system (7 of N) #17

hebasto commented Jul 10, 2023

hebasto commented Jul 10, 2023

fanquake Jul 10, 2023

fanquake Jul 10, 2023

hebasto Jul 10, 2023

hebasto commented Jul 10, 2023

theuni commented Jul 10, 2023

hebasto commented Jul 10, 2023

theuni left a comment

theuni Jul 17, 2023

hebasto Jul 17, 2023 •

edited

hebasto Jul 17, 2023

theuni Jul 17, 2023

hebasto Jul 17, 2023 •

edited

hebasto Jul 17, 2023 •

edited

theuni Jul 17, 2023

hebasto Jul 17, 2023

hebasto Jul 21, 2023

hebasto commented Jul 17, 2023 •

edited

hebasto commented Jul 17, 2023

theuni left a comment

TheCharlatan commented Jul 21, 2023

build: Add CMake-based build system (7 of N) #17

build: Add CMake-based build system (7 of N) #17

Conversation

hebasto commented Jul 10, 2023

hebasto commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Jul 10, 2023

theuni commented Jul 10, 2023

hebasto commented Jul 10, 2023

theuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto Jul 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto Jul 17, 2023 • edited

Choose a reason for hiding this comment

hebasto Jul 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Jul 17, 2023 • edited

hebasto commented Jul 17, 2023

theuni left a comment

Choose a reason for hiding this comment

TheCharlatan commented Jul 21, 2023

hebasto Jul 17, 2023 •

edited

hebasto Jul 17, 2023 •

edited

hebasto Jul 17, 2023 •

edited

hebasto commented Jul 17, 2023 •

edited