Merge #15382: util: add RunCommandParseJSON

31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0 [util] add RunCommandParseJSON (Sjors Provoost)
c17f54ee535faaedf9033717403e1f775b5f1530 [ci] use boost::process (Sjors Provoost)
32128ba682033560d6eb2e4848a9f77a842016d2 [doc] include Doxygen comments for HAVE_BOOST_PROCESS (Sjors Provoost)
3c84d85f7d218fa27e9343c5cd1a55e519218980 [build] msvc: add boost::process (Sjors Provoost)
c47e4bbf0b44f2de1278f9538124ec98ee0815bb [build] make boost-process opt-in (Sjors Provoost)
929cda5470f98d1ef85c05b1cad4e2fb9227e3b0 configure: add ax_boost_process (Sjors Provoost)
8314c23d7b39fc36dde8b40b03b6efbe96f85698 [depends] boost: patch unused variable in boost_process (Sjors Provoost)

Pull request description:

  Prerequisite for external signer support in #16546. Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d).

  This adds a new dependency [boost process](https://github.com/boostorg/process/tree/boost-1.64.0). This is part of Boost since 1.64 which is part of `depends`. Because the minimum Boost version is 1.47, this functionality is skipped for older versions of Boost.

  Use `./configure --with-boost-process` to opt in, which checks for the presence of Boost::Process.

  We add `UniValue runCommandParseJSON(const std::string& strCommand)` to `system.{h,cpp}` which calls an arbitrary command and processes the JSON returned by it. This is currently only called by the test suite.

  ~For testing purposes this adds a new regtest-only RPC method `runcommand`, as well as `test/mocks/command.py` used by functional tests.~ (this is no longer the case)

  TODO:
  - [ ] review boost process in #15440

ACKs for top commit:
  achow101:
    ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0
  hebasto:
    re-ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0, only rebased (verified with `git range-diff`) and removed an unintentional tab character since the [previous](https://github.com/bitcoin/bitcoin/pull/15382#pullrequestreview-458371035) review.
  meshcollider:
    Very light utACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0, although I am not very confident with build stuff.
  promag:
    Code review ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0, don't mind the nit.
  ryanofsky:
    Code review ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0. I left some comments below that could be ignored or followed up later. The current change is clean and comprehensive.

Tree-SHA512: c506e747014b263606e1f538ed4624a8ad7bcf4e025cb700c12cc5739964e254dc04a2bbb848996b170e2ccec3fbfa4fe9e2b3976b191222cfb82fc3e6ab182d

.travis.yml

@@ -200,7 +200,7 @@ after_success:
         QEMU_USER_CMD=""
 
     - stage: test
-      name: 'Win64  [GOAL: deploy]  [unit tests, no gui, no functional tests]'
+      name: 'Win64  [GOAL: deploy]  [unit tests, no gui, no boost::process, no functional tests]'
       env: >-
         FILE_ENV="./ci/test/00_setup_env_win64.sh"
 

build-aux/m4/ax_boost_process.m4

@@ -0,0 +1,121 @@
+# ===========================================================================
+#     https://www.gnu.org/software/autoconf-archive/ax_boost_process.html
+# ===========================================================================
+#
+# SYNOPSIS
+#
+#   AX_BOOST_PROCESS
+#
+# DESCRIPTION
+#
+#   Test for Process library from the Boost C++ libraries. The macro
+#   requires a preceding call to AX_BOOST_BASE. Further documentation is
+#   available at <http://randspringer.de/boost/index.html>.
+#
+#   This macro calls:
+#
+#     AC_SUBST(BOOST_PROCESS_LIB)
+#
+#   And sets:
+#
+#     HAVE_BOOST_PROCESS
+#
+# LICENSE
+#
+#   Copyright (c) 2008 Thomas Porschberg <thomas@randspringer.de>
+#   Copyright (c) 2008 Michael Tindal
+#   Copyright (c) 2008 Daniel Casimiro <dan.casimiro@gmail.com>
+#
+#   Copying and distribution of this file, with or without modification, are
+#   permitted in any medium without royalty provided the copyright notice
+#   and this notice are preserved. This file is offered as-is, without any
+#   warranty.
+
+#serial 2
+
+AC_DEFUN([AX_BOOST_PROCESS],
+[
+	AC_ARG_WITH([boost-process],
+	AS_HELP_STRING([--with-boost-process@<:@=special-lib@:>@],
+                   [use the Process library from boost - it is possible to specify a certain library for the linker
+                        e.g. --with-boost-process=boost_process-gcc-mt ]),
+        [
+        if test "$withval" = "no"; then
+			want_boost_process="no"
+        elif test "$withval" = "yes"; then
+            want_boost_process="yes"
+            ax_boost_user_process_lib=""
+        else
+		    want_boost_process="yes"
+		ax_boost_user_process_lib="$withval"
+		fi
+        ],
+        [want_boost_process="yes"]
+	)
+
+	if test "x$want_boost_process" = "xyes"; then
+        AC_REQUIRE([AC_PROG_CC])
+        AC_REQUIRE([AC_CANONICAL_BUILD])
+		CPPFLAGS_SAVED="$CPPFLAGS"
+		CPPFLAGS="$CPPFLAGS $BOOST_CPPFLAGS"
+		export CPPFLAGS
+
+		LDFLAGS_SAVED="$LDFLAGS"
+		LDFLAGS="$LDFLAGS $BOOST_LDFLAGS"
+		export LDFLAGS
+
+        AC_CACHE_CHECK(whether the Boost::Process library is available,
+					   ax_cv_boost_process,
+        [AC_LANG_PUSH([C++])
+			 CXXFLAGS_SAVE=$CXXFLAGS
+			 CXXFLAGS=
+
+             AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[@%:@include <boost/process.hpp>]],
+                [[boost::process::child* child = new boost::process::child; delete child;]])],
+                ax_cv_boost_process=yes, ax_cv_boost_process=no)
+			 CXXFLAGS=$CXXFLAGS_SAVE
+             AC_LANG_POP([C++])
+		])
+		if test "x$ax_cv_boost_process" = "xyes"; then
+			AC_SUBST(BOOST_CPPFLAGS)
+
+			AC_DEFINE(HAVE_BOOST_PROCESS,,[define if the Boost::Process library is available])
+            BOOSTLIBDIR=`echo $BOOST_LDFLAGS | sed -e 's/@<:@^\/@:>@*//'`
+
+			LDFLAGS_SAVE=$LDFLAGS
+            if test "x$ax_boost_user_process_lib" = "x"; then
+                for libextension in `ls -r $BOOSTLIBDIR/libboost_process* 2>/dev/null | sed 's,.*/lib,,' | sed 's,\..*,,'` ; do
+                     ax_lib=${libextension}
+				    AC_CHECK_LIB($ax_lib, exit,
+                                 [BOOST_PROCESS_LIB="-l$ax_lib"; AC_SUBST(BOOST_PROCESS_LIB) link_process="yes"; break],
+                                 [link_process="no"])
+				done
+                if test "x$link_process" != "xyes"; then
+                for libextension in `ls -r $BOOSTLIBDIR/boost_process* 2>/dev/null | sed 's,.*/,,' | sed -e 's,\..*,,'` ; do
+                     ax_lib=${libextension}
+				    AC_CHECK_LIB($ax_lib, exit,
+                                 [BOOST_PROCESS_LIB="-l$ax_lib"; AC_SUBST(BOOST_PROCESS_LIB) link_process="yes"; break],
+                                 [link_process="no"])
+				done
+                fi
+
+            else
+               for ax_lib in $ax_boost_user_process_lib boost_process-$ax_boost_user_process_lib; do
+				      AC_CHECK_LIB($ax_lib, exit,
+                                   [BOOST_PROCESS_LIB="-l$ax_lib"; AC_SUBST(BOOST_PROCESS_LIB) link_process="yes"; break],
+                                   [link_process="no"])
+                  done
+
+            fi
+            if test "x$ax_lib" = "x"; then
+                AC_MSG_ERROR(Could not find a version of the Boost::Process library!)
+            fi
+			if test "x$link_process" = "xno"; then
+				AC_MSG_ERROR(Could not link against $ax_lib !)
+			fi
+		fi
+
+		CPPFLAGS="$CPPFLAGS_SAVED"
+	LDFLAGS="$LDFLAGS_SAVED"
+	fi
+])

