Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 20, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
91d7327
[FIXUP] Boost
hebasto Jul 9, 2023
f2dbb17
[FIXUP] for "cmake: Build `test_bitcoin` executable"
hebasto Jul 8, 2023
31e4e62
[FIXUP] for "cmake: Build `test_bitcoin` executable"
hebasto Jul 10, 2023
769633e
[FIXUP] for "cmake: Add wallet functionality"
hebasto Jul 17, 2023
a933ddf
[FIXUP] Better document a workaround
hebasto Jul 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion cmake/module/AddBoostIfNeeded.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,35 @@ function(add_boost_if_needed)
find_package(Boost 1.64.0 REQUIRED)
set_target_properties(Boost::boost PROPERTIES IMPORTED_GLOBAL TRUE)
target_compile_definitions(Boost::boost INTERFACE
$<$<CONFIG:Debug>:BOOST_MULTI_INDEX_ENABLE_SAFE_MODE>
# We don't use multi_index serialization.
BOOST_MULTI_INDEX_DISABLE_SERIALIZATION
)
if(CMAKE_VERSION VERSION_LESS 3.15)
add_library(Boost::headers ALIAS Boost::boost)
endif()

if(BUILD_TESTS)
include(CheckCXXSourceCompiles)
set(CMAKE_REQUIRED_INCLUDES ${Boost_INCLUDE_DIR})
check_cxx_source_compiles("
#define BOOST_TEST_MAIN
#include <boost/test/included/unit_test.hpp>
" HAVE_BOOST_INCLUDED_UNIT_TEST_H
)
if(NOT HAVE_BOOST_INCLUDED_UNIT_TEST_H)
message(FATAL_ERROR "Building test_bitcoin executable requested but boost/test/included/unit_test.hpp header not available.")
endif()

check_cxx_source_compiles("
#define BOOST_TEST_MAIN
#include <boost/test/included/unit_test.hpp>
#include <boost/test/unit_test.hpp>
" HAVE_BOOST_UNIT_TEST_H
)
if(NOT HAVE_BOOST_UNIT_TEST_H)
message(FATAL_ERROR "Building test_bitcoin executable requested but boost/test/unit_test.hpp header not available.")
endif()
endif()

mark_as_advanced(Boost_INCLUDE_DIR)
endfunction()
19 changes: 18 additions & 1 deletion cmake/module/CrossPkgConfig.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,24 @@
find_package(PkgConfig REQUIRED)

function(remove_isystem_from_include_directories_internal target)
#[=[
A workaround for https://gitlab.kitware.com/cmake/cmake/-/issues/20652.

When the pkg-config provides CFLAGS with -isystem options, for instance:

$ pkg-config --cflags-only-I libzmq
-isystem /usr/include/mit-krb5 -I/usr/include/pgm-5.3 -I/usr/include/libxml2

an old CMake fails to parse them properly and the INTERFACE_INCLUDE_DIRECTORIES
property contains "-isystem" as a separated element:

-isystem;/usr/include/mit-krb5;/usr/include/pgm-5.3;/usr/include/libxml2

which ends with an error "Imported target includes non-existent path".

Fixing by removing the "-isystem" element from the INTERFACE_INCLUDE_DIRECTORIES.
]=]

get_target_property(include_directories ${target} INTERFACE_INCLUDE_DIRECTORIES)
if(include_directories)
list(REMOVE_ITEM include_directories -isystem)
Expand All @@ -25,7 +43,6 @@ macro(cross_pkg_check_modules prefix)
pkg_check_modules(${prefix} ${ARGN})
endif()

# A workaround for https://gitlab.kitware.com/cmake/cmake/-/issues/20652.
if(CMAKE_VERSION VERSION_LESS 3.17.3 AND TARGET PkgConfig::${prefix})
remove_isystem_from_include_directories_internal(PkgConfig::${prefix})
endif()
Expand Down
13 changes: 10 additions & 3 deletions cmake/optional.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,16 @@ endif()

if(ENABLE_WALLET)
if(WITH_SQLITE)
include(CrossPkgConfig)
cross_pkg_check_modules(sqlite sqlite3>=3.7.17 IMPORTED_TARGET)
if(sqlite_FOUND)
# TODO: Consider using the FindSQLite3 module after bumping
# the minimum required CMake version up to 3.14+.
if(MSVC)
# Use of the `unofficial::` namespace is a vcpkg package manager convention.
find_package(unofficial-sqlite3 CONFIG)
Copy link

@theuni theuni Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner Author

@hebasto hebasto Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner Author

@hebasto hebasto Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What wording do you prefer for the comment?

Copy link

@theuni theuni Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner Author

@hebasto hebasto Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner Author

@hebasto hebasto Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@theuni theuni Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

@hebasto hebasto Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Done.

Copy link
Owner Author

@hebasto hebasto Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else()
include(CrossPkgConfig)
cross_pkg_check_modules(sqlite3 sqlite3>=3.7.17 IMPORTED_TARGET)
endif()
if(TARGET unofficial::sqlite3::sqlite3 OR TARGET PkgConfig::sqlite3)
set(WITH_SQLITE ON)
set(USE_SQLITE ON)
elseif(WITH_SQLITE STREQUAL "AUTO")
Expand Down
7 changes: 7 additions & 0 deletions src/test/compilerbug_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,13 @@

BOOST_AUTO_TEST_SUITE(compilerbug_tests)

// At least one test case is required to avoid the "Test setup error: no test
// cases matching filter or all test cases were disabled" errror.
BOOST_AUTO_TEST_CASE(dummy)
{
BOOST_CHECK(true);
}

#if defined(__GNUC__)
// This block will also be built under clang, which is fine (as it supports noinline)
void __attribute__ ((noinline)) set_one(unsigned char* ptr)
Expand Down
6 changes: 5 additions & 1 deletion src/wallet/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ if(NOT USE_SQLITE AND NOT USE_BDB)
endif()
if(USE_SQLITE)
target_sources(bitcoin_wallet PRIVATE sqlite.cpp)
target_link_libraries(bitcoin_wallet PRIVATE PkgConfig::sqlite)
target_link_libraries(bitcoin_wallet
PRIVATE
$<TARGET_NAME_IF_EXISTS:unofficial::sqlite3::sqlite3>
$<TARGET_NAME_IF_EXISTS:PkgConfig::sqlite3>
)
endif()
if(USE_BDB)
target_sources(bitcoin_wallet PRIVATE bdb.cpp salvage.cpp)
Expand Down