#35167 Convert check-deps.sh to python
| Reviewability | Reviews | Agreement | Size | |||||||
|---|---|---|---|---|---|---|---|---|---|---|
| #35167 Convert check-deps.sh to python | Author silent 186d | 1 | Blocked | 5 + 291 tests | ||||||
|
|
|
|
| ||||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | ||||||||||
Goal
Replaces contrib/devtools/check-deps.sh with a Python implementation that reads library dependencies directly from doc/design/libraries.md Mermaid diagrams. The new script resolves transitive dependencies automatically, demangles symbol names with c++filt, and handles nm output formatting differences on macOS. It also moves init/common.cpp from libbitcoin_common to libbitcoin_node in CMakeLists.txt to eliminate a suppression.
Problem: The existing shell script required manual tracking of transitive library dependencies, suffered from symbol parsing bugs on macOS that produced false positives or silent passes, and had to keep a separate dependency list in sync with the documentation.
Reviewability
Stale · Author silent 186d
No author activity for 186 days; a review may wait a long time for a response.
Waiting on (from the thread read):
- author: address ryanofsky's review objections and rebase 2026-10-30
- author: rebase and addressing review objections from ryanofsky 2026-10-30
Agreement
Blocked
objection that needs a change, nobody has replied (ryanofsky)
Thread notes: Split rc:3276154300 into two separate objections (CI error handling and non-existent targets). Added missing suggestions from janb84, sedited, and ryanofsky, updated rc:3154874411 to clears_with: change / accepted, and marked rc:3275886532 as contested following pushback against maflcko's reply. / Split rc:3276154300 into two claims for CI warnings and building nonexistent targets. Recorded suggestions on build output suppression and CLI dependencies, updated rc:3275886532 to contested following
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| sedited (member) | suggestion, approach | remove the libbitcoin_cli -> libbitcoin_util dependency edge and add bitcoin-cli -> libbitcoin_util instead | open | answered | no | 2026-04-28: 'maybe we can add an edge for `bitcoin-cli`->`libbitcoin_util` and remove the existing `libbitcoin_cli`->`libbitcoin_util` edge?' |
| hebasto (member) | objection, approach | treating libraries.md as the source of truth causes confusion because binary dependencies are not reflected in the graph | answered · author explained scope of script and hebasto did not follow up | answered | yes | 2026-04-28: 'I don't think it can be considered the "source of truth" at this stage of development.' Status from: 2026-04-29: ajtowns: '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.' |
| janb84 (member) | suggestion, maintenance | clarify in libraries.md that the dependency graph illustrates permitted linker dependencies (an allowlist) rather than current actual usage | open | answered | no | 2026-04-28: 'For me it was not clear that the depicted Dependency graph is more a "allow list" than an actual state of dependency usage.' |
| janb84 (member) | objection, correctness | Apple nm output formatting difference causes defined symbols to be missed on macOS, leading to false success passes | accepted · fix pushed · author pushed regex fix and reviewer confirmed it resolves the false success | changed | yes | 2026-04-28: 'there is a slight behavioral difference between nm on linux and on mac' Status from: 2026-04-29: janb84: 'Big improvement for MacOs usage, it will no-longer report a false succes' |
| janb84 (member) | suggestion, interface | do not suppress build output by default as it is confusing to have nothing visible while building | open | answered | no | 2026-04-29: 'Not sure about suppressing build output, this deviates from the shell script variant and it somewhat confusing that "nothing" happens in default mode.' |
| ryanofsky (member) | objection, approach | adding a consensus to util dependency breaks the design goal of consensus being a minimal library independent of util | contested · ryanofsky reiterated the objection after maflcko suggested allowing low-level utils in consensus | changed | by others | 2026-05-20: '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.' Status from: 2026-05-21: ryanofsky: 'just because something isn't harmful doesn't make it a good idea' |
| ryanofsky (member) | objection, approach | adding direct GUI dependencies on node, wallet, and cli could allow improper access to internals and creates difficulties for alternative GUIs | open | changed | no | 2026-05-20: '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.' |
| ryanofsky (member) | suggestion, interface | make build progress visible or avoid suppressing output when --verbose is specified | open | answered | no | 2026-05-20: 'would be nice to drop this line so build progress is visible like in the previous script' |
| ryanofsky (member) | objection, correctness | check-deps silently warns instead of failing in CI when targets fail to build | open | changed | no | 2026-05-20: 'Since this is run in CI it doesn't seem good to silently just warn about targets that fail to build.' |
| ryanofsky (member) | objection, correctness | check-deps attempts to build non-existent targets such as bitcoin_kernel | open | changed | no | 2026-05-20: 'It also doesn't seem good to try to build targets that don't exist, like this currently does with `bitcoin_kernel`.' |
| ryanofsky (member) | objection, approach | moving init/common.cpp from libbitcoin_common to libbitcoin_node prevents auxiliary binaries like bitcoin-wallet and bitcoin-mine from using logging | open | changed | no | 2026-05-20: '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`' |
Support:
- sedited (member) (Concept ACK, 2026-04-27) [read: doc/design/libraries.md]: makes it indeed much cleaner
- sedited (member) (ACK, 2026-04-28) [read: contrib/devtools/check-deps.py, doc/design/libraries.md]: It's really nice to actually be able to read this now.
- janb84 (member) (Concept ACK, 2026-04-28) [read: contrib/devtools/check-deps.py]: Brings the script more in line with the programming language used for the other tools.
- janb84 (member) (Concept ACK, 2026-04-29) [tested: contrib/devtools/check-deps.py]: Big improvement for MacOs usage, it will no-longer report a false succes
- BrandonOdiwuor (contributor) (Concept ACK, 2026-04-30) [tested: contrib/devtools/check-deps.py]: moving the ./contrib/devtools/check-deps.sh to Python
- sedited (member) (ACK, 2026-05-01): Re-ACK 3c0e2f544ecc9097661baaac0ade1a4ea6ad6506 [not substantive]
- ryanofsky (member) (Concept ACK, 2026-05-20) [both: contrib/devtools/check-deps.py, doc/design/libraries.md, src/CMakeLists.txt]: It's a great idea to parse dependencies from libraries.md, and to not require manually listing transitive dependencies.
- stickies-v (member) (Concept ACK, 2026-09-18): Concept ACK [not substantive]
Participants:
- sedited (member, support, 5 statements 2026-04-27 to 2026-10-30): Concept ACK and multiple ACKs; reminded author of pending reviews and need to rebase
- hebasto (member, objection, 1 statements 2026-04-28 to 2026-04-28): objected to treating libraries.md as source of truth (answered)
- janb84 (member, objection, 6 statements 2026-04-28 to 2026-04-29): Concept ACK; reported macOS nm parsing bug (accepted fix), suggested showing build output and clarifying doc text
- BrandonOdiwuor (contributor, support, 1 statements 2026-04-30 to 2026-04-30): Concept ACK; noted silent background builds feel confusing
- fanquake (member, neutral, 1 statements 2026-05-20 to 2026-05-20): pinged ryanofsky for review
- ryanofsky (member, objection, 7 statements 2026-05-20 to 2026-05-21): Concept ACK with blocking objections on library dependencies, CI error handling, and init/common.cpp move
- maflcko (member, neutral, 1 statements 2026-05-21 to 2026-05-21): discussed Assume usage in consensus library in reply to ryanofsky
- stickies-v (member, support, 1 statements 2026-09-18 to 2026-09-18): Concept ACK
Files
489 lines under test/bench/ci.
- contrib/devtools/check-deps.py +290/-0
- contrib/devtools/check-deps.sh +0/-197
- doc/design/libraries.md +4/-20
- ci/test/03_test_script.sh +1/-1
- src/CMakeLists.txt +1/-1
Card
The short text the ranking pass compares across PRs.
Converts check-deps.sh to a Python script that parses the Mermaid dependency graph directly from doc/design/libraries.md. This automates transitive dependency resolution, demangles C++ symbols, and resolves macOS-specific false positives in nm output. Reviewers broadly favor the Python rewrite, but ryanofsky identified concerns about moving init/common.cpp to libbitcoin_node and adding unnecessary dependency edges to the design document. The PR currently needs a rebase and the author has been silent for several months.
Data
thread read through 2026-10-30 (27 statements), head 3c0e2f544e
labels: Needs rebase
Categories
Build and CI
P3 · platform fix
Resolves macOS symbol parsing bugs in CI dependency checks that caused silent passes or false positives. As noted in the description, the Python rewrite fixes nm formatting parsing on macOS and automates transitive dependency checks, improving CI linting reliability.
Membership: Modifies ci/test/03_test_script.sh, src/CMakeLists.txt, and the library dependency checker used in CI.
Factors: security/stability 0, bug 1, performance 0, user value 0, leverage 1
Tools and scripts
P3 · cleanup (ranking pass: Moved down: maintainability of a developer dependency check with unresolved design-doc objections and a stale branch ranks below concrete tool bugs and user-facing tooling in the same band.)
Rewrites a fragile shell utility into a more maintainable Python script for developers checking library modularity boundaries. Reviewers note it makes the script much cleaner to read and directly parses dependencies from design documentation rather than requiring manual tracking.
Membership: Rewrites contrib/devtools/check-deps.sh into Python as contrib/devtools/check-deps.py.
Factors: security/stability 0, bug 1, performance 0, user value 0, leverage 1