ci/test/00_setup_env_arm.sh

@@ -25,4 +25,4 @@ export RUN_INTEGRATION_TESTS=false
 export GOAL="install"
 # -Wno-psabi is to disable ABI warnings: "note: parameter passing for argument of type ... changed in GCC 7.1"
 # This could be removed once the ABI change warning does not show up by default
-export BITCOIN_CONFIG="--enable-reduce-exports --enable-suppress-external-warnings --enable-werror CXXFLAGS=-Wno-psabi"
+export BITCOIN_CONFIG="--enable-reduce-exports --enable-suppress-external-warnings --enable-werror CXXFLAGS=-Wno-psabi --with-boost-process"

ci/test/00_setup_env_mac.sh

@@ -14,4 +14,4 @@ export XCODE_BUILD_ID=12B45b
 export RUN_UNIT_TESTS=false
 export RUN_INTEGRATION_TESTS=false
 export GOAL="all deploy"
-export BITCOIN_CONFIG="--with-gui --enable-reduce-exports --disable-miner --enable-werror"
+export BITCOIN_CONFIG="--with-gui --enable-reduce-exports --disable-miner --enable-werror --with-boost-process"

ci/test/00_setup_env_mac_host.sh

@@ -10,7 +10,7 @@ export CONTAINER_NAME=ci_macos
 export HOST=x86_64-apple-darwin
 export PIP_PACKAGES="zmq lief"
 export GOAL="install"
