Skip to content

Conversation

@ryanofsky
Copy link
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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 12, 2022

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.

@fanquake
Copy link
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 c30a390 or make a similar change here?

@ryanofsky
Copy link
Contributor Author

Updated ba4f6c5 -> 879042a (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

@ryanofsky ryanofsky marked this pull request as ready for review October 13, 2022 18:11
@hebasto
Copy link
Member

hebasto commented Oct 13, 2022

Concept ACK.

@ryanofsky
Copy link
Contributor Author

Rebased 879042a -> 8e5ab1b (pr/intclean.2 -> pr/intclean.3, compare) due to conflict with #26313

@fanquake fanquake requested a review from theuni October 19, 2022 12:36
Copy link
Contributor Author

@ryanofsky ryanofsky left a 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

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK 9b093fd.

@hebasto
Copy link
Member

hebasto commented Oct 21, 2022

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.

ryanofsky and others added 3 commits November 29, 2022 08:12
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.
@hebasto
Copy link
Member

hebasto commented Nov 29, 2022

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?

Copy link
Contributor Author

@ryanofsky ryanofsky left a 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

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b19c412

@fanquake fanquake merged commit 7d51560 into bitcoin:master Dec 7, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 8, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants