RFC: Replacing Boost Process #24907

issue fanquake openend this issue on April 18, 2022
  1. fanquake commented at 2:34 pm on April 18, 2022: member

    Boost Process is currently relatively poorly maintained; bugs are remaining unfixed for multiple Boost releases, the code and issue tracking is confusingly spread across multiple repositories, see https://github.com/boostorg/process/ and https://github.com/klemens-morgenstern/boost-process), which themselves each have different versions of the code.

    There’s been no recent “modernization” of the module, i.e an option use std::filesystem over boost::filesystem, like other Boost modules, which is why we no-longer support external signing on Windows, see #24065.

    It’s inclusion in our project is also a downside to our Boost consolidation / removal, i.e #24742, as Process’s own dependencies (filesystem, system, win-api, + others) pull in even more Boost modules (i.e asio), which in turn pull in further modules..

    If we are going to continue to use/expand our usage of it, I think we need some sort of plan, addressing the above, rather than leaving it ignored while we start accumulating workarounds, i.e #24523, in our own code to support it, and it’s presence potentially blocks other project goals.

    One potential alternative is https://github.com/arun11299/cpp-subprocess.

  2. fanquake added the label Brainstorming on Apr 18, 2022
  3. prusnak commented at 4:19 pm on April 18, 2022: contributor

    One potential alternative is https://github.com/arun11299/cpp-subprocess.

    The suggested alternative looks quite nice:

    • small codebase (~2K LOC)
    • header-only template library
    • interface similar to Python subprocess module
    • no external dependencies
    • compatible licence (MIT)

    Pinging @Sjors as the only place where we use boost::process via RunCommandParseJSON is the ExternalSigner.

  4. laanwj commented at 4:58 pm on April 18, 2022: member

    One potential alternative is https://github.com/arun11299/cpp-subprocess.

    Concept ACK It looks like modern c++, only depends on the standard library, and the license matches (MIT). If it provides the needed functionality it seems like a good idea.

    Pinging @Sjors as the only place where we use boost::process via RunCommandParseJSON is the ExternalSigner.

    At the moment, yes. Though ideally we’d use whatever subprocess library we choose for the *notify commands as well, to be consistent.

  5. Sjors commented at 4:59 pm on April 18, 2022: member

    The main use case for RunCommandParseJSON is making the GUI work with hardware wallets. RPC users could in principle also call HWI manually. So one alternative to look at could be QT process. That said, the RPC code also serves to maintain good test coverage, and we wouldn’t want to pull a QT dependency into bitcoind. But maybe the QT relevant code can be extracted?

    Though ideally we’d use whatever subprocess library we choose for the *notify commands as well, to be consistent.

    There’s that too.

  6. prusnak commented at 5:48 pm on April 18, 2022: contributor

    So one alternative to look at could be QT process.

    We already use QProcess on macOS to workaround for a specific behavior - see #24668 (comment) and subsequent comments for context - and it seemed to me that the sentiment was to get rid of QProcess completely.

  7. Rspigler commented at 7:54 pm on April 18, 2022: contributor
    Concept ACK. Further removal of Boost is good, especially if bugs aren’t being fixed.
  8. theStack commented at 2:49 pm on May 3, 2022: contributor
    Strong Concept ACK
  9. hebasto commented at 11:01 am on July 28, 2022: member

    the code and issue tracking is confusingly spread across multiple repositories, see https://github.com/boostorg/process/ and https://github.com/klemens-morgenstern/boost-process), which themselves each have different versions of the code

    This is no longer the case.

    And Boost Process V2 is coming.

  10. fanquake commented at 2:04 pm on September 21, 2022: member

    And Boost Process V2 is coming.

    I don’t think that’s any improvement on the Boost Process front.

    Process V2 is now fully dependant on Boost ASIO:

    Since asio added pipes in boost 1.78, boost process V2 is fully asio based and uses it’s pipes and file-handles for the subprocess.

    This is another Boost (sub-)dependency we do not want. (I’m aware that process V1 already uses it in some capacity).

  11. theStack commented at 12:51 pm on March 13, 2023: contributor

    FWIW, I started working on replacing boost::process with cpp-subprocess on the following branch: https://github.com/theStack/bitcoin/tree/nuke_boost_process Currently the unit tests (system_tests/run_command) and functional tests (rpc_signer.py, wallet_signer.py) pass on both Linux and OpenBSD, but compiling for Windows via g++-mingw-w64-x86-64-posix fails due to missing headers (Windows.h). The changes are not very invasive, most work was probably to change from passing a single string to a list of arguments.

    If anyone is motivated to continue the work or maybe just want to evaluate if cpp-subprocess is a proper replacement candidate, feel free to take the commits. I’m currently not running any Windows system where I could even test the changes, the best I can do is to cross-compile (which as said above, doesn’t even work yet) and run the unit tests via Wine. cpp-subprocess’ README.md says:

    Support for Windows is limited at this time. Please report any specific use-cases that fail, and they will be fixed as they are reported.

    so there are likely more changes needed to make everything work as expected.

  12. willcl-ark commented at 12:17 pm on March 15, 2023: contributor

    FWIW, I started working on replacing boost::process with cpp-subprocess on the following branch:

    Does cpp-subprocess ok work on OpenBSD?

  13. theStack commented at 1:06 pm on March 15, 2023: contributor

    FWIW, I started working on replacing boost::process with cpp-subprocess on the following branch:

    Does cpp-subprocess ok work on OpenBSD?

    Yes, at least the corresponding unit tests and functional tests pass both on Linux (Ubuntu 22.04.1 LTS, Kernel 5.15.0) and OpenBSD (7.2). Would be nice if someone could test the branch (https://github.com/theStack/bitcoin/tree/nuke_boost_process) on MacOS.

  14. willcl-ark commented at 4:38 pm on March 15, 2023: contributor
    @theStack Just tested on MacOS 13.1 (without BDB and QT) and all functional and unit tests passed ✅
  15. fanquake commented at 10:05 am on November 28, 2023: member

    And Boost Process V2 is coming. I don’t think that’s any improvement on the Boost Process front. Process V2 is now fully dependant on Boost ASIO:

    Boost Process V2 (with Boost ASIO), has led to Boost Process silently hijacking the initialisation of our networking stack on Windows. See #28940.

  16. hebasto commented at 4:04 pm on November 30, 2023: member

    FWIW, I started working on replacing boost::process with cpp-subprocess on the following branch: theStack/bitcoin@nuke_boost_process

    I’ve picked up that branch. It appears that the upstream project requires a few patches to meet all our needs. The first one is https://github.com/arun11299/cpp-subprocess/pull/94.

  17. fanquake referenced this in commit f0e829022a on Dec 13, 2023
  18. fanquake closed this on Apr 10, 2024

  19. fanquake referenced this in commit 0a9cfd1752 on Apr 10, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me