-export BITCOIN_CONFIG="--with-gui --enable-reduce-exports --disable-miner --enable-werror"
+export BITCOIN_CONFIG="--with-gui --enable-reduce-exports --disable-miner --enable-werror --with-boost-process"
 export NO_DEPENDS=1
 export OSX_SDK=""
 export CCACHE_SIZE=300M

ci/test/00_setup_env_native_asan.sh

@@ -12,4 +12,4 @@ export TEST_RUNNER_EXTRA="--timeout-factor=4"  # Increase timeout because saniti
 export FUNCTIONAL_TESTS_CONFIG="--exclude wallet_multiwallet.py" # Temporarily suppress ASan heap-use-after-free (see issue #14163)
 export RUN_BENCH=true
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER --with-sanitizers=address,integer,undefined CC=clang CXX=clang++"
+export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER --with-sanitizers=address,integer,undefined CC=clang CXX=clang++ --with-boost-process"

ci/test/00_setup_env_native_fuzz.sh

@@ -16,4 +16,4 @@ export RUN_UNIT_TESTS=false
 export RUN_INTEGRATION_TESTS=false
 export RUN_FUZZ_TESTS=true
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-zmq --disable-ccache --enable-fuzz --with-sanitizers=fuzzer,address,undefined --enable-suppress-external-warnings CC=clang-15 CXX=clang++-15"
+export BITCOIN_CONFIG="--enable-zmq --disable-ccache --enable-fuzz --with-sanitizers=fuzzer,address,undefined --enable-suppress-external-warnings CC=clang-15 CXX=clang++-15 --with-boost-process"

ci/test/00_setup_env_native_multiprocess.sh

@@ -10,5 +10,5 @@ export CONTAINER_NAME=ci_native_multiprocess
 export PACKAGES="cmake python3"
 export DEP_OPTS="MULTIPROCESS=1"
 export GOAL="install"
