Page MenuHomePhabricator

[CMAKE] Drop dependency search for the native builds
ClosedPublic

Authored by Fabien on Oct 1 2020, 16:53.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABC15dc0ccc20c1: [CMAKE] Drop dependency search for the native builds
Summary

This is an alternative to D7552, intended to solve the need for
dependencies when building against the static depends.

This diff simply makes the dependency search conditional so it only
applies to the non native builds. It is limited to libevent and boost as
other dependencies already have an option to turn them off which
achieves the same.

Depends on D7816.

Test Plan

On Ubuntu Xenial:

cmake -GNinja .. -DCMAKE_TOOLCHAIN_FILE=../cmake/platforms/Linux64.cmake
ninja

Check the build succeeds.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested review of this revision.Oct 1 2020, 16:53

Snippet of first build failure:

[319/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/uint256.cpp.o
[320/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/pubkey.cpp.o
[321/482] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/consensus/tx_check.cpp.o
[322/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[323/482] Building CXX object src/CMakeFiles/script.dir/script/standard.cpp.o
[324/482] Building CXX object src/CMakeFiles/util.dir/util/system.cpp.o
[325/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[326/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[327/482] Building CXX object src/CMakeFiles/script.dir/script/sign.cpp.o
[328/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[329/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[330/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[331/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[332/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[333/482] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[334/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/db.cpp.o 
/usr/bin/cmake -E __run_co_compile --launcher=/usr/bin/ccache --tidy="/usr/bin/clang-tidy-8;-warnings-as-errors=*" --source=../../src/wallet/db.cpp -- /usr/bin/clang++  -DBOOST_AC_USE_STD_ATOMIC -DBOOST_SP_USE_STD_ATOMIC -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -isystem /usr/include/jemalloc -g -O2 -fPIC -fvisibility=hidden   -fstack-protector-all -Wstack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety-analysis -Wshadow -Wshadow-field -Wrange-loop-analysis -Wredundant-decls -Wformat-security -Wredundant-move -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -std=gnu++14 -MD -MT src/wallet/CMakeFiles/wallet.dir/db.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/db.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/db.cpp.o -c ../../src/wallet/db.cpp
/work/abc-ci-builds/build-clang-tidy/../../src/wallet/db.cpp:869:30: error: statement should be inside braces [readability-braces-around-statements,-warnings-as-errors]
                if (!fMockDb) dbenv->lsn_reset(strFile.c_str(), 0);
                             ^
                              {
2458 warnings generated.
Suppressed 2457 warnings (2457 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
[335/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[336/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[337/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqabstractnotifier.cpp.o
[338/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[339/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[340/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqnotificationinterface.cpp.o
[341/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/dns.cpp.o
[342/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/psbtwallet.cpp.o
[343/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/bitcoin.cpp.o
[344/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[345/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/db.cpp.o
[346/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[347/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[348/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[349/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqrpc.cpp.o
[350/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqpublishnotifier.cpp.o
[351/482] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[352/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[353/482] Building CXX object src/CMakeFiles/util.dir/util/time.cpp.o
[354/482] Linking CXX static library src/libutil.a
[355/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[356/482] Linking CXX static library src/librpcclient.a
[357/482] Linking CXX static library src/zmq/libzmq.a
[358/482] Linking CXX static library src/libscript.a
[359/482] Linking CXX static library src/libcommon.a
[360/482] Linking CXX static library src/libbitcoinconsensus.a
[361/482] Linking CXX static library src/seeder/libseeder.a
[362/482] Linking CXX shared library src/libbitcoinconsensus.so.0.22.4
[363/482] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[364/482] Linking CXX executable src/bitcoin-cli
[365/482] Linking CXX executable src/bitcoin-tx
[366/482] Linking CXX executable src/seeder/bitcoin-seeder
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1
deadalnix requested changes to this revision.Oct 6 2020, 13:03
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
cmake/modules/TestSuite.cmake
201 ↗(On Diff #24167)

The __ clearly indicate this is not for public consumption. You need to add some kind of API around this.

This revision now requires changes to proceed.Oct 6 2020, 13:03

Hide the private variable behind a wrapper macro.
Note: I tried another better API skip_if_native_build(content) which writes the content to a script file and includes it back but it has too many edge cases (especially due to variable expansion) to work reliably.

Set a couple options to drop the openssl requirement for native builds.

deadalnix requested changes to this revision.Oct 7 2020, 18:51
deadalnix added inline comments.
cmake/modules/TestSuite.cmake
204 ↗(On Diff #24302)

Add a comment as to why that is.

src/CMakeLists.txt
70 ↗(On Diff #24302)

This would have already landed if it was in its own patch.

577 ↗(On Diff #24302)

If you need to add a branch every time libevent is used, this is obviously going to be a pain in the ass to maintain.

src/secp256k1/CMakeLists.txt
283 ↗(On Diff #24302)

This can go in right away too.

This revision now requires changes to proceed.Oct 7 2020, 18:51

Rebase on top of D7816.

Add a wrapper function to search a library and link it only for non native builds.
This allow for simplifying Boost and Event linkage and avoid branch duplication.
This can be later extended to other libraries as well.

Add a comment to the test suite cmake file. It cannot use the wrapper for boost
because of the dynamic linkage check.

Fabien edited the summary of this revision. (Show Details)

Rebase

Ok he part you split are very good. This is also looking better. I need to think about it a bit more. I'm not super happy with that result, but I'm also not super happy with useless deps, so we might be reaching positive territory :)

This revision is now accepted and ready to land.Oct 9 2020, 13:33