Convert check-deps.sh to python#35167
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35167. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Not sure if we want to remove all the transitive deps from the md file, but without them it's a fair bit easier to follow, at least compared to master: %%{ init : { "flowchart" : { "curve" : "basis" }}}%%
graph TD;
bitcoin-cli[bitcoin-cli]-->libbitcoin_cli;
bitcoind[bitcoind]-->libbitcoin_node;
bitcoind[bitcoind]-->libbitcoin_wallet;
bitcoin-qt[bitcoin-qt]-->libbitcoinqt;
bitcoin-wallet[bitcoin-wallet]-->libbitcoin_wallet;
libbitcoin_cli-->libbitcoin_util;
libbitcoin_consensus-->libbitcoin_util;
libbitcoin_common-->libbitcoin_consensus;
libbitcoin_kernel-->libbitcoin_consensus;
libbitcoin_node-->libbitcoin_kernel;
libbitcoin_node-->libbitcoin_common;
libbitcoinqt-->libbitcoin_cli;
libbitcoinqt-->libbitcoin_node;
libbitcoinqt-->libbitcoin_wallet;
libbitcoin_util-->libbitcoin_crypto;
libbitcoin_wallet-->libbitcoin_common;
classDef bold stroke-width:2px, font-weight:bold, font-size: smaller;
class bitcoin-qt,bitcoind,bitcoin-cli,bitcoin-wallet bold
|
|
Nice, Concept ACK
I feel like this was brought up before - makes it indeed much cleaner. |
|
It's really nice to actually be able to read this now. Re the additional warning we get now:
ACK 24b0d4c |
| ``` | ||
| </td></tr><tr><td> | ||
|
|
||
| **Dependency graph**. Arrows show linker symbol dependencies. *Crypto* lib depends on nothing. *Util* lib is depended on by everything. *Kernel* lib depends only on consensus, crypto, and util. |
There was a problem hiding this comment.
| **Dependency graph**. Arrows show permitted linker symbol dependencies; the maximum each library is allowed to reference. Actual usage is a subset; the linker enforces nothing automatically. A permitted edge may be unexercised, which is not a bug and can be removed from the graph once verified. *Crypto* lib depends on nothing. *Util* lib is depended on by everything. *Kernel* lib depends only on consensus, crypto, and util. |
NIT: For me it was not clear that the depicted Dependency graph is more a "allow list" than an actual state of dependency usage. If you know you will read this distinction in the rest of the document but I think it would help to be a bit more clear for the casual reader / first time bitcoin dev-er. Text is nothing more than a suggestion.
| loc = parts[0] # lib:obj:addr or lib:obj: | ||
| sym_type = parts[1] | ||
| symbol = parts[2] |
There was a problem hiding this comment.
| loc = parts[0] # lib:obj:addr or lib:obj: | |
| sym_type = parts[1] | |
| symbol = parts[2] | |
| loc = parts[0] | |
| sym_type = parts[-2] | |
| symbol = parts[-1] |
NIT: there is a slight behavioral difference between nm on linux and on mac:
linux nm: "lib.a:obj.o: " (addr glued to obj:, blank for undefined)
Apple nm: "lib.a:obj.o: " (space after obj:, addr always its own token)
Both, the last two whitespace tokens are <type> <symbol>, NIT is to use that.
Details
nm linux:
nm -o -g build_dev_mode/lib/libbitcoin_util.a 2>/dev/null | head -8
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: U __assert_fail
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: U __cxa_allocate_exception
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: U __cxa_begin_catch
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: U __cxa_end_catch
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: U __cxa_free_exception
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: U __cxa_throw
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o:0000000000000000 V DW.ref.__gxx_personality_v0
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o:0000000000000000 V DW.ref._ZTIN10tinyformat12format_errorEnm macos:
nm -o -g build_dev_mode/lib/libbitcoin_util.a 2>/dev/null | head -8
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: U __Unwind_Resume
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: 00000000000012fc T __Z11DecodeAsmapN2fs4pathE
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: U __Z11GetMockTimev
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: 0000000000001d08 T __Z12AsmapVersionNSt3__14spanIKSt4byteLm18446744073709551615EEE
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: U __Z14SysErrorStringi
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: 0000000000000994 T __Z16SanityCheckAsmapNSt3__14spanIKSt4byteLm18446744073709551615EEEi
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: 0000000000000f48 T __Z18CheckStandardAsmapNSt3__14spanIKSt4byteLm18446744073709551615EEE
build_dev_mode/lib/libbitcoin_util.a:asmap.cpp.o: 0000000000000ffc T __Z22LogPrintFormatInternalIJEEvO14SourceLocationN5BCLog8LogFlagsEN4util3log5LevelEbNS4_21ConstevalFormatStringIXsZT_EEEDpRKT_script to check behavior:
Details
python3 -c "
import sys
sys.path.insert(0, 'contrib/devtools')
import importlib.util
spec = importlib.util.spec_from_file_location('cd', 'contrib/devtools/check-deps.py')
m = importlib.util.module_from_spec(spec); spec.loader.exec_module(m)
defined, undefined = m.get_symbols_per_obj('build_dev_mode/lib/libbitcoin_util.a')
print('defined symbols :', sum(len(v) for v in defined.values()))
print('undefined symbols:', sum(len(v) for v in undefined.values()))
print('sample defined :', sorted(next(iter(defined.values())))[:2])
"
on macos, before:
defined symbols captured : 0
undefined symbols captured: 1104
After:
defined symbols : 902
undefined symbols: 1104
There was a problem hiding this comment.
It would have also failed on macos with the shell script, right?
There was a problem hiding this comment.
Switched to a regex that should also work on macos
There was a problem hiding this comment.
It would have also failed on macos with the shell script, right?
Yes, well even worse it would give a "false" succes report.
I have always considered I don't think it can be considered the "source of truth" at this stage of development. For example, |
These init functions are only used by node implementations, so the special case in dep-check can be removed.
Neither the shell or python check-deps script check binary dependencies -- binaries without all their deps will just fail to run anyway, so there's not much point. Even if they were checked, the transitive resolution in the python script would cover this case -- bitcoind depends on libbitcoin_node which depends on libbitcoin_common. (bitcoind would still need to explicitly link to libbitcoin_common presuming libbitcoin_node doesn't publicly incorporate libbitcoin_common though) |
24b0d4c to
3c0e2f5
Compare
janb84
left a comment
There was a problem hiding this comment.
Concept ACK 3c0e2f5
Big improvement for MacOs usage, it will no-longer report a false succes :
Master:
Warning: suppression 'common.cpp.o interface_ui.cpp.o _Z9InitErrorRK13bilingual_str' was ignored, consider deleting.
Warning: suppression 'common.cpp.o interface_ui.cpp.o _Z11InitWarningRK13bilingual_str' was ignored, consider deleting.
Success! No unexpected dependencies were detected.New Python based script (--no-build to reuse master build):
./contrib/devtools/check-deps.py --no-build
Warning: libraries not found: bitcoin_kernel, bitcoinqt
Error: bitcoin_common -> bitcoin_node (2 symbols)
common.cpp.o -> interface_ui.cpp.o: InitWarning(bilingual_str const&)
common.cpp.o -> interface_ui.cpp.o: InitError(bilingual_str const&)
(use --show-suppressions to see suppress directives)
2 unexpected dependencies found.| for target in declared_deps: | ||
| result = subprocess.run( | ||
| ["cmake", "--build", str(build_dir), "-j", str(os.cpu_count()), "-t", target], | ||
| capture_output=True, text=True |
There was a problem hiding this comment.
Not sure about suppressing build output, this deviates from the shell script variant and it somewhat confusing that "nothing" happens in default mode. (but it's building in the background)
There was a problem hiding this comment.
Concept ACK 3c0e2f5 moving the ./contrib/devtools/check-deps.sh to Python
The previous check-deps.sh was transparent about triggering a rebuild, while the new check-deps.py builds silently in the background with no feedback, which feels confusing
Master (before PR)
Running `./contrib/devtools/check-deps.sh`
git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
brandon-odiwuor@brandon-odiwuor-ubuntu:~/bitcoin/bitcoin$ ./contrib/devtools/check-deps.sh build_dev_mode/
-- Could NOT find XKB (missing: XKB_LIBRARY XKB_INCLUDE_DIR) (Required is at least version "0.5.0")
-- Could NOT find Doxygen (missing: DOXYGEN_EXECUTABLE dot)
Configuring secp256k1 subtree...
-- Could NOT find Valgrind (missing: Valgrind_INCLUDE_DIR Valgrind_WORKS)
secp256k1 configure summary
===========================
Build artifacts:
library type ........................ Static
Optional modules:
ECDH ................................ OFF
ECDSA pubkey recovery ............... ON
extrakeys ........................... ON
schnorrsig .......................... ON
musig ............................... ON
ElligatorSwift ...................... ON
Parameters:
ecmult window size .................. 15
ecmult gen table size ............... 86 KiB
Optional features:
assembly ............................ x86_64
external callbacks .................. OFF
Optional binaries:
benchmark ........................... OFF
noverify_tests ...................... ON
tests ............................... ON
exhaustive tests .................... ON
ctime_tests ......................... OFF
examples ............................ OFF
Cross compiling ....................... FALSE
API visibility attributes ............. OFF
Valgrind .............................. OFF
Preprocessor defined macros ........... ECMULT_WINDOW_SIZE=15 COMB_BLOCKS=43 COMB_TEETH=6 USE_ASM_X86_64=1
C compiler ............................ GNU 13.3.0, /usr/bin/cc
CFLAGS ................................
Compile options ....................... -Wall -pedantic -Wcast-align -Wcast-align=strict -Wextra -Wnested-externs -Wno-long-long -Wno-overlength-strings -Wno-unused-function -Wshadow -Wstrict-prototypes -Wundef
Build type:
- CMAKE_BUILD_TYPE ................... RelWithDebInfo
- CFLAGS ............................. -O2 -g
- LDFLAGS for executables ............
- LDFLAGS for shared libraries .......
Configure summary
=================
Executables:
bitcoin ............................. ON
bitcoind ............................ ON
bitcoin-node (multiprocess) ......... ON
bitcoin-qt (GUI) .................... ON
bitcoin-gui (GUI, multiprocess) ..... ON
bitcoin-cli ......................... ON
bitcoin-tx .......................... ON
bitcoin-util ........................ ON
bitcoin-wallet ...................... ON
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
Optional features:
wallet support ...................... ON
external signer ..................... ON
ZeroMQ .............................. ON
IPC ................................. ON
Embedded ASMap ...................... ON
USDT tracing ........................ ON
QR code (GUI) ....................... ON
DBus (GUI) .......................... ON
Tests:
test_bitcoin ........................ ON
test_bitcoin-qt ..................... ON
bench_bitcoin ....................... ON
fuzz binary ......................... ON
Cross compiling ....................... FALSE
C++ compiler .......................... GNU 13.3.0, /usr/bin/c++
CMAKE_BUILD_TYPE ...................... RelWithDebInfo
Preprocessor defined macros ...........
C++ compiler flags .................... -O2 -g -std=c++20 -fPIC -fno-extended-identifiers -fmacro-prefix-map=/home/brandon-odiwuor/bitcoin/bitcoin/src=. -fstack-reuse=none -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter
Linker flags .......................... -O2 -g -fstack-reuse=none -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie
NOTE: The summary above may not exactly match the final applied build flags
if any additional CMAKE_* or environment variables have been modified.
To see the exact flags applied, build with the --verbose option.
Treat compiler warnings as errors ..... OFF
Use ccache for compiling .............. ON
-- Configuring done (42.1s)
-- Generating done (4.5s)
-- Build files have been written to: /home/brandon-odiwuor/bitcoin/bitcoin/build_dev_mode
[ 0%] Generating bitcoin-build-info.h
[ 0%] Built target generate_build_info
[ 0%] Building CXX object src/CMakeFiles/bitcoin_node.dir/rpc/mempool.cpp.o
[ 0%] Linking CXX static library ../lib/libbitcoin_node.a
[100%] Built target bitcoin_node
Built target bitcoin_cli
[100%] Built target bitcoin_consensus
[ 0%] Generating bitcoin-build-info.h
[ 0%] Built target generate_build_info
[100%] Built target bitcoin_wallet
[ 0%] Generating bitcoin-build-info.h
[ 0%] Built target generate_build_info
[ 0%] Building CXX object src/CMakeFiles/bitcoin_common.dir/init/common.cpp.o
[ 0%] Linking CXX static library ../lib/libbitcoin_common.a
[100%] Built target bitcoin_common
[ 0%] Generating bitcoin-build-info.h
[ 0%] Built target generate_build_info
[100%] Built target bitcoin_util
[100%] Built target bitcoin_crypto
Success! No unexpected dependencies were detected.PR at 3c0e2f5
Running `./contrib/devtools/check-deps.py`
brandon-odiwuor@brandon-odiwuor-ubuntu:~/bitcoin/bitcoin$ rm -rf build_dev_mode/
brandon-odiwuor@brandon-odiwuor-ubuntu:~/bitcoin/bitcoin$ cmake --preset dev-mode
Preset CMake variables:
BUILD_BENCH="ON"
BUILD_CLI="ON"
BUILD_DAEMON="ON"
BUILD_FUZZ_BINARY="ON"
BUILD_GUI="ON"
BUILD_GUI_TESTS="ON"
BUILD_KERNEL_LIB="ON"
BUILD_SHARED_LIBS="ON"
BUILD_TESTS="ON"
BUILD_TX="ON"
BUILD_UTIL="ON"
BUILD_UTIL_CHAINSTATE="ON"
BUILD_WALLET_TOOL="ON"
ENABLE_EXTERNAL_SIGNER="ON"
ENABLE_IPC="ON"
ENABLE_WALLET="ON"
WITH_QRENCODE="ON"
WITH_USDT="ON"
WITH_ZMQ="ON"
-- The CXX compiler identification is GNU 13.3.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Setting build type to "RelWithDebInfo" as none was specified
-- Performing Test CXX_SUPPORTS__WERROR
-- Performing Test CXX_SUPPORTS__WERROR - Success
-- Performing Test CXX_SUPPORTS__G3
-- Performing Test CXX_SUPPORTS__G3 - Success
-- Performing Test LINKER_SUPPORTS__G3
-- Performing Test LINKER_SUPPORTS__G3 - Success
-- Performing Test CXX_SUPPORTS__FTRAPV
-- Performing Test CXX_SUPPORTS__FTRAPV - Success
-- Performing Test LINKER_SUPPORTS__FTRAPV
-- Performing Test LINKER_SUPPORTS__FTRAPV - Success
-- Found SQLite3: /usr/include (found suitable version "3.45.1", minimum required is "3.7.17")
-- Found PkgConfig: /usr/bin/pkg-config (found version "1.8.1")
-- Found ZeroMQ: /usr/lib/x86_64-linux-gnu (found suitable version "4.3.5", minimum required is "4.0.0")
-- Performing Test HAVE_USDT_H
-- Performing Test HAVE_USDT_H - Success
-- Found USDT: /usr/include/x86_64-linux-gnu
-- Found QRencode: /usr/lib/x86_64-linux-gnu/libqrencode.so (found version "4.1.1")
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Performing Test HAVE_STDATOMIC
-- Performing Test HAVE_STDATOMIC - Success
-- Found WrapAtomic: TRUE
-- Found OpenGL: /usr/lib/x86_64-linux-gnu/libOpenGL.so
-- Found WrapOpenGL: TRUE
-- Could NOT find XKB (missing: XKB_LIBRARY XKB_INCLUDE_DIR) (Required is at least version "0.5.0")
-- Found WrapVulkanHeaders: /usr/include
-- Found Qt: /usr/lib/x86_64-linux-gnu/cmake/Qt6 (found suitable version "6.4.2", minimum required is "6.2")
-- Performing Test LINKER_SUPPORTS__WL___FATAL_WARNINGS
-- Performing Test LINKER_SUPPORTS__WL___FATAL_WARNINGS - Success
-- Performing Test FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION
-- Performing Test FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION - Failed
-- Performing Test NO_DIAGNOSTICS_BOOST_NO_CXX98_FUNCTION_BASE
-- Performing Test NO_DIAGNOSTICS_BOOST_NO_CXX98_FUNCTION_BASE - Success
-- Found Libevent: /usr/lib/x86_64-linux-gnu (found suitable version "2.1.12-stable", minimum required is "2.1.8")
-- Performing Test HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR
-- Performing Test HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR - Failed
-- Looking for O_CLOEXEC
-- Looking for O_CLOEXEC - found
-- Looking for fdatasync
-- Looking for fdatasync - found
-- Looking for fork
-- Looking for fork - found
-- Looking for pipe2
-- Looking for pipe2 - found
-- Looking for setsid
-- Looking for setsid - found
-- Performing Test IFADDR_LINKS_WITHOUT_LIBSOCKET
-- Performing Test IFADDR_LINKS_WITHOUT_LIBSOCKET - Success
-- Performing Test STD_ATOMIC_LINKS_WITHOUT_LIBATOMIC
-- Performing Test STD_ATOMIC_LINKS_WITHOUT_LIBATOMIC - Success
-- Looking for std::system
-- Looking for std::system - found
-- Looking for ::_wsystem
-- Looking for ::_wsystem - not found
-- Performing Test STRERROR_R_CHAR_P
-- Performing Test STRERROR_R_CHAR_P - Success
-- Looking for malloc_info
-- Looking for malloc_info - found
-- Performing Test HAVE_MALLOPT_ARENA_MAX
-- Performing Test HAVE_MALLOPT_ARENA_MAX - Success
-- Performing Test HAVE_POSIX_FALLOCATE
-- Performing Test HAVE_POSIX_FALLOCATE - Success
-- Performing Test HAVE_STRONG_GETAUXVAL
-- Performing Test HAVE_STRONG_GETAUXVAL - Success
-- Performing Test HAVE_SOCKADDR_UN
-- Performing Test HAVE_SOCKADDR_UN - Success
-- Performing Test HAVE_GETRANDOM
-- Performing Test HAVE_GETRANDOM - Success
-- Performing Test HAVE_GETENTROPY_RAND
-- Performing Test HAVE_GETENTROPY_RAND - Success
-- Performing Test HAVE_SYSCTL
-- Performing Test HAVE_SYSCTL - Failed
-- Performing Test HAVE_SYSCTL_ARND
-- Performing Test HAVE_SYSCTL_ARND - Failed
-- Performing Test HAVE_SSE41
-- Performing Test HAVE_SSE41 - Success
-- Performing Test HAVE_AVX2
-- Performing Test HAVE_AVX2 - Success
-- Performing Test HAVE_X86_SHANI
-- Performing Test HAVE_X86_SHANI - Success
-- Performing Test HAVE_ARM_SHANI
-- Performing Test HAVE_ARM_SHANI - Failed
-- Performing Test CXX_SUPPORTS__WALL
-- Performing Test CXX_SUPPORTS__WALL - Success
-- Performing Test CXX_SUPPORTS__WEXTRA
-- Performing Test CXX_SUPPORTS__WEXTRA - Success
-- Performing Test CXX_SUPPORTS__WGNU
-- Performing Test CXX_SUPPORTS__WGNU - Failed
-- Performing Test CXX_SUPPORTS__WCOVERED_SWITCH_DEFAULT
-- Performing Test CXX_SUPPORTS__WCOVERED_SWITCH_DEFAULT - Failed
-- Performing Test CXX_SUPPORTS__WFORMAT__WFORMAT_SECURITY
-- Performing Test CXX_SUPPORTS__WFORMAT__WFORMAT_SECURITY - Success
-- Performing Test CXX_SUPPORTS__WVLA
-- Performing Test CXX_SUPPORTS__WVLA - Success
-- Performing Test CXX_SUPPORTS__WSHADOW_FIELD
-- Performing Test CXX_SUPPORTS__WSHADOW_FIELD - Failed
-- Performing Test CXX_SUPPORTS__WTHREAD_SAFETY
-- Performing Test CXX_SUPPORTS__WTHREAD_SAFETY - Failed
-- Performing Test CXX_SUPPORTS__WTHREAD_SAFETY_POINTER
-- Performing Test CXX_SUPPORTS__WTHREAD_SAFETY_POINTER - Failed
-- Performing Test CXX_SUPPORTS__WLOOP_ANALYSIS
-- Performing Test CXX_SUPPORTS__WLOOP_ANALYSIS - Failed
-- Performing Test CXX_SUPPORTS__WREDUNDANT_DECLS
-- Performing Test CXX_SUPPORTS__WREDUNDANT_DECLS - Success
-- Performing Test CXX_SUPPORTS__WUNUSED_MEMBER_FUNCTION
-- Performing Test CXX_SUPPORTS__WUNUSED_MEMBER_FUNCTION - Failed
-- Performing Test CXX_SUPPORTS__WDATE_TIME
-- Performing Test CXX_SUPPORTS__WDATE_TIME - Success
-- Performing Test CXX_SUPPORTS__WCONDITIONAL_UNINITIALIZED
-- Performing Test CXX_SUPPORTS__WCONDITIONAL_UNINITIALIZED - Failed
-- Performing Test CXX_SUPPORTS__WDUPLICATED_BRANCHES
-- Performing Test CXX_SUPPORTS__WDUPLICATED_BRANCHES - Success
-- Performing Test CXX_SUPPORTS__WDUPLICATED_COND
-- Performing Test CXX_SUPPORTS__WDUPLICATED_COND - Success
-- Performing Test CXX_SUPPORTS__WLOGICAL_OP
-- Performing Test CXX_SUPPORTS__WLOGICAL_OP - Success
-- Performing Test CXX_SUPPORTS__WOVERLOADED_VIRTUAL
-- Performing Test CXX_SUPPORTS__WOVERLOADED_VIRTUAL - Success
-- Performing Test CXX_SUPPORTS__WSUGGEST_OVERRIDE
-- Performing Test CXX_SUPPORTS__WSUGGEST_OVERRIDE - Success
-- Performing Test CXX_SUPPORTS__WIMPLICIT_FALLTHROUGH
-- Performing Test CXX_SUPPORTS__WIMPLICIT_FALLTHROUGH - Success
-- Performing Test CXX_SUPPORTS__WUNREACHABLE_CODE
-- Performing Test CXX_SUPPORTS__WUNREACHABLE_CODE - Success
-- Performing Test CXX_SUPPORTS__WDOCUMENTATION
-- Performing Test CXX_SUPPORTS__WDOCUMENTATION - Failed
-- Performing Test CXX_SUPPORTS__WSELF_ASSIGN
-- Performing Test CXX_SUPPORTS__WSELF_ASSIGN - Failed
-- Performing Test CXX_SUPPORTS__WBIDI_CHARS_ANY
-- Performing Test CXX_SUPPORTS__WBIDI_CHARS_ANY - Success
-- Performing Test CXX_SUPPORTS__WUNDEF
-- Performing Test CXX_SUPPORTS__WUNDEF - Success
-- Performing Test CXX_SUPPORTS__WLEADING_WHITESPACE_SPACES
-- Performing Test CXX_SUPPORTS__WLEADING_WHITESPACE_SPACES - Failed
-- Performing Test CXX_SUPPORTS__WTRAILING_WHITESPACE_ANY
-- Performing Test CXX_SUPPORTS__WTRAILING_WHITESPACE_ANY - Failed
-- Performing Test CXX_SUPPORTS__WUNUSED_PARAMETER
-- Performing Test CXX_SUPPORTS__WUNUSED_PARAMETER - Success
-- Performing Test CXX_SUPPORTS__FNO_EXTENDED_IDENTIFIERS
-- Performing Test CXX_SUPPORTS__FNO_EXTENDED_IDENTIFIERS - Success
-- Performing Test CXX_SUPPORTS__FMACRO_PREFIX_MAP_A_B
-- Performing Test CXX_SUPPORTS__FMACRO_PREFIX_MAP_A_B - Success
-- Performing Test CXX_SUPPORTS__FSTACK_REUSE_NONE
-- Performing Test CXX_SUPPORTS__FSTACK_REUSE_NONE - Success
-- Performing Test LINKER_SUPPORTS__FSTACK_REUSE_NONE
-- Performing Test LINKER_SUPPORTS__FSTACK_REUSE_NONE - Success
-- Performing Test CXX_SUPPORTS__U_FORTIFY_SOURCE__D_FORTIFY_SOURCE_3_2d08
-- Performing Test CXX_SUPPORTS__U_FORTIFY_SOURCE__D_FORTIFY_SOURCE_3_2d08 - Success
-- Performing Test LINKER_SUPPORTS__U_FORTIFY_SOURCE__D_FORTIFY_SOURCE_3_2d08
-- Performing Test LINKER_SUPPORTS__U_FORTIFY_SOURCE__D_FORTIFY_SOURCE_3_2d08 - Success
-- Performing Test CXX_SUPPORTS__WSTACK_PROTECTOR
-- Performing Test CXX_SUPPORTS__WSTACK_PROTECTOR - Success
-- Performing Test CXX_SUPPORTS__FSTACK_PROTECTOR_ALL
-- Performing Test CXX_SUPPORTS__FSTACK_PROTECTOR_ALL - Success
-- Performing Test LINKER_SUPPORTS__FSTACK_PROTECTOR_ALL
-- Performing Test LINKER_SUPPORTS__FSTACK_PROTECTOR_ALL - Success
-- Performing Test CXX_SUPPORTS__FCF_PROTECTION_FULL
-- Performing Test CXX_SUPPORTS__FCF_PROTECTION_FULL - Success
-- Performing Test LINKER_SUPPORTS__FCF_PROTECTION_FULL
-- Performing Test LINKER_SUPPORTS__FCF_PROTECTION_FULL - Success
-- Performing Test CXX_SUPPORTS__FSTACK_CLASH_PROTECTION
-- Performing Test CXX_SUPPORTS__FSTACK_CLASH_PROTECTION - Success
-- Performing Test LINKER_SUPPORTS__FSTACK_CLASH_PROTECTION
-- Performing Test LINKER_SUPPORTS__FSTACK_CLASH_PROTECTION - Success
-- Performing Test LINKER_SUPPORTS__WL___ENABLE_RELOC_SECTION
-- Performing Test LINKER_SUPPORTS__WL___ENABLE_RELOC_SECTION - Failed
-- Performing Test LINKER_SUPPORTS__WL___DYNAMICBASE
-- Performing Test LINKER_SUPPORTS__WL___DYNAMICBASE - Failed
-- Performing Test LINKER_SUPPORTS__WL___NXCOMPAT
-- Performing Test LINKER_SUPPORTS__WL___NXCOMPAT - Failed
-- Performing Test LINKER_SUPPORTS__WL___HIGH_ENTROPY_VA
-- Performing Test LINKER_SUPPORTS__WL___HIGH_ENTROPY_VA - Failed
-- Performing Test LINKER_SUPPORTS__WL__Z_RELRO
-- Performing Test LINKER_SUPPORTS__WL__Z_RELRO - Success
-- Performing Test LINKER_SUPPORTS__WL__Z_NOW
-- Performing Test LINKER_SUPPORTS__WL__Z_NOW - Success
-- Performing Test LINKER_SUPPORTS__WL__Z_SEPARATE_CODE
-- Performing Test LINKER_SUPPORTS__WL__Z_SEPARATE_CODE - Success
-- Found Python3: /usr/bin/python3 (found suitable version "3.12.3", minimum required is "3.10") found components: Interpreter
-- Could NOT find Doxygen (missing: DOXYGEN_EXECUTABLE dot)
-- Performing Test HAVE_BUILTIN_PREFETCH
-- Performing Test HAVE_BUILTIN_PREFETCH - Success
-- Performing Test HAVE_MM_PREFETCH
-- Performing Test HAVE_MM_PREFETCH - Success
-- Performing Test HAVE_SSE42
-- Performing Test HAVE_SSE42 - Success
-- Performing Test HAVE_ARM64_CRC32C
-- Performing Test HAVE_ARM64_CRC32C - Failed
-- Looking for F_FULLFSYNC
-- Looking for F_FULLFSYNC - not found
-- Performing Test HAVE_CLMUL
-- Performing Test HAVE_CLMUL - Success
-- Performing Test HAVE_PTHREAD_GETNAME_NP
-- Performing Test HAVE_PTHREAD_GETNAME_NP - Success
-- Performing Test HAVE_PTHREAD_THREADID_NP
-- Performing Test HAVE_PTHREAD_THREADID_NP - Failed
-- Performing Test HAVE_PTHREAD_GETTHREADID_NP
-- Performing Test HAVE_PTHREAD_GETTHREADID_NP - Failed
-- The C compiler identification is GNU 13.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
Configuring secp256k1 subtree...
-- Performing Test HAVE_X86_64_ASM
-- Performing Test HAVE_X86_64_ASM - Success
-- Could NOT find Valgrind (missing: Valgrind_INCLUDE_DIR Valgrind_WORKS)
-- Performing Test C_SUPPORTS__WALL
-- Performing Test C_SUPPORTS__WALL - Success
-- Performing Test C_SUPPORTS__PEDANTIC
-- Performing Test C_SUPPORTS__PEDANTIC - Success
-- Performing Test C_SUPPORTS__WCAST_ALIGN
-- Performing Test C_SUPPORTS__WCAST_ALIGN - Success
-- Performing Test C_SUPPORTS__WCAST_ALIGN_STRICT
-- Performing Test C_SUPPORTS__WCAST_ALIGN_STRICT - Success
-- Performing Test C_SUPPORTS__WCONDITIONAL_UNINITIALIZED
-- Performing Test C_SUPPORTS__WCONDITIONAL_UNINITIALIZED - Failed
-- Performing Test C_SUPPORTS__WEXTRA
-- Performing Test C_SUPPORTS__WEXTRA - Success
-- Performing Test C_SUPPORTS__WLEADING_WHITESPACE_SPACES
-- Performing Test C_SUPPORTS__WLEADING_WHITESPACE_SPACES - Failed
-- Performing Test C_SUPPORTS__WNESTED_EXTERNS
-- Performing Test C_SUPPORTS__WNESTED_EXTERNS - Success
-- Performing Test C_SUPPORTS__WNO_LONG_LONG
-- Performing Test C_SUPPORTS__WNO_LONG_LONG - Success
-- Performing Test C_SUPPORTS__WNO_OVERLENGTH_STRINGS
-- Performing Test C_SUPPORTS__WNO_OVERLENGTH_STRINGS - Success
-- Performing Test C_SUPPORTS__WNO_UNUSED_FUNCTION
-- Performing Test C_SUPPORTS__WNO_UNUSED_FUNCTION - Success
-- Performing Test C_SUPPORTS__WRESERVED_IDENTIFIER
-- Performing Test C_SUPPORTS__WRESERVED_IDENTIFIER - Failed
-- Performing Test C_SUPPORTS__WSHADOW
-- Performing Test C_SUPPORTS__WSHADOW - Success
-- Performing Test C_SUPPORTS__WSTRICT_PROTOTYPES
-- Performing Test C_SUPPORTS__WSTRICT_PROTOTYPES - Success
-- Performing Test C_SUPPORTS__WTRAILING_WHITESPACE_ANY
-- Performing Test C_SUPPORTS__WTRAILING_WHITESPACE_ANY - Failed
-- Performing Test C_SUPPORTS__WUNDEF
-- Performing Test C_SUPPORTS__WUNDEF - Success
-- Looking for sys/types.h
-- Looking for sys/types.h - found
-- Looking for sys/wait.h
-- Looking for sys/wait.h - found
-- Looking for unistd.h
-- Looking for unistd.h - found
secp256k1 configure summary
===========================
Build artifacts:
library type ........................ Static
Optional modules:
ECDH ................................ OFF
ECDSA pubkey recovery ............... ON
extrakeys ........................... ON
schnorrsig .......................... ON
musig ............................... ON
ElligatorSwift ...................... ON
Parameters:
ecmult window size .................. 15
ecmult gen table size ............... 86 KiB
Optional features:
assembly ............................ x86_64
external callbacks .................. OFF
Optional binaries:
benchmark ........................... OFF
noverify_tests ...................... ON
tests ............................... ON
exhaustive tests .................... ON
ctime_tests ......................... OFF
examples ............................ OFF
Cross compiling ....................... FALSE
API visibility attributes ............. OFF
Valgrind .............................. OFF
Preprocessor defined macros ........... ECMULT_WINDOW_SIZE=15 COMB_BLOCKS=43 COMB_TEETH=6 USE_ASM_X86_64=1
C compiler ............................ GNU 13.3.0, /usr/bin/cc
CFLAGS ................................
Compile options ....................... -Wall -pedantic -Wcast-align -Wcast-align=strict -Wextra -Wnested-externs -Wno-long-long -Wno-overlength-strings -Wno-unused-function -Wshadow -Wstrict-prototypes -Wundef
Build type:
- CMAKE_BUILD_TYPE ................... RelWithDebInfo
- CFLAGS ............................. -O2 -g
- LDFLAGS for executables ............
- LDFLAGS for shared libraries .......
-- Performing Test CXX_SUPPORTS__WUNIQUE_OBJECT_DUPLICATION
-- Performing Test CXX_SUPPORTS__WUNIQUE_OBJECT_DUPLICATION - Failed
Configure summary
=================
Executables:
bitcoin ............................. ON
bitcoind ............................ ON
bitcoin-node (multiprocess) ......... ON
bitcoin-qt (GUI) .................... ON
bitcoin-gui (GUI, multiprocess) ..... ON
bitcoin-cli ......................... ON
bitcoin-tx .......................... ON
bitcoin-util ........................ ON
bitcoin-wallet ...................... ON
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
Optional features:
wallet support ...................... ON
external signer ..................... ON
ZeroMQ .............................. ON
IPC ................................. ON
Embedded ASMap ...................... ON
USDT tracing ........................ ON
QR code (GUI) ....................... ON
DBus (GUI) .......................... ON
Tests:
test_bitcoin ........................ ON
test_bitcoin-qt ..................... ON
bench_bitcoin ....................... ON
fuzz binary ......................... ON
Cross compiling ....................... FALSE
C++ compiler .......................... GNU 13.3.0, /usr/bin/c++
CMAKE_BUILD_TYPE ...................... RelWithDebInfo
Preprocessor defined macros ...........
C++ compiler flags .................... -O2 -g -std=c++20 -fPIC -fno-extended-identifiers -fmacro-prefix-map=/home/brandon-odiwuor/bitcoin/bitcoin/src=. -fstack-reuse=none -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter
Linker flags .......................... -O2 -g -fstack-reuse=none -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie
NOTE: The summary above may not exactly match the final applied build flags
if any additional CMAKE_* or environment variables have been modified.
To see the exact flags applied, build with the --verbose option.
Treat compiler warnings as errors ..... OFF
Use ccache for compiling .............. ON
-- Configuring done (30.8s)
-- Generating done (0.5s)
-- Build files have been written to: /home/brandon-odiwuor/bitcoin/bitcoin/build_dev_mode
brandon-odiwuor@brandon-odiwuor-ubuntu:~/bitcoin/bitcoin$ ./contrib/devtools/check-deps.py build_dev_mode/
Warning: targets not buildable: bitcoin_kernel
Success! No unexpected dependencies detected.|
@ryanofsky you might be interested in reviewing here? |
ryanofsky
left a comment
There was a problem hiding this comment.
Code review 3c0e2f5 and concept ACK.
These are very nice changes. It's a great idea to parse dependencies from libraries.md, and to not require manually listing transitive dependencies.
In theory, there could be cases where we might not want to allow transitive dependencies, but if you look at the list of implicit dependencies which are newly allowed as a result of this PR, they all seem acceptable even if they are unnecessary:
Implicit transitive dependencies (allowed transitively, not declared directly):
bitcoin_cli -> bitcoin_consensus
bitcoin_cli -> bitcoin_crypto
bitcoin_wallet -> bitcoin_consensus
I do think there are some changes that should be made to this PR before it is merged. I left various comments below with suggestions, and also applied all the suggestions in a branch you should feel free to use: https://github.com/ryanofsky/bitcoin/commits/pr/dep2
| libbitcoin_cli-->libbitcoin_util; | ||
| libbitcoin_cli-->libbitcoin_common; | ||
|
|
||
| libbitcoin_consensus-->libbitcoin_util; |
There was a problem hiding this comment.
In commit "doc: Update library dependencies" (8a74832)
The dependency doesn't really make sense to add. Consensus library was meant to be a very minimal library and not need things in util.
There is no problem dropping this dependency for release builds, but for debug builds I needed to add a suppression for the Assume used in merkle.cpp, similar to the suppression the PR already adds for muhash.cpp.o:
bitcoin/contrib/devtools/check-deps.py
Line 30 in 9877c09
There was a problem hiding this comment.
The dependency doesn't really make sense to add. Consensus library was meant to be a very minimal library and not need things in util.
No strong opinion on the implementation details, but I think devs should be able to use Assume/Assert or other low-level utils in consensus. Also linking to your earlier comment #34520 (comment):
Another option would to just let the consensus library depend on util. I don't think there would be harm from that if consensus is not distributed separately.
😅
There is no problem dropping this dependency for release builds, but for debug builds I needed to add a suppression for the Assume
Seems fine, if those are meant as temporary suppressions, with the goal of fixing it with a proper dependency in one way or another.
There was a problem hiding this comment.
On the earlier comment, just because something isn't harmful doesn't make it a good idea.
To fix the suppression, I'd be most inclined to switch from Assume to assert, or maybe just make the consensus code part of kernel if there is no longer a clear distinction between consensus & kernel libraries. Different approaches seem fine though.
|
|
||
| libbitcoinqt-->libbitcoin_cli; | ||
| libbitcoinqt-->libbitcoin_node; | ||
| libbitcoinqt-->libbitcoin_wallet; |
There was a problem hiding this comment.
In commit "doc: Update library dependencies" (8a74832)
These dependencies are not great to add, and it would be better to use suppressions instead. GUI code is meant to use src/interfaces classes to call node and wallet code. Or, in cases where common functions (like psbt functions) need to be shared by GUI/node/wallet libraries, they should be moved to bitcoin_common.
GUI code calling node and wallet functions is a problem because it could indicate improper access to node/wallet internals, and create difficulties for other GUI's like the QML port. Also, the GUI should not depend on bitcoin_cli since it is not an RPC client.
In my branch I suggested keeping check-deps behavior unchanged and not checking bitcoinqt for now:
bitcoin/contrib/devtools/check-deps.py
Lines 43 to 47 in 9877c09
| for target in declared_deps: | ||
| result = subprocess.run( | ||
| ["cmake", "--build", str(build_dir), "-j", str(os.cpu_count()), "-t", target], | ||
| capture_output=True, text=True |
There was a problem hiding this comment.
In commit "ci: Convert check-deps to python and use design doc dependencies directly" (5d3c538)
Not important, but would be nice to drop this line so build progress is visible like in the previous script.
Alternately if the goal is actually to suppress build output, maybe don't do it when --verbose is specified, and consider using stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL instead of capturing and later discarding.
| if result.returncode != 0: | ||
| not_buildable.append(target) | ||
| if not_buildable: | ||
| print(f"Warning: targets not buildable: {', '.join(not_buildable)}", file=sys.stderr) |
There was a problem hiding this comment.
In commit "ci: Convert check-deps to python and use design doc dependencies directly" (5d3c538)
Since this is run in CI it doesn't seem good to silently just warn about targets that fail to build.
It also doesn't seem good to try to build targets that don't exist, like this currently does with bitcoin_kernel.
In my suggested branch, I skip building bitcoin_kernel:
bitcoin/contrib/devtools/check-deps.py
Lines 37 to 41 in 9877c09
and change both warnings about missing libraries to errors
bitcoin/contrib/devtools/check-deps.py
Line 194 in 9877c09
bitcoin/contrib/devtools/check-deps.py
Line 219 in 9877c09
| core_io.cpp | ||
| deploymentinfo.cpp | ||
| external_signer.cpp | ||
| init/common.cpp |
There was a problem hiding this comment.
In commit "refactor: Move init/common.cpp to libbitcoin_node" (9399bf3)
I don't think it's good to remove init/common.cpp from libbitcoin_common and move it to libbitcoin_node, because this prevents binaries other than bitcoind from being able to call StartLogging and take advantage of logging.
Currently, we don't have other command line binaries that use logging, but in #30437 StartLogging is called to make IPC messages visible in the bitcoin-mine client, and in #10102 it is called from the bitcoin-wallet wallet subprocess to log wallet messages. Both of these PR's use aa78d98, which basically does the opposite of this commit, moving the depended-on file from libbitcoin_node to libbitcoin_common instead of the other way around.
So suggestion here is just to replace this commit with aa78d98
|
@ajtowns there's been some pending review here for a while. Can you follow up on the comments and questions? I still think the changes here are basically fine as is, but it might give other reviewers more confidence if the pending questions are answered. |
Converts check-deps.sh to python so that it can use the mermaid graph from doc/design/libraries.md as its source of truth, as well as automatically resolve transitive dependencies and demangle C++ names.