-export BITCOIN_CONFIG=""
+export BITCOIN_CONFIG="--with-boost-process"
 export TEST_RUNNER_ENV="BITCOIND=dash-node"

ci/test/00_setup_env_native_nowallet.sh

@@ -10,4 +10,4 @@ export CONTAINER_NAME=ci_native_nowallet
 export PACKAGES="python3-zmq"
 export DEP_OPTS="NO_WALLET=1"
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-reduce-exports"
+export BITCOIN_CONFIG="--enable-reduce-exports --with-boost-process"

ci/test/00_setup_env_native_qt5.sh

@@ -15,4 +15,4 @@ export RUN_UNIT_TESTS_SEQUENTIAL="true"
 export RUN_UNIT_TESTS="false"
 export GOAL="install"
 export PREVIOUS_RELEASES_TO_DOWNLOAD="v0.15.0.0 v0.16.1.1 v0.17.0.3 v18.2.2 v19.3.0 v20.0.1"
-export BITCOIN_CONFIG="--enable-zmq --enable-reduce-exports --disable-fuzz-binary LDFLAGS=-static-libstdc++"
+export BITCOIN_CONFIG="--enable-zmq --enable-reduce-exports --disable-fuzz-binary LDFLAGS=-static-libstdc++ --with-boost-process"

ci/test/00_setup_env_native_tsan.sh

@@ -12,7 +12,7 @@ export DEP_OPTS="CC=clang-15 CXX='clang++-15 -stdlib=libc++"
 export TEST_RUNNER_EXTRA="--extended --exclude feature_pruning,feature_dbcrash,wallet_multiwallet.py" # Temporarily suppress ASan heap-use-after-free (see issue #14163)
 export TEST_RUNNER_EXTRA="${TEST_RUNNER_EXTRA} --timeout-factor=4"  # Increase timeout because sanitizers slow down
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-zmq --with-gui=no --with-sanitizers=thread CC=clang-15 CXX=clang++-15"
+export BITCOIN_CONFIG="--enable-zmq --with-gui=no --with-sanitizers=thread CC=clang-15 CXX=clang++-15 --with-boost-process"
 export CPPFLAGS="-DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG"
 export PYZMQ=true
 export RUN_SYMBOL_TESTS=false

ci/test/00_setup_env_s390x.sh

@@ -21,4 +21,4 @@ export CONTAINER_NAME=ci_s390x
 export RUN_UNIT_TESTS=true
 export RUN_FUNCTIONAL_TESTS=true
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests"  # GUI tests disabled for now, see https://github.com/bitcoin/bitcoin/issues/23730
+export BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests --with-boost-process"  # GUI tests disabled for now, see https://github.com/bitcoin/bitcoin/issues/23730

ci/test/00_setup_env_win64.sh

@@ -13,5 +13,5 @@ export DPKG_ADD_ARCH="i386"
 export RUN_INTEGRATION_TESTS=false
 export RUN_SECURITY_TESTS="false"
 export GOAL="deploy"
-export BITCOIN_CONFIG="--enable-gui --enable-reduce-exports --disable-miner"
+export BITCOIN_CONFIG="--enable-gui --enable-reduce-exports --disable-miner --without-boost-process"
 export DIRECT_WINE_EXEC_TESTS=true

configure.ac

@@ -1457,6 +1457,9 @@ if test x$want_boost = xno; then
 fi
 AX_BOOST_FILESYSTEM
 
+dnl Opt-in to boost-process
+AS_IF([ test x$with_boost_process != x ], [ AX_BOOST_PROCESS ], [ ax_cv_boost_process=no ] )
+
 if test x$suppress_external_warnings != xno; then
     dnl Prevent use of std::unary_function, which was removed in C++17,
     dnl and will generate warnings with newer compilers.
@@ -1932,6 +1935,7 @@ esac
 
 echo
 echo "Options used to compile and link:"
