Details

Reviewers

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

Branch

cmake_no_native_dependencies

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 13260
Build 26561: Build Diff	lint-circular-dependencies · build-without-wallet · build-clang · build-diff · build-clang-tidy
Build 26560: arc lint + arc unit

Event Timeline

Fabien created this revision.Oct 1 2020, 16:53

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 1 2020, 16:53

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

Harbormaster completed remote builds in B13128: Diff 24156.Oct 1 2020, 17:05

Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

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

Rebase

Harbormaster completed remote builds in B13136: Diff 24167.Oct 1 2020, 18:48

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.

Harbormaster completed remote builds in B13209: Diff 24302.Oct 6 2020, 14:40

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

Fabien edited the summary of this revision. (Show Details)Oct 8 2020, 07:29

Fabien added a parent revision: D7816: [CMAKE] Drop native build dependency on Openssl.

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.

Harbormaster completed remote builds in B13260: Diff 24380.Oct 8 2020, 07:48

Fabien edited the summary of this revision. (Show Details)Oct 8 2020, 12:25

Rebase

Fabien removed a parent revision: D7816: [CMAKE] Drop native build dependency on Openssl.Oct 8 2020, 12:25

Fabien edited the test plan for this revision. (Show Details)

Fabien added a parent revision: D7816: [CMAKE] Drop native build dependency on Openssl.

Harbormaster completed remote builds in B13264: Diff 24387.Oct 8 2020, 12:45

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 :)

deadalnix accepted this revision.Oct 9 2020, 13:33

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

Closed by commit rABC15dc0ccc20c1: [CMAKE] Drop dependency search for the native builds (authored by Fabien). · Explain WhyOct 9 2020, 13:49

This revision was automatically updated to reflect the committed changes.

Fabien added a commit: rABC15dc0ccc20c1: [CMAKE] Drop dependency search for the native builds.

[CMAKE] Drop dependency search for the native builds
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 24380

cmake/modules/NativeExecutable.cmake

cmake/modules/TestSuite.cmake

src/CMakeLists.txt

[CMAKE] Drop dependency search for the native buildsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 24380

cmake/modules/NativeExecutable.cmake

cmake/modules/TestSuite.cmake

src/CMakeLists.txt

[CMAKE] Drop dependency search for the native builds
ClosedPublic
Actions

Revision Contents
Changeset List