Conversation
|
Concept ACK. |
|
|
||
| ### Miscellaneous | ||
| - [Assets Attribution](assets-attribution.md) | ||
| - [Assumeutxo design](assumeutxo.md) |
There was a problem hiding this comment.
Maybe add a doc/design/README.md (although the above description doesn't add much to just a directory view)
There was a problem hiding this comment.
re: #24352 (comment)
Maybe add a
doc/design/README.md(although the above description doesn't add much to just a directory view)
I think I'd hold off on adding another readme for now. It seems like it would be pretty bare and even if there were things to say, they might be more helpful and visible in the main readme. Could rethink in the future as more things are added here
| - Libraries should generally be careful about what other libraries they depend on, and only reference symbols following the arrows shown in the dependency graph below: | ||
|
|
||
| ```mermaid | ||
| graph TD; |
There was a problem hiding this comment.
This looks a bit scary with all these curved lines. It's probably better to use (a reasonably chosen set of) fixed positions and straight lines.
There was a problem hiding this comment.
Yeah this graph is atrocious. I thought mermaid might do a better job with it. I think the markdown source code is more undertandable than the graphic, but I will look for mermaid options (straight lines!) which might make this more readable.
There was a problem hiding this comment.
FWIW, I figured out how to make the lines straight. I thinks it cuts down some of the chaos and helps a little. I also added a caption to the graph to call out more important aspects.
There was a problem hiding this comment.
Flow chart still looks confusing. I tried creating some flowcharts using mermaid live editor based on syntax shared here. I was able to simplify bitcoind, bitcoin-qt and bitcoin-wallet but not other things (used different color for binaries):
flowchart TB
subgraph 1
d[bitcoind]-->libbitcoin_wallet
end
subgraph 2
qt[bitcoin-qt]-->libbitcoin_node
qt[bitcoin-qt]-->libbitcoinqt
qt[bitcoin-qt]-->libbitcoin_wallet
end
subgraph 3
w[bitcoin-wallet]-->libbitcoin_wallet
w[bitcoin-wallet]-->libbitcoin_wallet_tool
end
classDef teal fill:#033E3E,stroke:#AFDCEC,stroke-width:1px;
class d,qt,w tealflowchart TB
subgraph 1
d[bitcoind]-->libbitcoin_wallet
end
subgraph 2
qt[bitcoin-qt]-->libbitcoin_node
qt[bitcoin-qt]-->libbitcoinqt
qt[bitcoin-qt]-->libbitcoin_wallet
end
subgraph 3
w[bitcoin-wallet]-->libbitcoin_wallet
w[bitcoin-wallet]-->libbitcoin_wallet_tool
end
classDef teal fill:#033E3E,stroke:#AFDCEC,stroke-width:1px;
class d,qt,w teal
There was a problem hiding this comment.
re: #24352 (comment)
Thanks! I applied some of the styling here to highlight the exe outputs, though I avoided black text on teal background, because that was not readable on my PC. I also experimented with subgraphs, but trying to apply them to complete graph always seemed to blow up the graph. Probably there is room to do fancier things here in the future, but for now I'm happy if graph just shows what symbol dependencies between libraries are allowed.
There was a problem hiding this comment.
though I avoided black text on teal background, because that was not readable on my PC
Just realized it looks different in github (light theme). Text is white in github (dark theme)
Probably there is room to do fancier things here in the future, but for now I'm happy if graph just shows what symbol dependencies between libraries are allowed.
Makes sense.
|
I had a conversation with Ryan about this and agree to the broad strokes of things. We decided that it would be good to move:
I need to make that move and see where things fall to make an educated assessment of the hierarchy described here (mostly the part about |
Implemented this here: https://github.com/dongcarl/bitcoin/commits/2022-03-libbitcoinkernel-utilcommon-restructure Seems to me like a nice dividing line between util and common. Would love more Concept ACKs |
|
Concept ACK (on adding docs for this) - will look at the actual content. |
jamesob
left a comment
jamesob
left a comment
There was a problem hiding this comment.
ACK
Not sure how I missed this when it was filed, but I think this is really valuable documentation - thanks for writing it up! I also don't think the mermaid graph looks too bad, and I think it's certainly preferably to maintaining some kind of in-repo image. I say we keep it.
I think the constraints you outline later in the document (module X should never depend on module Y etc.) is very helpful information - can it be enforced somehow at the build system level? Obviously we'd know if there were ever violations because it'd be clear (I think) from Makefile changes, but wondering if there's some extra step we could take.
doc/design/libraries.md
Outdated
| | Name | Description | | ||
| |------------------------|-------------| | ||
| | libbitcoin_cli | RPC client functionality used by `bitcoin-cli` executable | | ||
| | libbitcoin_common | Home for common functionality shared by different executables and libraries. Similar to `libbitcoin_util`, but higher-level (see [Dependencies](#dependencies). | |
There was a problem hiding this comment.
Missing a closing paren on this line
There was a problem hiding this comment.
doc/design/libraries.md
Outdated
| | libbitcoinqt | GUI functionality used by `bitcoin-qt` and `bitcoin-gui` executables | | ||
| | libbitcoin_ipc | IPC functionality used by `bitcoin-node`, `bitcoin-wallet`, `bitcoin-gui` executables to communicate when [`--enable-multiprocess`](multiprocess.md) is used. | | ||
| | libbitcoin_node | P2P and RPC server functionality used by `bitcoind` and `bitcoin-qt` executables. | | ||
| | libbitcoin_util | Home for common functionality shared by different executables and libraries. Similar to `libbitcoin_common`, but lower-level (see [Dependencies](#dependencies). | |
There was a problem hiding this comment.
There was a problem hiding this comment.
Updated 4131c5f -> d076c18 (pr/libs.1 -> pr/libs.2, compare) with various edits and some clarifications about kernel and util and common libs
Updated d076c18 -> 93fd8d9 (pr/libs.2 -> pr/libs.3, compare) with minor edits
re: #24352 (review)
I think the constraints you outline later in the document (module X should never depend on module Y etc.) is very helpful information - can it be enforced somehow at the build system level?
Probably it would not be too hard to write a linter to check this. It could run nm src/libbitcoin_util.a etc, look at all the symbols each library defines (T), and all the symbols each library uses (U), and make sure libraries only uses symbols from libraries they are allowed to depend on.
|
|
||
| ### Miscellaneous | ||
| - [Assets Attribution](assets-attribution.md) | ||
| - [Assumeutxo design](assumeutxo.md) |
There was a problem hiding this comment.
re: #24352 (comment)
Maybe add a
doc/design/README.md(although the above description doesn't add much to just a directory view)
I think I'd hold off on adding another readme for now. It seems like it would be pretty bare and even if there were things to say, they might be more helpful and visible in the main readme. Could rethink in the future as more things are added here
doc/design/libraries.md
Outdated
| | Name | Description | | ||
| |------------------------|-------------| | ||
| | libbitcoin_cli | RPC client functionality used by `bitcoin-cli` executable | | ||
| | libbitcoin_common | Home for common functionality shared by different executables and libraries. Similar to `libbitcoin_util`, but higher-level (see [Dependencies](#dependencies). | |
There was a problem hiding this comment.
doc/design/libraries.md
Outdated
| | libbitcoinqt | GUI functionality used by `bitcoin-qt` and `bitcoin-gui` executables | | ||
| | libbitcoin_ipc | IPC functionality used by `bitcoin-node`, `bitcoin-wallet`, `bitcoin-gui` executables to communicate when [`--enable-multiprocess`](multiprocess.md) is used. | | ||
| | libbitcoin_node | P2P and RPC server functionality used by `bitcoind` and `bitcoin-qt` executables. | | ||
| | libbitcoin_util | Home for common functionality shared by different executables and libraries. Similar to `libbitcoin_common`, but lower-level (see [Dependencies](#dependencies). | |
There was a problem hiding this comment.
| - Libraries should generally be careful about what other libraries they depend on, and only reference symbols following the arrows shown in the dependency graph below: | ||
|
|
||
| ```mermaid | ||
| graph TD; |
There was a problem hiding this comment.
FWIW, I figured out how to make the lines straight. I thinks it cuts down some of the chaos and helps a little. I also added a caption to the graph to call out more important aspects.
ryanofsky
changed the title
|
ACK 93fd8d9 Looks reasonable to me! |
|
|
||
| <table><tr><td> | ||
|
|
||
| ```mermaid |
There was a problem hiding this comment.
Huh TIL, had no idea github could do this. Cool.
|
ACK, this matches with my understanding of the dependencies and library uses. At some point we could add something about the exported APIs of the shared libraries, but no need to do that here. |
|
ACK dc1e7ad |
9 participants
Prompted by the libbitcoinkernel issue #24303 and PRs, I started looking at existing libraries and what their dependencies are and wrote this document to describe them and where
libbitcoinkernelfits in.Readable link is: https://github.com/ryanofsky/bitcoin/blob/pr/libs/doc/design/libraries.md
Feedback is welcome