+echo "  boost process = $ax_cv_boost_process"
 echo "  multiprocess  = $build_multiprocess"
 echo "  with wallet         = $enable_wallet"
 echo "  with gui / qt       = $bitcoin_enable_qt"

doc/Doxyfile.in

@@ -2093,7 +2093,7 @@ INCLUDE_FILE_PATTERNS  =
 # recursively expanded use the := operator instead of the = operator.
 # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.
 
-PREDEFINED             =
+PREDEFINED             = HAVE_BOOST_PROCESS
 
 # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this
 # tag can be used to specify a list of macro names that should be expanded. The

src/Makefile.test.include

@@ -160,6 +160,7 @@ BITCOIN_TESTS =\
   test/streams_tests.cpp \
   test/subsidy_tests.cpp \
   test/sync_tests.cpp \
+  test/system_tests.cpp \
   test/util_threadnames_tests.cpp \
   test/timedata_tests.cpp \
   test/torcontrol_tests.cpp \

src/test/system_tests.cpp

@@ -0,0 +1,95 @@
+// Copyright (c) 2019 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+//
+#include <test/util/setup_common.h>
+#include <util/system.h>
+#include <univalue.h>
+
+#ifdef HAVE_BOOST_PROCESS
+#include <boost/process.hpp>
+#endif // HAVE_BOOST_PROCESS
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(system_tests, BasicTestingSetup)
+
+// At least one test is required (in case HAVE_BOOST_PROCESS is not defined).
+// Workaround for https://github.com/bitcoin/bitcoin/issues/19128
+BOOST_AUTO_TEST_CASE(dummy)
+{
+    BOOST_CHECK(true);
+}
+
+#ifdef HAVE_BOOST_PROCESS
+
+bool checkMessage(const std::runtime_error& ex)
+{
+    // On Linux & Mac: "No such file or directory"
+    // On Windows: "The system cannot find the file specified."
+    const std::string what(ex.what());
+    BOOST_CHECK(what.find("file") != std::string::npos);
+    return true;
+}
+
+bool checkMessageFalse(const std::runtime_error& ex)
+{
+    BOOST_CHECK_EQUAL(ex.what(), std::string("RunCommandParseJSON error: process(false) returned 1: \n"));
+    return true;
+}
+
+bool checkMessageStdErr(const std::runtime_error& ex)
+{
+    const std::string what(ex.what());
+    BOOST_CHECK(what.find("RunCommandParseJSON error:") != std::string::npos);
+    return checkMessage(ex);
+}
+
+BOOST_AUTO_TEST_CASE(run_command)
+{
+    {
+        const UniValue result = RunCommandParseJSON("");
+        BOOST_CHECK(result.isNull());
+    }
+    {
+#ifdef WIN32
+        // Windows requires single quotes to prevent escaping double quotes from the JSON...
+        const UniValue result = RunCommandParseJSON("echo '{\"success\": true}'");
+#else
+        // ... but Linux and macOS echo a single quote if it's used
+        const UniValue result = RunCommandParseJSON("echo \"{\"success\": true}\"");
+#endif
+        BOOST_CHECK(result.isObject());
+        const UniValue& success = find_value(result, "success");
+        BOOST_CHECK(!success.isNull());
+        BOOST_CHECK_EQUAL(success.getBool(), true);
+    }
+    {
+        // An invalid command is handled by Boost
+        BOOST_CHECK_EXCEPTION(RunCommandParseJSON("invalid_command"), boost::process::process_error, checkMessage); // Command failed
+    }
+    {
+        // Return non-zero exit code, no output to stderr
+        BOOST_CHECK_EXCEPTION(RunCommandParseJSON("false"), std::runtime_error, checkMessageFalse);
+    }
+    {
+        // Return non-zero exit code, with error message for stderr
+        BOOST_CHECK_EXCEPTION(RunCommandParseJSON("ls nosuchfile"), std::runtime_error, checkMessageStdErr);
+    }
+    {
+        BOOST_REQUIRE_THROW(RunCommandParseJSON("echo \"{\""), std::runtime_error); // Unable to parse JSON
+    }
+    // Test std::in, except for Windows
+#ifndef WIN32
+    {
+        const UniValue result = RunCommandParseJSON("cat", "{\"success\": true}");
+        BOOST_CHECK(result.isObject());
+        const UniValue& success = find_value(result, "success");
+        BOOST_CHECK(!success.isNull());
+        BOOST_CHECK_EQUAL(success.getBool(), true);
+    }
+#endif
+}
+#endif // HAVE_BOOST_PROCESS
+
+BOOST_AUTO_TEST_SUITE_END()

