This came up in #26196: #26196 (comment). Opening this as a draft for the minute, to point to from another PR.
Ultimately the goal here (aside from common quite possibly being a better home for this code) is to move the Boost Signals2 using code out of libbitcoin_util, which is/will be used by the kernel.
When this is done, we can drop $(BOOST_CPPFLAGS) from libbitcoin_util (last commit).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
I think something should be done about the inconsistency of having libbitcoin_common.a source files outside of the src/common/ and this is one possible option. Other possible options are:
src/interface/ files to a libbitcoin_interfaces.a librarysrc/interface/*.cpp files and make src/interfaces/ directory only contain headersI think I like these options a little better, because they would keep source structure flat and avoid too much nesting. They would also avoid namespace inconsistencies or the need to have a nested common::interfaces:: namespace that would make function declarations in headers more verbose.
I want look into option 2 a little more since it seems like the 3 .cpp files in the interfaces directory are very minimal and don’t contain much real code.
As you mentioned, I agree that we should go for a less nested option when possible.
Now considering the alternatives you mentioned, Option 2 seems like a pretty interesting idea. I thought it to be a preferable option because it seemed a cleaner way to achieve our goal.
However, on second thought, what if we need to expand src/interfaces in the future for some reason, and going with a header-only route for the expansion might be unreasonable? So we might have to move to an option similar to your first suggestion.
This might be an infeasible scenario, and there might exist other options than the two you mentioned to achieve this goal, which I haven’t considered. But I just wanted to explain why I think the second option, though cleaner, might not be feasible in the long run.
I implemented option 2 above in #26298. It changes fewer files than this PR and avoids the nested directory structure.
However, on second thought, what if we need to expand
src/interfacesin the future for some reason, and going with a header-only route for the expansion might be unreasonable?
I don’t see this happening, just because the point of src/interfaces/ directory is just to define abstract classes with virtual methods. Implementation of the interfaces belongs in the node, wallet, or common libraries, or in the GUI. If implementation code sneaks into the interfaces directory it’s probably a mistake. Or if it is really intentional, it could logically go into a libbitcoin_interfaces.a library that without requiring all the interfaces to move somewhere.
because they would keep source structure flat and avoid too much nesting.
Is this something we are actively trying to avoid? i.e in #26250 (review) we seem to be going the other direction.
In any case, closing this in favour of #26298.
because they would keep source structure flat and avoid too much nesting.
Is this something we are actively trying to avoid? i.e in #26250 (comment) we seem to be going the other direction.
I think both changes make sense. In that case, the file mempool_utils.cpp is renamed util/mempool.cpp, so it’s really just swapping out a _ separator for the / separator, not actually adding a deeper level of nesting. I think that change is an improvement because it groups mempool utils with other utils instead of grouping them with mempool tests. It should make mempool utils more discoverable so they can be used by tests using mempool indirectly, not just tests directly testing the mempool.
In this case, this PR isn’t just changing a _ separator to a / separator or :: separator, but really adding an extra level to the hierarchy, renaming “interfaces” to “common interfaces” where I think the word “common” isn’t helpful descriptively, and isn’t great organizationally since it groups wallet and node interface definitions in with libbitcoin_common library code that shouldn’t know anything about nodes or wallets. It could also make headers more verbose with identifiers like common::interfaces::Wallet instead of interfaces::Wallet, or introducing an inconsistency between the directory structure and namespace structure.
The idea for src/interfaces/ is that it defines abstract interfaces that libraries can use to call each other without depending on each other at link time. Since #20494, the libbitcoin_node library provides Node and Chain implementations, the libbitcoin_wallet library provides Wallet and WalletLoader implementations. #26298 just extends #20494 so the libbitcoin_common provides now provides Handler implementations.
Hope that makes sense.