Skip to content
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.

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

Rebased cmake-staging branch (post PRs 17, 18, 19) #31

Closed
wants to merge 38 commits into from

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Sep 16, 2023

This is the cmake-staging branch rebased on the recent bitcoin/master one with squashed "FIXUP" commits.

Additionally addressed #18 (comment).


Previous linearization PRs:


A few notes regarding testing this PR on Windows:

  1. The Chocolatey package manager can be used to install required and optional tools:
choco install python3
choco install ccache --version=4.7.5
  1. To build and test natively on Windows, run commands as follows:
mkdir build
cd build
cmake .. -A x64 -DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake -DVCPKG_TARGET_TRIPLET=x64-windows-static
cmake --build . --config Release
ctest -C Release
  1. Run functional tests as follows:
set BITCOIND=src\Release\bitcoind.exe
set BITCOINCLI=src\Release\bitcoin-cli.exe
set BITCOINUTIL=src\Release\bitcoin-util.exe
set BITCOINWALLET=src\Release\bitcoin-wallet.exe
py -3 test\functional\test_runner.py
  1. The bench_bitcoin.exe fails due to a bug, For more details, please see test, bench: Initialize and terminate use of Winsock properly bitcoin/bitcoin#28486.

KNOWN ISSUES:

@hebasto
Copy link
Owner Author

hebasto commented Sep 16, 2023

@vasild @TheCharlatan @theStack @stickies-v @Sjors @sipsorcery @theuni @willcl-ark

Ready for extensive testing :)

The next meaningful PR with new commits is going to be based on this branch and will be submitted shortly.

@hebasto
Copy link
Owner Author

hebasto commented Sep 16, 2023

In this comment I'm going to post tested configurations on particular systems.

  1. Debian 10 (buster). The system's CMake version 3.13.4 is the minimum supported one.
  • with system packages:
mkdir build
cd build
cmake -S .. -DCMAKE_C_COMPILER=clang-13 -DCMAKE_CXX_COMPILER='clang++-13;-stdlib=libc++' -DWITH_BDB=OFF
cmake --build . -j $(nproc)
ctest -j $(nproc)
# No functional tests as Python 3.8 is not available by default.
  • with depends:
make -j $(nproc) -C depends NO_QT=1 CC=clang-13 CXX='clang++-13 -stdlib=libc++'
mkdir build
cd build
cmake -S .. -DCMAKE_TOOLCHAIN_FILE=../depends/x86_64-pc-linux-gnu/share/toolchain.cmake
cmake --build . -j $(nproc)
ctest -j $(nproc)
# No functional tests as Python 3.8 is not available by default.
  1. Ubuntu 20.04 LTS with system's CMake 3.16.3.
  • with system packages:
mkdir build
cd build
cmake -S .. -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++
cmake --build . -j $(nproc)
ctest -j $(nproc)
./test/functional/test_runner.py -j $(nproc)
  1. Fedora 38.
  • with depends:
make -j $(nproc) -C depends NO_QT=1
mkdir build
cd build
cmake -S .. --toolchain ../depends/x86_64-pc-linux-gnu/share/toolchain.cmake
cmake --build . -j $(nproc)
ctest -j $(nproc)
./test/functional/test_runner.py -j $(nproc)
  1. macOS Monterey (Intel).
  • with Homebrew's packages:
mkdir build
cd build
cmake -S ..
cmake --build . -j 2
ctest -j 2
./test/functional/test_runner.py
  1. macOS Ventura 13.6 (Apple M1, Xcode 15.0).
  • with Homebrew's packages:
mkdir build
cd build
cmake -S ..
cmake --build . -j 8
ctest -j 8
./test/functional/test_runner.py -j 8
  1. Cross-compiling on Ubuntu 22.04.
  • for Windows:
make -C depends -j $(nproc) HOST=x86_64-w64-mingw32 NO_QT=1
mkdir build
cd build
cmake -S .. --toolchain ../depends/x86_64-w64-mingw32/share/toolchain.cmake
cmake --build . -j $(nproc)
ctest -j $(nproc)
  • for macOS arm64