src/util/system.cpp

@@ -7,6 +7,11 @@
 #include <util/system.h>
 
 #include <support/allocators/secure.h>
+
+#ifdef HAVE_BOOST_PROCESS
+#include <boost/process.hpp>
+#endif // HAVE_BOOST_PROCESS
+
 #include <chainparamsbase.h>
 #include <ctpl_stl.h>
 #include <stacktraces.h>
@@ -1295,6 +1300,43 @@ void RenameThreadPool(ctpl::thread_pool& tp, const char* baseName)
     }
 }
 
+#ifdef HAVE_BOOST_PROCESS
+UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in)
+{
+    namespace bp = boost::process;
+
+    UniValue result_json;
+    bp::opstream stdin_stream;
+    bp::ipstream stdout_stream;
+    bp::ipstream stderr_stream;
+
+    if (str_command.empty()) return UniValue::VNULL;
+
+    bp::child c(
+        str_command,
+        bp::std_out > stdout_stream,
+        bp::std_err > stderr_stream,
+        bp::std_in < stdin_stream
+    );
+    if (!str_std_in.empty()) {
+        stdin_stream << str_std_in << std::endl;
+    }
+    stdin_stream.pipe().close();
+
+    std::string result;
+    std::string error;
+    std::getline(stdout_stream, result);
+    std::getline(stderr_stream, error);
+
+    c.wait();
+    const int n_error = c.exit_code();
+    if (n_error) throw std::runtime_error(strprintf("RunCommandParseJSON error: process(%s) returned %d: %s\n", str_command, n_error, error));
+    if (!result_json.read(result)) throw std::runtime_error("Unable to parse JSON: " + result);
+
+    return result_json;
+}
+#endif // HAVE_BOOST_PROCESS
+
 void SetupEnvironment()
 {
 #ifdef HAVE_MALLOPT_ARENA_MAX

src/util/system.h

@@ -52,6 +52,8 @@ extern bool fDisableGovernance;
 extern int nWalletBackups;
 extern const std::string gCoinJoinName;
 
+class UniValue;
+
 // Application startup time (used for uptime calculation)
 int64_t GetStartupTime();
 
@@ -124,6 +126,16 @@ std::string ShellEscape(const std::string& arg);
 #if HAVE_SYSTEM
 void runCommand(const std::string& strCommand);
 #endif
+#ifdef HAVE_BOOST_PROCESS
+/**
+ * Execute a command which returns JSON, and parse the result.
+ *
+ * @param str_command The command to execute, including any arguments
+ * @param str_std_in string to pass to stdin
+ * @return parsed JSON
+ */
+UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in="");
+#endif // HAVE_BOOST_PROCESS
 
 /**
  * Most paths passed as configuration arguments are treated as relative to

test/lint/lint-includes.sh

@@ -60,6 +60,7 @@ EXPECTED_BOOST_INCLUDES=(
     boost/multi_index_container.hpp
     boost/optional.hpp
     boost/pool/pool_alloc.hpp
+    boost/process.hpp
     boost/signals2/connection.hpp
     boost/signals2/optional_last_value.hpp
     boost/signals2/signal.hpp