-
Notifications
You must be signed in to change notification settings - Fork 38.7k
refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a #26298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
ba4f6c5 to
879042a
Compare
|
Updated ba4f6c5 -> 879042a (
Thanks, done now |
|
Concept ACK. |
879042a to
8e5ab1b
Compare
|
Rebased 879042a -> 8e5ab1b ( |
8e5ab1b to
9b093fd
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
Updated 8e5ab1b -> 9b093fd (pr/intclean.3 -> pr/intclean.4, compare) with suggested cleanups
hebasto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK 9b093fd.
|
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. |
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.
9b093fd to
b19c412
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As #26294 has been merged, mind rebasing this PR?
Thanks, rebased now
Rebased 9b093fd -> b19c412 (pr/intclean.4 -> pr/intclean.5, compare) after #26294 to fix macos build issue
hebasto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK b19c412
These belong in
libbitcoin_common.a, notlibbitcoin_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 inlibbitcoin_util.ais to prevent them from being used by the kernel library.Also rename ambiguous
MakeHandlerfunctions toMakeCleanupHandlerandMakeSignalHandler. 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.