kernel: move RunCommandParseJSON to its own file

theuni commented at 6:24 pm on September 28, 2022: member

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.

in src/util/run_command.h:1 in fffbae5be3 outdated

0@@ -0,0 +1,22 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto

maflcko commented at 6:32 pm on September 28, 2022:

boost process was written in 2014 :eyes:

theuni commented at 8:01 pm on September 28, 2022:

🧙🔮

theuni commented at 3:15 pm on October 3, 2022:

As you pointed out, this is obviously all new code from 2020+, so I’ve dropped this credit.

maflcko commented at 6:33 pm on September 28, 2022: member

Concept ACK

maflcko added the label Refactoring on Sep 28, 2022

theuni commented at 8:03 pm on September 28, 2022: member

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 commented at 8:58 pm on September 28, 2022: member

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 commented at 11:22 pm on September 28, 2022: contributor

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 commented at 11:31 am on September 29, 2022: member

Concept ACK.

fanquake approved

fanquake commented at 12:15 pm on October 3, 2022: member

ACK fffbae5be3ee3bb34c5a2b4dbf3acc082ddd4444 - 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 on Oct 3, 2022

theuni commented at 3:16 pm on October 3, 2022: member

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

fanquake approved

fanquake commented at 3:26 pm on October 3, 2022: member

re-ACK 98a8ad8b2b8b578c75a06ddee2f9895bae5a8955

in src/util/run_command.h:8 in 98a8ad8b2b outdated

0@@ -0,0 +1,21 @@
1+// Copyright (c) 2022 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_UTIL_RUN_COMMAND_H
6+#define BITCOIN_UTIL_RUN_COMMAND_H
7+
8+#include <string>

hebasto commented at 8:20 am on October 4, 2022:

nit: Could be a forward declaration.

in src/util/run_command.cpp:11 in 98a8ad8b2b outdated

 6+#include <config/bitcoin-config.h>
 7+#endif
 8+
 9+#include <tinyformat.h>
10+#include <univalue.h>
11+#include <util/run_command.h>

hebasto commented at 8:22 am on October 4, 2022:

nit: Usually, a related header is placed at the top of the source file.

in src/util/run_command.cpp:25 in 98a8ad8b2b outdated

19+#endif
20+#include <boost/process.hpp>
21+#if defined(__GNUC__)
22+#pragma GCC diagnostic pop
23+#endif
24+#endif // ENABLE_EXTERNAL_SIGNER

hebasto commented at 8:26 am on October 4, 2022:

Perhaps, it’s worth to move this code snippet into the header, and DRY src/test/system_tests.cpp?

theuni commented at 1:46 pm on October 4, 2022:

Would rather not, as kinda the point of the PR is to cordon off boost includes/hacks by forcing cpp files admit they need them :)

hebasto approved

hebasto commented at 8:27 am on October 4, 2022: member

ACK 98a8ad8b2b8b578c75a06ddee2f9895bae5a8955

Sjors commented at 9:09 am on October 4, 2022: member

ACK 98a8ad8b2b8b578c75a06ddee2f9895bae5a8955. 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 commented at 1:49 pm on October 4, 2022: member

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.

kernel: move RunCommandParseJSON to its own file

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

192325a77d

theuni force-pushed on Oct 4, 2022

theuni commented at 2:02 pm on October 4, 2022: member

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

fanquake approved

fanquake commented at 2:21 pm on October 4, 2022: member

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

hebasto approved

hebasto commented at 2:30 pm on October 4, 2022: member

re-ACK 192325a77d593e404e74ef5e204aed8801b4e66f

ryanofsky approved

ryanofsky commented at 2:51 pm on October 4, 2022: contributor

Code review ACK 192325a77d593e404e74ef5e204aed8801b4e66f, 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 force-pushed on Oct 4, 2022

theuni commented at 6:22 pm on October 4, 2022: member

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 commented at 7:55 pm on October 4, 2022: member

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.

refactor: move run_command from util to common

Quoting ryanofsky: "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."

43b8777dc3

theuni force-pushed on Oct 4, 2022

theuni commented at 9:22 pm on October 4, 2022: member

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 commented at 2:40 pm on October 5, 2022: contributor

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 approved

ryanofsky commented at 2:43 pm on October 5, 2022: contributor

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

fanquake approved

fanquake commented at 9:56 am on October 10, 2022: member

ACK 43b8777dc3e63f4a1b20a3cb23e44c1b9e32862b

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.

fanquake merged this on Oct 10, 2022

fanquake closed this on Oct 10, 2022

in src/common/run_command.cpp:9 in 43b8777dc3

0@@ -0,0 +1,64 @@
1+// Copyright (c) 2022 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#if defined(HAVE_CONFIG_H)
6+#include <config/bitcoin-config.h>
7+#endif
8+
9+#include <common/run_command.h>

maflcko commented at 10:06 am on October 10, 2022:

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

fanquake commented at 1:18 pm on October 11, 2022:

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

sidhujag referenced this in commit f1f968f057 on Oct 10, 2022

fanquake commented at 1:17 pm on October 11, 2022: member

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.

bitcoin locked this on Oct 11, 2023

kernel: move RunCommandParseJSON to its own file #26196

Conflicts