make -C depends -j $(nproc) HOST=arm64-apple-darwin NO_QT=1
mkdir build
cd build
cmake -S .. --toolchain ../depends/arm64-apple-darwin/share/toolchain.cmake
cmake --build . -j $(nproc)

@hebasto
Copy link
Owner Author

hebasto commented Sep 17, 2023

Fixed a couple of compatibility issues found during testing on a range of platforms.

@hebasto
Copy link
Owner Author

hebasto commented Sep 17, 2023

I found a bug: it fails to find sys/sdt.h in depends. Going to fix it in a follow-up.

@hebasto
Copy link
Owner Author

hebasto commented Sep 17, 2023

Another couple of minor issues have been found during testing and fixed.

The list of the tested platforms and configurations has been updated as well.

@hebasto
Copy link
Owner Author

hebasto commented Sep 17, 2023

The next meaningful PR with new commits is going to be based on this branch and will be submitted shortly.

Fellow reviewers, I'm going to suggest you hardening and shared libraries :)

Any other suggestions?

And good news! After that, only two steps remain until switching Guix to CMake: Qt and Guix scripts themselves.

@Sjors
Copy link

Sjors commented Sep 18, 2023

Tested on Intel macOS 13.5.2 (Ventura).

The "configure" step took 53 seconds the first time, 34 seconds the second time (after a git clean -dfx, it caches?). Compared to 18 + 40 seconds for ./autogen && ./configure --without-gui --disable-fuzz-binary.

The warning when UPNC dependencies are not present is a bit verbose, but fine for now:

-- Could NOT find MiniUPnPc (missing: MiniUPnPc_LIBRARY MiniUPnPc_INCLUDE_DIR MiniUPnPc_API_VERSION_OK) 
CMake Warning at cmake/optional.cmake:62 (message):
  libminiupnpc not found, disabling.

  To skip libminiupnpc check, use "-DWITH_MINIUPNPC=OFF".

Call Stack (most recent call first):
  CMakeLists.txt:240 (include)

I don't see a similar warning for USDT. Since UPNC is disabled by default, we should probably not complain if it's missing unless the user asked for it.

(update: yesterday I was on battery, which seems to make a 4x difference; updated the numbers with a plugged in device)

cmake --build . -j 2 (after wiping ccache) takes 1:52 minutes, vs. 0:15 minutes for make -j17 (I wiped ccache). So it's quite a bit slower, but maybe that's worth it?

I also briefly tested that bitcoind still works and I can run the functional test suite.

@theuni
Copy link

theuni commented Sep 22, 2023

I'm away on holiday until the 2nd, but will pick this up asap when I return.

@willcl-ark
Copy link

Just given this a quick test run, and was also interested in comparing against autotools...

One thing I noticed up front was that with cmake the BDB wallet is built where I would need to pass --with-incompatible-bdb with autotools, e.g. when using system BDB. You do get a clear warning, but still a change in behaviour, I think:

-- Found BerkeleyDB: /usr/lib/x86_64-linux-gnu/libdb_cxx.so (found suitable version "5.3", minimum required is "4.8")
CMake Warning at cmake/optional.cmake:148 (message):
  Found Berkeley DB (BDB) other than 4.8.
Call Stack (most recent call first):
  CMakeLists.txt:240 (include)


CMake Warning at cmake/optional.cmake:150 (message):
  BDB (legacy) wallets opened by this build would not be portable!

Will come back to this to review the new code and test hardening/shared libraries later in the week.
  If this is intended, pass "-DWARN_INCOMPATIBLE_BDB=OFF".

  Passing "-DWITH_BDB=OFF" will suppress this warning.

Call Stack (most recent call first):
  CMakeLists.txt:240 (include)

These are the results I got building with various combinations of cmake, cmake + ninja and autotools:

  1. cmake:
cmake_script
#!/usr/bin/env fish

ccache --clear

mkdir build
cd build

set start_time (date +%s)

# configure
# use CMake to generate and clang as compiler
cmake -S .. -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCCACHE=OFF

set configure_time (date +%s)
set configure_interval (math "$configure_time - $start_time")

# build
cmake --build . -j (nproc)

set build_time (date +%s)
set build_interval (math "$build_time - $configure_time")

# test
# use CTest
ctest -j (nproc)

set end_time (date +%s)
set test_interval (math "$end_time - $build_time")
set total_time (math "$end_time - $start_time")

echo "Time taken for configuration: $configure_interval seconds"
echo "Time taken for build: $build_interval seconds"
echo "Time taken for testing: $test_interval seconds"
echo "Total time: $total_time seconds"
Time taken for configuration: 16 seconds
Time taken for build: 168 seconds
Time taken for testing: 12 seconds
Total time: 196 seconds
  1. cmake + ninja:
cmake_ninja_script
#!/usr/bin/env fish

ccache --clear

mkdir build
cd build

set start_time (date +%s)

# configure
# use CMake to generate, specify Ninja as generator and clang as compiler
# cmake -G Ninja -S .. -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCCACHE=OFF
cmake -S .. -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCCACHE=OFF

set configure_time (date +%s)
set configure_interval (math "$configure_time - $start_time")

# build
# ninja -j (nproc)
cmake --build . -j (nproc)

set build_time (date +%s)
set build_interval (math "$build_time - $configure_time")

# test
# use CTest
ctest -j (nproc)

set end_time (date +%s)
set test_interval (math "$end_time - $build_time")
set total_time (math "$end_time - $start_time")

echo "Time taken for configuration: $configure_interval seconds"
echo "Time taken for build: $build_interval seconds"
echo "Time taken for testing: $test_interval seconds"
echo "Total time: $total_time seconds"
Time taken for configuration: 17 seconds
Time taken for build: 150 seconds
Time taken for testing: 12 seconds
Total time: 179 seconds
  1. autotools:
autotools_script
#!/usr/bin/env fish

ccache --clear

set start_time (date +%s)

# configure
./configure --without-gui --with-incompatible-bdb

set configure_time (date +%s)
set configure_interval (math "$configure_time - $start_time")

# build
make -j(nproc) -C src bitcoind bitcoin-cli bitcoin-tx bitcoin-util bitcoin-wallet test/test_bitcoin

set build_time (date +%s)
set build_interval (math "$build_time - $configure_time")

# test
set -l test_suites addrman_tests allocator_tests amount_tests argsman_tests arith_uint256_tests banman_tests base32_tests base58_tests base64_tests bech32_tests bip32_tests blockchain_tests blockencodings_tests blockfilter_index_tests blockfilter_tests blockmanager_tests bloom_tests bswap_tests checkqueue_tests coins_tests coinstatsindex_tests compilerbug_tests compress_tests crypto_tests cuckoocache_tests dbwrapper_tests denialofservice_tests descriptor_tests flatfile_tests fs_tests getarg_tests hash_tests headers_sync_chainwork_tests httpserver_tests i2p_tests interfaces_tests key_io_tests key_tests logging_tests mempool_tests merkle_tests merkleblock_tests miner_tests miniscript_tests minisketch_tests multisig_tests net_peer_eviction_tests net_tests netbase_tests orphanage_tests pmt_tests policy_fee_tests policyestimator_tests pow_tests prevector_tests raii_event_tests random_tests rbf_tests rest_tests result_tests reverselock_tests rpc_tests sanity_tests scheduler_tests script_p2sh_tests script_parse_tests script_segwit_tests script_standard_tests script_tests scriptnum_tests serfloat_tests serialize_tests settings_tests sighash_tests sigopcount_tests skiplist_tests sock_tests streams_tests sync_tests system_tests timedata_tests torcontrol_tests transaction_tests translation_tests txindex_tests txpackage_tests txreconciliation_tests txrequest_tests txvalidation_tests txvalidationcache_tests uint256_tests util_tests util_threadnames_tests validation_block_tests validation_chainstate_tests validation_chainstatemanager_tests validation_flush_tests validation_tests validationinterface_tests versionbits_tests xoroshiro128plusplus_tests
echo $test_suites | tr ' ' '\n' | parallel -j (nproc) './src/test/test_bitcoin -t {}'

set end_time (date +%s)
set test_interval (math "$end_time - $build_time")
set total_time (math "$end_time - $start_time")

