Replace Boost.Process with cpp-subprocess

[hebasto]

Closes #24907.

This PR is based on theStack's work.

The subprocess.hpp header has been sourced from the upstream repo with the only modification being the removal of convenience functions, which are not utilized in our codebase.

Windows-related changes will be addressed in subsequent follow-ups.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, theStack, Sjors, fanquake
Concept ACK	TheCharlatan, dergoegge

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:

• #29744 (lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner by davidgumberg)
• #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.

[TheCharlatan]

Concept ACK

[maflcko]

I guess the compile error is due to subprocess requiring C++17 and not allowing C++20 (or later)?

[maflcko]

nit: Could split this into one-per-line to avoid overly long lines, diffs and reduce merge conflicts?

[fanquake]

Probably easier to just fix the typos, then add more things for linters to exclude? No new ones should be introduced.

[fanquake]

Concept ACK on removing Boost Process. Is the plan to also delete any unused code from this header? I don't think copy-pasting dead/untested code into this repository is a good idea, and only serves to complicate review, and potentially cause issues in future (it maybe already doesn't work with c++20?)

[theStack]

Concept ACK, thanks for picking this up.

[hebasto]

Is the plan to also delete any unused code from this header?

I haven't consider it yet.

it maybe already doesn't work with c++20?

CI jobs that uses C++20 are passing so far.

[sipa]

Is the plan to also delete any unused code from this header?

If at all possible, I'd prefer for it to be subtreed, so that updating to newer upstream versions is easy.

[fanquake]

I was actually thinking the opposite. I think a subtree is overkill here, and, upstream isn't exactly active (some random merges but no development), so I'm not sure if there will be many new versions.

I'd rather take the small(er) portion of any code we need, and have something better integrated/reviewed. Hopefully that will help avoid randomly pulling code from upstream, that isn't well developed or reviewed.

[fanquake]

Note that the last version from upstream was nearly 4 years ago: https://github.com/arun11299/cpp-subprocess/releases/tag/v2.0.

[hebasto]

I'd rather take the small(er) portion of any code we need, and have something better integrated/reviewed.

Maybe a subtree from our own fork? (to avoid bringing testing stuff to the main repo)

[fanquake]

Maybe a subtree from our own fork?

I think that would mostly just re-introduce the awkwardness we ended up with with univalue, for not much benefit, and for code that shouldn't be critical (i.e compared to leveldb & libsecp256k1), this is for an optional feature, that is not used by a large proportion of the people running this software.

[TheCharlatan]

Re #28981 (comment)

I'd rather take the small(er) portion of any code we need, and have something better integrated/reviewed.

Looking at the coverage report of the header, there seems to be some potential for pruning out (89.2% function coverage and 67.4% line coverage),

[hebasto]

I switched to the Popen constructor that accepts const std::string& instead of std::vector<std::string> to address quoting issues on Windows.

@theStack
Since you initially chose to use a vector, do you have any objections to this approach?

---

Here are the Windows functional tests -- https://github.com/hebasto/bitcoin/actions/runs/7072075125/job/19250502576

[hebasto]

CI turned green. Undrafted.

cc @achow101 @Sjors

[theStack]

I switched to the Popen constructor that accepts const std::string& instead of std::vector<std::string> to address quoting issues on Windows.

@theStack Since you initially chose to use a vector, do you have any objections to this approach?

No objections at all. I would have expected that passing the arguments as a single string is more prone to quoting issues than if it's done in a list, where quoting shouldn't be needed at all, but if it works, all the better :) (Maybe that unverified assumption made me use the std::vector Popen ctor back then, but I honestly don't really remember).

[hebasto]

I switched to the Popen constructor that accepts const std::string& instead of std::vector<std::string> to address quoting issues on Windows.
@theStack Since you initially chose to use a vector, do you have any objections to this approach?

No objections at all. I would have expected that passing the arguments as a single string is more prone to quoting issues than if it's done in a list, where quoting shouldn't be needed at all, but if it works, all the better :) (Maybe that unverified assumption made me use the std::vector Popen ctor back then, but I honestly don't really remember).

To support the string-based approach, let's consider echo-ing a JSON-parsable string on Windows:

>echo {"success": true}
{"success": true}

and reproducing it using Python's subprocess module, which interface is mimicked in cpp-subprocess:

>>> import subprocess
>>> subprocess.run("cmd.exe /c echo {\"success\": true}")
{"success": true}
CompletedProcess(args='cmd.exe /c echo {"success": true}', returncode=0)

It becomes intricate when using a list of strings:

>>> subprocess.run(["cmd.exe", "/c", "echo", "{\"success\": true}"])
"{\"success\": true}"

or

>>> subprocess.run(["cmd.exe", "/c", "echo", "{\"success\":", "true}"])
{\"success\": true}

or

>>> subprocess.run(['cmd.exe', '/c', 'echo', '{"success":', 'true}'])
{\"success\": true}

[hebasto]

Rebased due to the conflict with the merged #29479.

@Sjors @theStack @achow101 sorry for invalidating your acks :)

[achow101]

reACK d5a7155

Checked diff is just for resolving rebase conflicts.

[theStack]

Light re-ACK d5a7155

(as per git range-diff 86e1f24...d5a7155)

[Sjors]

re-tACK d5a7155

Tested display address from the GUI on a Trezor on Ubuntu.

[fanquake]

ACK d5a7155 - with the expectation that this code is going to be maintained as our own. Next PRs should:

• Remove linter exclusions and fix all issues.
• Delete all unused code.
• Rewrite in C++20.
• hpp -> cpp ?

[hebasto]

... with the expectation that this code is going to be maintained as our own. Next PRs should:

• Remove linter exclusions and fix all issues.

See:

• Fix typos in subprocess.hpp #29849
• refactor: Rename subprocess.hpp to follow our header name conventions #29910

• Delete all unused code.

The work has been started in #29865.