Any reason for that? (Note that my review so far is only based on comparing git repos, haven’t looked at the concrete backport code yet.) For easier review, it might make sense to add a list of the backported upstream PRs also to the PR description.
hebasto
commented at 0:29 am on April 30, 2025:
member
From what I found, the following merged upstream PRs since our initial cpp-subprocess import (see PR #28981, commit cc8b987, based on upstream arun11299/cpp-subprocess@4025693) are not included yet in this PR branch:
i have not compared this to upstream but the change in themselves LGTM.
DrahtBot requested review from theStack
on Apr 30, 2025
fanquake
commented at 1:42 pm on April 30, 2025:
member
During my quick (and possibly not thorough) tests, I was unable to reproduce the issues mentioned in those PRs on our codebase.
Seems odd to not pull in all changes, given no real rationale (i.e the code being incorrect/broken) for excluding them.
This also means that next time this header is updated, they may or may not get pulled in anyways, depending on the author of the change, given there’s nothing documented to say they should be excluded.
subprocess: Fix memory leaks
I encountered this issue while running my code with Valgrind today.
Below is part of the Valgrind error message:
```
==1578139== 472 bytes in 1 blocks are still reachable in loss record 1 of 1
==1578139== at 0x4848899: malloc (...)
==1578139== by 0x4B3AF62: fdopen@@GLIBC_2.2.5 (...)
==1578139== by 0x118B09: subprocess::Popen::execute_process() (...)
```
I noticed that a similar fix had been proposed by another contributor
previously. I did not mean to scoop their work, but merely hoping to fix
it sooner so other people don't get confused by it just as I did today.
Github-Pull: arun11299/cpp-subprocess#106
Rebased-From: 3afe581c1f22f106d59cf54b9b65251e6c554671
2fd3f2fec6
subprocess: Fix string_arg when used with rref
When passing in a rvalue reference, compiler
considers it ambiguous between std::string and
std::string&&. Making one of them take a lvalue
reference makes compilers correctly pick the right
one depending on whether the passed in value binds
to a rvalue or lvalue reference.
Github-Pull: arun11299/cpp-subprocess#110
Rebased-From: 2d8a8eebb03e509840e2c3c755d1abf32d930f33
d3f511b458
subprocess: Get Windows return code in wait()
Currently, wait() returns 0 on windows regardless
of the actual return code of processes.
Github-Pull: arun11299/cpp-subprocess#109
Rebased-From: 04b015a8e52ead4d8bb5f0eb486419c77e418a17
647630462f
subprocess: Fix cross-compiling with mingw toolchain
subprocess: Avoid leaking POSIX name aliases beyond `subprocess.h`
The commit a32c0f3df4b6bcd1d2e93f19e8f380bb890cd507 introduced code to
silence MSVC's "warning C4996: The POSIX name for this item is
deprecated."
However, it exhibits several issues:
1. The aliases may leak into code outside the `subprocess.hpp` header.
2. They are unnecessarily applied when using the MinGW-w64 toolchain.
3. The fix is incomplete: downstream projects still see C4996 warnings.
4. The implementation lacks documentation.
This change addresses all of the above shortcomings.
Github-Pull: arun11299/cpp-subprocess#112
Rebased-From: 778543b2f2ca7f5d1c4f0241b635bbb265d750dd
Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
174bd43f2e
subprocess: Explicitly define move constructor of Streams class
This suppresses the following warning caused by clang-20.
```
error: definition of implicit copy constructor for 'Streams' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
```
Copy constructor or move constructor is called when std::vector re-allocates
memory. In this case, move constructor should be called, because copying
Streams instances breaks file-descriptor management.
Communication class is modified as well, since it's instance is a member of
Streams class.
Github-Pull: arun11299/cpp-subprocess#107
Rebased-From: 38d98d9d20be50c7187b98ac9bc9a6e66920f6ef
bb9ffea53f
subprocess: Do not escape double quotes for command line arguments on Windows
* refactor: Guard `util::quote_argument()` with `#ifdef __USING_WINDOWS__`
The `util::quote_argument()` function is specific to Windows and is used
in code already guarded by `#ifdef __USING_WINDOWS__`.
* Do not escape double quotes for command line arguments on Windows
This change fixes the handling of double quotes and aligns the behavior
with Python's `Popen` class. For example:
```
>py -3
>>> import subprocess
>>> p = subprocess.Popen("cmd.exe /c dir \"C:\\Program Files\"", stdout=subprocess.PIPE, text=True)
>>> print(f"Captured stdout:\n{stdout}")
```
Currently, the same command line processed by the `quote_argument()`
function looks like `cmd.exe /c dir "\"C:\Program" "Files\""`, which is
broken.
With this change, it looks correct: `cmd.exe /c dir "C:\Program Files"`.
Github-Pull: arun11299/cpp-subprocess#113
Rebased-From: ed313971c04ac10dc006104aba07d016ffc6542a
7423214d8d
subprocess: Proper implementation of wait() on Windows
This commit makes sure:
1. WaitForSingleObject returns with expected
code before proceeding.
2. Process handle is properly closed.
Github-Pull: arun11299/cpp-subprocess#116
Rebased-From: 625a8775791e62736f20f3fa3e6cc4f1b24aa89a
b7288decdf
subprocess: check and handle fcntl(F_GETFD) failure
Add missing error check for fcntl(fd, F_GETFD, 0) in set_clo_on_exec.
Raise OSError on failure to align with existing FD_SETFD behavior.
This improves robustness in subprocess setup and error visibility.
Github-Pull: arun11299/cpp-subprocess#117
Rebased-From: 9974ff69cdd5fc1a2722cb63f006df9308628b35
cd95c9d6a7
hebasto force-pushed
on May 1, 2025
hebasto
commented at 9:27 pm on May 1, 2025:
member
During my quick (and possibly not thorough) tests, I was unable to reproduce the issues mentioned in those PRs on our codebase.
Seems odd to not pull in all changes, given no real rationale (i.e the code being incorrect/broken) for excluding them. This also means that next time this header is updated, they may or may not get pulled in anyways, depending on the author of the change, given there’s nothing documented to say they should be excluded.
theStack
commented at 10:58 pm on May 3, 2025:
contributor
Light ACKcd95c9d6a7ec08cca0f9c98328c759be586720f8
Thanks for following up! Can’t comment much on the concrete code changes as I’m not too familiar with Windows development, but verified that all the upstream PRs are now included and were applied correctly (as a note for other reviewers, sometimes the diff is smaller here, as some commits touches code for areas that we have removed).
DrahtBot requested review from laanwj
on May 3, 2025
hebasto
commented at 7:44 pm on May 4, 2025:
member
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: 2025-05-19 09:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