echo "Time taken for configuration: $configure_interval seconds"
echo "Time taken for build: $build_interval seconds"
echo "Time taken for testing: $test_interval seconds"
echo "Total time: $total_time seconds"
Time taken for configuration: 28 seconds
Time taken for build: 130 seconds
Time taken for testing: 7 seconds
Total time: 165 seconds

I only ran these twice each (and took the best result of two runs). I also used parallel to run the tests in the autotools run (as not all of them are run via ctest yet), which appears to be more efficient than having ctest run them :)

@hebasto
Copy link
Owner Author

hebasto commented Sep 25, 2023

@willcl-ark

Just given this a quick test run, and was also interested in comparing against autotools...

Thank you for testing!

I'd like to add a few points as follows.

  1. To configure Autotools to build the same set of binaries as this branch, one should run:
./configure --without-gui --without-libs --with-incompatible-bdb --disable-fuzz-binary
  1. Both ctest and Autotools make check runs the same 119 tests from the src/test and src/wallet/test directories plus 3 additional tests:
  • test/util/test_runner.py
  • test/util/rpcauth-test.py
  • bench/bench_bitcoin -sanity-check -priority-level=high

However, Autotools make check runs for minsketch, secp256k1 and univalue subtrees as well.

Do we actually need to test code in subtrees?

@willcl-ark
Copy link

I'd like to add a few points as follows.

1. To configure Autotools to build the same set of binaries as this branch, one should run:
./configure --without-gui --without-libs --with-incompatible-bdb --disable-fuzz-binary

Thanks, I missed --without-libs --disable-fuzz-binary. This config yielded these results per my updated script (best of 2 runs):

Time taken for configuration: 22 seconds
Time taken for build: 130 seconds
Time taken for testing: 6 seconds
Total time: 158 seconds

However, Autotools make check runs for minsketch, secp256k1 and univalue subtrees as well.

Do we actually need to test code in subtrees?

Yes this is what I was trying to correct for, as it makes quite some difference, not that I expected there to be any runtime difference in particular.

I think I'd agree that subtrees don't need to be run, although I think it'd be good practice to run them after any update to the subtree, if there was a convenient mechanism to do so...

FYI I scratched out a basic CMakePresets.json file to make for faster building which is quite nice. I think packaging some presets (besides the defaults?) could be a nice improvement for us?

CMakePresets.json
{
  "version": 6,
  "configurePresets": [
    {
      "name": "debug",
      "description": "Debug configuration with verbose output.",
      "binaryDir": "${sourceDir}/build/debug",
      "cacheVariables": {
        "CMAKE_BUILD_TYPE": "Debug",
        "CMAKE_VERBOSE_MAKEFILE": "ON"
      }
    },
    {
      "name": "debug-ninja",
      "description": "Debug configuration with ninja.",
      "binaryDir": "${sourceDir}/build/debug",
      "cacheVariables": {
        "CMAKE_BUILD_TYPE": "Debug",
        "CMAKE_VERBOSE_MAKEFILE": "ON"
      },
      "generator": "Ninja"
    },
    {
      "name": "release",
      "description": "Release configuration.",
      "binaryDir": "${sourceDir}/build/release",
      "cacheVariables": {
        "CMAKE_BUILD_TYPE": "Release"
      }
    }
  ],
  "buildPresets": [
    {
      "name": "debug",
      "configurePreset": "debug",
      "description": "Build using the debug configuration."
    },
    {
      "name": "debug-ninja",
      "configurePreset": "debug-ninja",
      "description": "Build the debug configuration using ninja."
    },
    {
      "name": "release",
      "configurePreset": "release",
      "description": "Build using the release configuration."
    }
  ],
  "testPresets": [
    {
      "name": "all",
      "description": "Run all tests.",
      "configurePreset": "debug"
    }
  ],
  "workflowPresets": [
    {
      "name": "debug-ninja",
      "description": "Configure and build all in debug mode using ninja (auto nproc).",
      "steps": [
        {
          "type": "configure",
          "name": "debug-ninja"
        },
        {
          "type": "build",
          "name": "debug-ninja"
        }
      ]
    }
  ]
}

