kernel: move RunCommandParseJSON to its own file

[theuni]

Because libbitcoinkernel does not include this new object, this has the side-effect of eliminating its unnecessary boost::process dependency.

This leaves libbitcoinkernel with 3 remaining boost dependencies:

• boost::date_time for util/time.cpp, which I'll separate out next. Exactly like this PR.
• boost::signals2 for which I have a POC re-implementation here: https://github.com/theuni/bitcoin/commits/replace-boost-signals
• boost::multi_index which I'm not sure about yet.

[maflcko]

Concept ACK

[theuni]

boost::date_time for util/time.cpp, which I'll separate out next. Exactly like this PR.

@fanquake mentioned to me today that he has this done already in a local branch, so I'll leave that to him to PR it at some point.

[fanquake]

Concept ACK

fanquake mentioned to me today that he has this done already in a local branch, so I'll leave that to him to PR it at some point.

See #26198.

boost::multi_index which I'm not sure about yet.

I have a larger Boost reduction branch, that builds on your signals2 implementation, that might a starting point for this.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22417 (util/system: Close non-std fds when execing slave processes by luke-jr)

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.

[hebasto]

Concept ACK.

[fanquake]

ACK fffbae5 - looks good. Could fixup the header comments if that's wanted. I'm optimistic that Boost Process will "go away" at some point. Removing it from the kernel is an improvement for now.

[theuni]

Force-pushed the requested copyright comment fixup, no other changes.

[fanquake]

re-ACK 98a8ad8

[hebasto]

ACK 98a8ad8

[Sjors]

ACK 98a8ad8. This certainly sounds like something that doesn't need to be in the kernel.

Given the name of the new file it would make sense to move runCommand over as well. As would moving the tests to their own file. But both can wait.

[theuni]

ACK 98a8ad8. This certainly sounds like something that doesn't need to be in the kernel.

Given the name of the new file it would make sense to move runCommand over as well. As would moving the tests to their own file. But both can wait.

I'd rather not, as we're really trying to separate out the nasty boost dependency here as opposed to the process launching functionality. If anything, I think it'd make more sense to just rename the function/file to be more specific as a follow-up.

[theuni]

Rearranged the headers as suggested by @hebasto. Would prefer to leave any other fixups to follow-up PRs.

[fanquake]

re-ACK 192325a - failing Windows CI can be ignored. Unrelated to this change.

[hebasto]

re-ACK 192325a

[ryanofsky]

Code review ACK 192325a, but I think you should consider moving the new file to libbitcoin_common.a instead of libbitcoin_util_a and src/common/ instead of src/util/.

Previously the difference between util and common was more nebulous, but now after talking to Carl and writing https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md, it seems like util can be the library for things included in the kernel which the kernel can depend on, and common can be the library for other code that needs to be shared internally, but should not be part of the kernel or shared externally.

[theuni]

I think you should consider moving the new file to libbitcoin_common.a instead of libbitcoin_util_a and src/common/ instead of src/util/.

I know I said above that I was going to leave this PR as-is, but this makes a lot of sense to me and it may as well be moved to a proper home from the start. So I've added a second commit to move this out of util and into common. Thanks @ryanofsky for the suggestion!

I do agree with @Sjors that maybe the file/function should be renamed and maybe some other functionality should be moved in, but I'd still prefer to discuss that in a follow-up PR as the primary goal here is getting a boost dep out of the kernel.

[theuni]

I believe c-i blew up here because libbitcoin_common.a currently does not get built with BOOST_CPPFLAGS but libbitcoin_util.a does. I'm going to push a test commit to reverse that because of the move here. Hopefully that works as opposed to having to keep it for both libs.

Edit: sigh, this is going to crash and burn because of interfaces/handler.cpp.

[theuni]

Mmm, that test-run really should have failed, but the compile failure is hidden because our boost header path is always implicitly included. Another instance of this: #26086 (comment)

I have now begrudgingly made $(BOOST_CPPFLAGS) available to both libs. We can get rid of it for utils when we decide on an approach for nuking boost::signals from there.

I suspect that @ryanofsky will be ok with this but @fanquake will consider it a regression. I'll let you two fight it out :). I'm fine with either:

• adding the boost flags where needed (done in 2nd commit)
• dropping the 2nd commit entirely for now

Edit: to be clear, util needs boost for interfaces/handler.cpp and common needs it for common/run_command.cpp.

[ryanofsky]

Edit: to be clear, util needs boost for interfaces/handler.cpp and common needs it for common/run_command.cpp.

That's interesting. I would say interfaces/*.cpp files belong more naturally in libbitcoin_common_a than libbitcoin_util_a anyway, since they aren't general purpose utilities, just bits of common code shared by node/wallet/gui executables. Also they shouldn't be part of the kernel. Maybe it would be easy to build these files as part of the common library instead of util library here, or it could be done as a followup to fully drop the boost dependency from util.

[ryanofsky]

Code review ACK 43b8777. Could consider squashing the two commits, so the code just moves once instead of twice.

[fanquake]

ACK 43b8777

but the compile failure is hidden because our boost header path is always implicitly included. Another instance of this: #26086 (comment)

We'll follow up here (suggested change) very shortly.

I suspect that ryanofsky will be ok with this but fanquake will consider it a regression. I'll let you two fight it out :)

Happy to just move this forward. Things are going in the right direction.

or it could be done as a followup to fully drop the boost dependency from util.

I am happy to follow up with this.

[maflcko]

pico-nit: Shouldn't this be in the very first line after the comment?

[fanquake]

I'll look at the follow up for moving more code, and could address as part of that.

[fanquake]

I am happy to follow up with this.

This is now #26293.

We'll follow up here (suggested change) very shortly.

After a second look, the remaining change in that branch is (as self-described) a hack, and I somehow forgot I'd already removed BOOST_CPPFLAGS from the BITCOIN_INCLUDES, so less to actually follow up on here.