refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a #26298

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/intclean changing 10 files +88 −116
  1. ryanofsky commented at 3:32 pm on October 12, 2022: contributor

    These belong in libbitcoin_common.a, not libbitcoin_util.a, because they aren’t general-purpose utilities, they just contain some common glue code that is used by both the node and the wallet. Another reason not to include these in libbitcoin_util.a is to prevent them from being used by the kernel library.

    Also rename ambiguous MakeHandler functions to MakeCleanupHandler and MakeSignalHandler. Cleanup function handler was introduced after boost signals handler, so original naming didn’t make much sense.

    This just contains a move-only commit, and a rename commit. There are no actual code or behavior changes.

    This PR is an alternative to #26293, and solves the same issue of removing a boost dependency from the util library. The advantages of this PR compared to #26293 are that it keeps the source directory structure more flat, and it avoids having to change #includes all over the codebase.

  2. DrahtBot added the label Refactoring on Oct 12, 2022
  3. DrahtBot commented at 10:39 pm on October 12, 2022: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hebasto
    Concept ACK fanquake

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #26177 (refactor / kernel: Move non-gArgs chainparams functionality to kernel by TheCharlatan)
    • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)

    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.

  4. fanquake commented at 5:19 am on October 13, 2022: member

    Concept ACK - closed #26293 in favour of this PR.

    and solves the same issue of removing a boost dependency from the util library.

    Want to pull in https://github.com/bitcoin/bitcoin/pull/26293/commits/c30a390760e3fe0af00d161fccfcdc93ae55f9dc or make a similar change here?

  5. ryanofsky force-pushed on Oct 13, 2022
  6. ryanofsky commented at 6:11 pm on October 13, 2022: contributor

    Updated ba4f6c501bf277ebfbeaec57ed25e65b8764d017 -> 879042a114fa60937ed9732d82986a17361a7dcf (pr/intclean.1 -> pr/intclean.2, compare) adding fanquake’s commit dropping boost dependency from util library

    Want to pull in c30a390 or make a similar change here?

    Thanks, done now

  7. ryanofsky marked this as ready for review on Oct 13, 2022
  8. hebasto commented at 8:02 pm on October 13, 2022: member
    Concept ACK.
  9. DrahtBot added the label Needs rebase on Oct 18, 2022
  10. ryanofsky force-pushed on Oct 18, 2022
  11. ryanofsky commented at 3:35 pm on October 18, 2022: contributor
    Rebased 879042a114fa60937ed9732d82986a17361a7dcf -> 8e5ab1b6433629ee2f5cf29c5ae24fd61155006c (pr/intclean.2 -> pr/intclean.3, compare) due to conflict with #26313
  12. DrahtBot removed the label Needs rebase on Oct 18, 2022
  13. fanquake requested review from theuni on Oct 19, 2022
  14. in src/interfaces/init.h:10 in 8e5ab1b643 outdated 
     4@@ -5,6 +5,12 @@
 5 #ifndef BITCOIN_INTERFACES_INIT_H
 6 #define BITCOIN_INTERFACES_INIT_H
 7 
 8+#include <interfaces/chain.h>
 9+#include <interfaces/echo.h>
10+#include <interfaces/init.h>
    hebasto commented at 4:20 pm on October 20, 2022:

    0087e2e36ddeec15adfce7cd72579a9a73e37540

    Why this header?

    ryanofsky commented at 4:25 pm on October 20, 2022:

    In commit “refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a” (0087e2e36ddeec15adfce7cd72579a9a73e37540)

    Why this header?

    Good catch, removed

  15. in src/interfaces/init.h:37 in 8e5ab1b643 outdated 
    42-    virtual Ipc* ipc();
43+    virtual std::unique_ptr<Node> makeNode() { return nullptr; }
44+    virtual std::unique_ptr<Chain> makeChain() { return nullptr; }
45+    virtual std::unique_ptr<WalletLoader> makeWalletLoader(Chain& chain) { return nullptr; }
46+    virtual std::unique_ptr<Echo> makeEcho() { return nullptr; }
47+    virtual Ipc* ipc() { return nullptr; }
    hebasto commented at 4:21 pm on October 20, 2022:

    0087e2e36ddeec15adfce7cd72579a9a73e37540

    Add #include <interfaces/ipc.h>?

    ryanofsky commented at 4:27 pm on October 20, 2022:

    In commit “refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a” (0087e2e36ddeec15adfce7cd72579a9a73e37540)

    Add #include <interfaces/ipc.h>?

    Thanks, include above was wrong but this is actually ok because of the forward declaration.

  16. ryanofsky force-pushed on Oct 20, 2022
  17. ryanofsky commented at 5:09 pm on October 20, 2022: contributor

    Thanks for the review!

    Updated 8e5ab1b6433629ee2f5cf29c5ae24fd61155006c -> 9b093fdacfd3a4b82ad2024edcb5f8ec24a55d42 (pr/intclean.3 -> pr/intclean.4, compare) with suggested cleanups

  18. hebasto commented at 9:16 pm on October 20, 2022: member
    Approach ACK 9b093fdacfd3a4b82ad2024edcb5f8ec24a55d42.
  19. in src/common/interfaces.cpp:9 in 9b093fdacf outdated 
     5+#include <interfaces/echo.h>
 6+#include <interfaces/handler.h>
 7+
 8+#include <boost/signals2/connection.hpp>
 9+#include <utility>
10+#include <memory>
    hebasto commented at 9:20 pm on October 20, 2022:

    pico-nit: clang-format-diff.py complains about header order:

    0#include <memory>
1#include <utility>
    ryanofsky commented at 10:52 pm on November 29, 2022:

    pico-nit: clang-format-diff.py complains about header order:

    Thanks, fixed

  20. hebasto commented at 9:09 am on October 21, 2022: member

    This PR needs to be built on top of 39adc3e90232f6e9aaf9d484007723a9136f0df0 from #26294. At least, on macOS M1.

    Or separate “build: remove BOOST_CPPFLAGS from libbitcoin_util” commit.

  21. refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a 
    These belong in libbitcoin_common.a, not libbitcoin_util.a, because they aren't
general-purpose utilities, they just contain common code that is used by both
the node and the wallet. Another reason to reason to not include these in
libbitcoin_util.a is to prevent them from being used by the kernel library.
    82e272a109
  22. build: remove BOOST_CPPFLAGS from libbitcoin_util dd6e8bd71c
  23. refactor: Rename ambiguous interfaces::MakeHandler functions b19c4124b3
  24. hebasto commented at 1:36 pm on November 29, 2022: member

    This PR needs to be built on top of 39adc3e from #26294. At least, on macOS M1.

    As #26294 has been merged, mind rebasing this PR?

  25. ryanofsky force-pushed on Nov 29, 2022
  26. ryanofsky commented at 10:56 pm on November 29, 2022: contributor

    As #26294 has been merged, mind rebasing this PR?

    Thanks, rebased now

    Rebased 9b093fdacfd3a4b82ad2024edcb5f8ec24a55d42 -> b19c4124b3c9a15fe3baf8792c55eb26eca51c0f (pr/intclean.4 -> pr/intclean.5, compare) after #26294 to fix macos build issue

  27. hebasto approved
  28. hebasto commented at 10:16 am on November 30, 2022: member
    ACK b19c4124b3c9a15fe3baf8792c55eb26eca51c0f
  29. fanquake merged this on Dec 7, 2022
  30. fanquake closed this on Dec 7, 2022
  31. sidhujag referenced this in commit 5ae4ee3fcc on Dec 8, 2022
  32. bitcoin locked this on Dec 7, 2023


ryanofsky DrahtBot fanquake hebasto


theuni

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me