You can use like:

# configure preset
cmake --preset debug-ninja

# build preset
cmake --build --preset debug-ninja

# configure and run preset
cmake --workflow --preset debug-ninja

It should be possible to also add test steps to the workflow preset, but I haven't done it yet...

@fanquake
Copy link

You do get a clear warning, but still a change in behaviour, I think:

We definitely should not be reverting to building the BDB wallet by default, without the additional flags.

@hebasto
Copy link
Owner Author

hebasto commented Sep 26, 2023

On macOS (Xcode 15.0), the linker emits warnings:

[....] Linking CXX executable test_bitcoin
ld: warning: ignoring duplicate libraries: '../../libleveldb.a', '../libbitcoin_common.a', '../univalue/libunivalue.a', '../util/libbitcoin_util.a'
[....] Linking CXX executable bench_bitcoin
ld: warning: ignoring duplicate libraries: '../../libleveldb.a', '../univalue/libunivalue.a', '../wallet/libbitcoin_wallet.a'

I'm going to tackle with them in follow-ups.

UPD. There is an ongoing discussion in the CMake community: https://discourse.cmake.org/t/avoid-duplicate-linking-to-avoid-xcode-15-warnings/9084.

@hebasto
Copy link
Owner Author

hebasto commented Oct 11, 2023

@TheCharlatan @theuni

Do you reckon we can move further with this rebasing PR and update the staging branch?

Copy link

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK c20de96

Checked the changes with git range-diff. I did not do a lot of platform testing, will start doing that with the next feature pull request.

Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

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

ACK c20de96.

Rebase looks fine to me, added one question about the only thing that stood out about the diff. It's not a big deal though.

I'll also wait to do major platform testing until the next PR.

bitcoind.cpp
init/bitcoind.cpp
)
target_link_libraries(bitcoind
Copy link

Choose a reason for hiding this comment

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

This is PRIVATE in the staging branch, and I don't see any explanation for the change?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Distinguishing PRIVATE vs PUBLIC vs INTERFACE scopes matters only for targets that have dependants.

That is not the case for executable targets. Therefore, target_link_libraries with a simpler signature is used.

This particular change has done for the sake of consistency with other similar cases.

@hebasto
Copy link
Owner Author

hebasto commented Oct 17, 2023

This branch has been force pushed into the https://github.com/hebasto/bitcoin/tree/cmake-staging.

Closing.

@hebasto hebasto closed this Oct 17, 2023
@hebasto
Copy link
Owner Author

hebasto commented Oct 19, 2023

On macOS (Xcode 15.0), the linker emits warnings:

[....] Linking CXX executable test_bitcoin
ld: warning: ignoring duplicate libraries: '../../libleveldb.a', '../libbitcoin_common.a', '../univalue/libunivalue.a', '../util/libbitcoin_util.a'
[....] Linking CXX executable bench_bitcoin
ld: warning: ignoring duplicate libraries: '../../libleveldb.a', '../univalue/libunivalue.a', '../wallet/libbitcoin_wallet.a'

I'm going to tackle with them in follow-ups.

Fixed in #34.

@hebasto
Copy link
Owner Author

hebasto commented Oct 20, 2023

I found a bug: it fails to find sys/sdt.h in depends. Going to fix it in a follow-up.

Fixed in #37.

Copy link

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

tACK with cmake 3.22.1-1ubuntu1.22.04.1 following instructions above for ubuntu.

Ran both unit & functional tests.
Quick manual tests using bitcoind & bitcoin-cli.

hebasto added a commit that referenced this pull request Oct 24, 2023
ab82110 fixup! cmake: Add `systemtap-sdt` optional package support (Hennadii Stepanov)

Pull request description:

  The bug was [noticed](#31 (comment)) during testing the #31.

ACKs for top commit:
  TheCharlatan:
    ACK ab82110

Tree-SHA512: ca11ecfb0459defbbf29789b5a077600bbb74a4416f8e1cdad3fe722ea62e89d66102e5423bc91a69abdc6c18fa334745075d587dc28ab6521a099255ab90403
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants