Use subprocess library for notifications #32566

pull laanwj wants to merge 5 commits into bitcoin:master from laanwj:2025-05-subprocess-system changing 33 files +143 −113

laanwj commented at 2:55 pm on May 19, 2025: member
Builds on #29868 (to have windows support).

This switches the -*notify options to use the subprocess library, which is currently used for only the external signer. The main advantage of this is that file descriptor leaks are avoided (#32343). This could also add future flexibility in notifications, for example to bypass the shell (and launch commands with their arguments directly[1]), or do something with command output (like log it). But for now, this is meant to be one-to-one.
- Generalize ENABLE_EXTERNAL_SIGNER build option to ENABLE_SUBPROCESS.
- Unify everything dealing with external commands in common/run_command.cpp. Use the new functions RunShell and RunShellInThread.
- Remove HAVE_SYSTEM.
[1] There’s a long-running issue on Windows where it can’t pass the wallet name because there’s no way to escape it for the shell.
laanwj added the label Build system on May 19, 2025
laanwj added the label Utils/log/libs on May 19, 2025
DrahtBot commented at 2:55 pm on May 19, 2025: contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32566.

Reviews

See the guideline for information on the review process.

Type Reviewers

Concept ACK fanquake, hebasto, theStack

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:
- #32380 (Modernize use of UTF-8 in Windows code by hebasto)
- #29868 (Reintroduce external signer support for Windows by hebasto)
- #28792 (Embed default ASMap as binary dump header file by fjahr)
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.
laanwj force-pushed on May 19, 2025
DrahtBot added the label CI failed on May 19, 2025
DrahtBot commented at 3:00 pm on May 19, 2025: contributor
🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/42487791631 LLM reason (✨ experimental): The CI failure is due to a lint check indicating unused includes of the bitcoin-build-config.h header.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
- Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
- A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
- An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
laanwj force-pushed on May 19, 2025
laanwj force-pushed on May 19, 2025
laanwj force-pushed on May 19, 2025
fanquake commented at 3:47 pm on May 19, 2025: member

Concept ACK. Seems like there is some overlap here with #32380, but that this PR should go first, otherwise #32380 is going to be refactoring code, that is being deleted here.
laanwj commented at 4:06 pm on May 19, 2025: member

i’m not 100% sure ENABLE_SUBPROCESS needs to exist as an option at all, btw, it may be something to always enable, but maybe not on mobile/sandboxed platforms so wasn’t sure whether to make that decision here.

Edit: most notably, the ENABLE_SUBPROCESS=OFF path isn’t tested in the CI at all, it probably should? but in what run?
hebasto commented at 4:16 pm on May 19, 2025: member

Concept ACK.

Seems like there is some overlap here with #32380, but that this PR should go first, otherwise #32380 is going to be refactoring code, that is being deleted here.

I agree. And we are not going to switch to C++26 any time soon when #32380 will be required.
theStack commented at 5:02 pm on May 19, 2025: contributor

Concept ACK
laanwj force-pushed on May 19, 2025
laanwj force-pushed on May 19, 2025
laanwj force-pushed on May 19, 2025
laanwj force-pushed on May 19, 2025
laanwj force-pushed on May 19, 2025
laanwj force-pushed on May 19, 2025
laanwj force-pushed on May 19, 2025
laanwj force-pushed on May 19, 2025
laanwj force-pushed on May 20, 2025
DrahtBot removed the label CI failed on May 20, 2025
laanwj force-pushed on May 20, 2025
laanwj force-pushed on May 20, 2025

Type	Reviewers
Concept ACK	fanquake, hebasto, theStack

in src/util/subprocess.h:1134 in 2193963779 outdated

1129   for (auto arg : this->vargs_) {
1130+    if (!first_arg) {
1131+      command_line += L" ";
1132+    } else {
1133+      first_arg = false;
1134+    }

hebasto commented at 10:57 am on May 20, 2025:

Backported from upstream in #32567.

DrahtBot added the label CI failed on May 20, 2025
DrahtBot added the label Needs rebase on May 20, 2025
DrahtBot commented at 11:54 am on May 20, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

build: Rename ENABLE_EXTERNAL_SIGNER to ENABLE_SUBPROCESS

Generalize it, because we're going to use subprocess for other things.

47d62f42e5

test: Rename external_signer methods to subprocess 6c52c96831
subprocess: Don't add an extra whitespace at end of Windows command line
Upstreamed as: https://github.com/arun11299/cpp-subprocess/pull/119
0901351dc9

in src/common/system.cpp:52 in 3e2b4660b8 outdated

46-{
47-    if (strCommand.empty()) return;
48-#ifndef WIN32
49-    int nErr = ::system(strCommand.c_str());
50-#else
51-    int nErr = ::_wsystem(std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>,wchar_t>().from_bytes(strCommand).c_str());

hebasto commented at 2:00 pm on May 20, 2025:

3e2b4660b84af872d114afc1556649f3dd93a1af:

#include <codecvt> might be removed now.

laanwj commented at 2:51 pm on May 20, 2025:

Done, thanks

laanwj force-pushed on May 20, 2025
common: Change std::system usage to use subprocess 443d44e862
build: Remove HAVE_SYSTEM
It is no longer used.
ceb1783c1a
laanwj force-pushed on May 20, 2025

Contributors
laanwj DrahtBot fanquake hebasto theStack

Labels
Build system Utils/log/libs Needs rebase CI failed

Use subprocess library for notifications #32566

Code Coverage & Benchmarks

Reviews

Conflicts