Use subprocess library for notifications #32566
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32566. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
397ff54 to
7b25522
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
8aebdab to
e32bfc4
Compare
|
i'm not 100% sure Edit: most notably, the |
|
Concept ACK |
9069ed3 to
722cd1d
Compare
ec5c0c0 to
27d4ae1
Compare
27d4ae1 to
02d7fb6
Compare
02d7fb6 to
56fb60a
Compare
9de7029 to
2193963
Compare
14362bc to
ceb1783
Compare
|
|
||
| #ifdef ENABLE_EXTERNAL_SIGNER | ||
| #ifdef ENABLE_SUBPROCESS |
There was a problem hiding this comment.
I think you can just drop these #ifdefs around our own headers. Same for in RunCommandParseJSON below.
There was a problem hiding this comment.
Wasn't sure about this. One reason for setting ENABLE_SUBPROCESS=OFF in the first place would be that the operating system doesn't support subprocesses. As subprocess is a header-only library, including the subprocess.hpp header would mean use of fork execv CreateProcessW etc. So it could break the compile.
There was a problem hiding this comment.
If we don't care about supporting such operating systems (only iOS comes to mind), it would imo be preferable to remove the option entirely (and with it, all the ifdefs).
There was a problem hiding this comment.
I think I agree. Given we don't actually test or ship anything for iOS, it seems like we can clean up the code now, and figure out what to do if/when this might be an issue later.
| auto c = sp::Popen("cmd.exe /c " + str_command); | ||
| #else | ||
| // Emulate system(3) behavior, but don't leak file descriptors. | ||
| auto c = sp::Popen({"/bin/sh", "-c", str_command}, sp::close_fds{true}); |
There was a problem hiding this comment.
TIL that upstream subprocess has a shell option that does the equivalent (https://github.com/arun11299/cpp-subprocess/blob/master/cpp-subprocess/subprocess.hpp#L1630) . But we removed it.
Unfortunately, currently it doesn't support Windows. Could revert the option and switch to using that, when it does. Or, unless they had reason to not do it in the trivial way we do, it seems easy to add.
There was a problem hiding this comment.
A related discussion takes place at #32577.
There was a problem hiding this comment.
This is the first use of the Popen(std::initializer_list<const char*> cmd_args, Args&& ...args) ctor. Before this, it was a candidate for cleanup.
e63a703 subprocess: Don't add an extra whitespace at end of Windows command line (laanwj) Pull request description: A list of the backported PRs: - arun11299/cpp-subprocess#119 The following PRs were skipped for backporting: - arun11299/cpp-subprocess#118 because there is no changes in the header code. Required for #32566. ACKs for top commit: laanwj: Code review ACK e63a703 Tree-SHA512: 69a74aa7f9c611a9ec910e27161c5e9e147067d37f8335953cd3875fcc88dc840a2f7b206bb603f22507159e406b1449f1dc4702fffe890bb824672641b4feed
Generalize it, because we're going to use subprocess for other things.
It is no longer used.
ceb1783 to
82aeba6
Compare
|
Rebased for #32567: ceb1783c1af07f59885f51fdd15e7143fefdd821 → 82aeba60c2b3a166302e10fbf6e3eaf0593fab4d, which dropped one commit. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Builds on #29868 (to have windows support).
This switches the
-*notifyoptions 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.ENABLE_EXTERNAL_SIGNERbuild option toENABLE_SUBPROCESS.common/run_command.cpp. Use the new functionsRunShellandRunShellInThread.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.