Fix check-deps.sh to check for weak symbols so it can detect when an exported template function like is used from another library.
Also update the script to work with cmake and configure it to run as part of CI.
Problem was reported by hebasto in #29015 (comment)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, hebasto |
| Stale ACK | m3dwards |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
57@@ -58,6 +58,10 @@ SUPPRESS["libbitcoin_common_a-common.o libbitcoin_node_a-interface_ui.o _Z9InitE
58 # rpc/external_signer.cpp adds defines node RPC methods but is built as part of the
59 # common library. It should be moved to the node library instead.
60 SUPPRESS["libbitcoin_common_a-external_signer.o libbitcoin_node_a-server.o _ZN9CRPCTable13appendCommandERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEPK11CRPCCommand"]=1
61+# pubkey.cpp in consensus library currently calls ParseHex in util to set
62+# NUMS_H_DATA constant, but this dependency can be eliminated in #30377 so
re: #30415 (review)
nit: Maybe make a reference to a PR consistent with the previous one?
Thanks, updated
pr/weakcheck.1 -> pr/weakcheck.2, compare) making comment more consistent
./contrib/devtools/check-deps.sh in the source directory. The script will automatically call make to build the bitcoin libraries and should show Success! No unexpected dependencies were detected. if successful.
Seems like if a script such as this is going to exist, it should be getting run as part of the CI, so that regressions are actually caught (if that’s what we want to do)?
note: libbitcoin_crypto_arm_shani is missing from LIBS[crypto] crypto.
Seems like if a script such as this is going to exist, it should be getting run as part of the CI, so that regressions are actually caught (if that’s what we want to do)?
I think it would be nice to run as part of CI. I added a commit to try that.
If we think the script is a maintenance burden, it is also not essential to keep. I wrote it mainly to help write #29015 to figure out which parts of the util library should be moved to common, crypto, and consensus libraries.
note:
libbitcoin_crypto_arm_shaniis missing fromLIBS[crypto]crypto.
Not sure if this might be expected but when I try that on my system I see:
0crypto/sha256_arm_shani.cpp:16:10: fatal error: arm_acle.h: No such file or directory
1 16 | #include <arm_acle.h>
Added 1 commits 114b2a406e604747bd856f566aa8c7ad84dd8f15 -> dde57405e99c00dcf511ebedd42deb2de5e67e5f (pr/weakcheck.2 -> pr/weakcheck.3, compare) to try to run check-deps script in CI
If we think the script is a maintenance burden, it is also not essential to keep. I wrote it mainly to help write #29015 to figure out which parts of the util library should be moved to common, crypto, and consensus libraries.
I think it is worth having this script to prevent re-introducing undocumented inter-library dependencies.
150@@ -151,6 +151,10 @@ if [ -n "$USE_VALGRIND" ]; then
151 "${BASE_ROOT_DIR}/ci/test/wrap-valgrind.sh"
152 fi
153
154+if [ "$RUN_CHECK_DEPS" = "true" ]; then
155+ contrib/devtools/check-deps.sh
pr/weakcheck.3 -> pr/weakcheck.4, compare) to fix ci failure https://cirrus-ci.com/task/5809606310494208 “/ci_container_base/ci/test/03_test_script.sh: line 155: contrib/devtools/check-deps.sh: No such file or directory” predicted by maflcko
Added 1 commits 07509a4b691b4250c8e085d247dad9ddb4e5baa5 -> d21803b08e51dd8812116ba8f8d8ad6009b5d0fc (pr/weakcheck.4 -> pr/weakcheck.5, compare) adding debug commit to debug silent failure https://cirrus-ci.com/task/5394916199628800
Updated d21803b08e51dd8812116ba8f8d8ad6009b5d0fc -> 0c83d553a528ba8b736747d92ad7e266370018ae (pr/weakcheck.5 -> pr/weakcheck.6, compare) fixing verbose failure
https://cirrus-ci.com/task/4801670352207872
Updated 0c83d553a528ba8b736747d92ad7e266370018ae -> 2bca4af323c5b505689c5203e6ddba8becac5dc8 (pr/weakcheck.6 -> pr/weakcheck.7, compare) dropping verbose output after success in https://cirrus-ci.com/task/6691470202109952
ACK 2bca4af323c5b505689c5203e6ddba8becac5dc8
Tested on Debian x86.
With the suppression disabled I got:
0Error: libbitcoin_consensus_a-pubkey.o depends on libbitcoin_util_a-strencodings.o symbol 'std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsigned char>(std::basic_string_view<char, std::char_traits<char> >)'
57@@ -58,6 +58,10 @@ SUPPRESS["libbitcoin_common_a-common.o libbitcoin_node_a-interface_ui.o _Z9InitE
58 # rpc/external_signer.cpp adds defines node RPC methods but is built as part of the
59 # common library. It should be moved to the node library instead.
60 SUPPRESS["libbitcoin_common_a-external_signer.o libbitcoin_node_a-server.o _ZN9CRPCTable13appendCommandERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEPK11CRPCCommand"]=1
61+# pubkey.cpp in consensus library currently calls ParseHex in util to set
62+# NUMS_H_DATA constant, but this suppression can be dropped when runtime
63+# dependency is dropped in https://github.com/bitcoin/bitcoin/issues/30377
Fix check-deps.sh to check for weak symbols so it can detect when an exported
template function is used from another library.
In a previous version of this commit, this change caused an invalid dependency
in the consensus library on the TryParseHex template function from the util
library to be detected, and a suppression was added here. But #30377 removed
the invalid dependency so the suppression is no longer needed.
The invalid dependency and problem detecting weak symbol usage was originally
reported by Hennadii Stepanov in
https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2209258843
Script was failing when called on libraries that do not import symbols, because
bash pipefail option was specified, and grep was used in some pipelines to
filter symbols, and grep returns status 1 when it doesn't match any lines. This
could cause the script to fail on some systems and configurations, such as the
clang-tidy CI configuration
https://cirrus-ci.com/task/4801670352207872?logs=ci#L6191 where the
libbitcoin_crypto_x86_shani.a library does not import symbols.
pr/weakcheck.7 -> pr/weakcheck.8, compare) updating this to work with cmake and dropping suppression no longer needed after #30377
38 # Add minor dependencies omitted from doc/design/libraries.md to keep the
39 # dependency diagram simple.
40 ALLOWED_DEPENDENCIES+=(
41 "wallet consensus"
42- "wallet_tool common"
43- "wallet_tool crypto"
What happened to wallet_tool in the latest push?
2 dependencies exist according to /doc/design/libraries.md:
0libbitcoin_wallet_tool-->libbitcoin_wallet;
1libbitcoin_wallet_tool-->libbitcoin_util;
I didn’t dig into it but it appeared wallet_tool library disappeared in the conversion to cmake…
I couldn’t bring myself to create a library for just a single object file. :)
For the reference, the library was defined as follows: https://github.com/bitcoin/bitcoin/blob/80f00cafdeef0600fa1a5e906686720786c2336c/src/Makefile.am#L547-L549
pr/weakcheck.8 -> pr/weakcheck.9, compare) to fix failing CI job due to changed default build directory (https://cirrus-ci.com/task/5141392714891264?logs=ci#L5325)
190@@ -194,6 +191,7 @@ cd "$BUILD_DIR"
191 # shellcheck disable=SC2046
192 make -j"$(nproc)" $(lib_targets)
cmake --build over make .
re: #30415 (review)
I think we are trying to consistently use
cmake --buildovermake.
Thanks, switched to cmake –build
pr/weakcheck.9 -> pr/weakcheck.10, compare) with suggested change to use cmake –build wrapper instead of calling make directly
pr/weakcheck.10 -> pr/weakcheck.11, compare) simplifying by dropping cd since cmake –build ignores the current directory.
Fixing this looks good. I guess going forward it currently has the caveat that it’s expected to work most correctly in the CI (on the same hardware used by the CI). i.e this will fail for any dev trying to run the script locally, on aarch64:
0[ 25%] Built target bitcoin_crypto_arm_shani
1[100%] Built target bitcoin_crypto
2gmake: *** No rule to make target 'bitcoin_crypto_x86_shani'. Stop.
or if a dev compiles on x86_64, but doesn’t match the CI config, they may see incorrect output i.e:
0[100%] Built target bitcoin_crypto_sse41
1Built target bitcoin_crypto_avx2
2Warning: suppression 'init.cpp.o bdb.cpp.o _ZN6wallet27BerkeleyDatabaseSanityCheckEv' was ignored, consider deleting.
3Success! No unexpected dependencies were detected.
This seems fine for now, as long as weak symbols/regressions are caught, and could be followed up on.