util: remove unused cpp-subprocess options #29865

theStack commented at 10:15 AM on April 13, 2024: contributor

The newly introduced cpp-subprocess library provides a good number of options for the Popen class: https://github.com/bitcoin/bitcoin/blob/0de63b8b46eff5cda85b4950062703324ba65a80/src/util/subprocess.hpp#L1009-L1020 Some of them are either not fully implemented (shell, missing an implementation on Windows), implemented in an ugly way (e.g. using "Impoverished, meager, needy, truly needy version of type erasure" for preexec_func according to the author's own words) or simply unlikely to be ever needed for our external signer use-case (defer_spawn). Instead of maintaining incomplete and/or unneeded code, I'd suggest to get rid of it and only keep support for options if there is a strong reason for it.

DrahtBot commented at 10:15 AM on April 13, 2024: contributor

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	hebasto, achow101
Concept ACK	TheCharlatan, laanwj

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:

#29910 (refactor: Rename subprocess.hpp to follow our header name conventions by hebasto)
#29868 (Reintroduce external signer support for Windows by hebasto)

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.

DrahtBot added the label Utils/log/libs on Apr 13, 2024

TheCharlatan commented at 10:58 AM on April 13, 2024: contributor

Nice, Concept ACK.

hebasto commented at 7:21 PM on April 13, 2024: member

Concept ACK.

hebasto commented at 7:29 PM on April 13, 2024: member

remove unused cpp-subprocess options

Are all remaining options used?

laanwj commented at 2:23 PM on April 14, 2024: member

Concept ACK, but could it be that we could need some of these? Eg close_fds sounds like what's described in #22417

theStack commented at 2:15 PM on April 15, 2024: contributor

remove unused cpp-subprocess options

Are all remaining options used?

From the 12 available cpp-subprocess options we currently only use 4 (namely executable, input, output and error): https://github.com/bitcoin/bitcoin/blob/22c86140f8fe4f13b7b5415ff62922e497fd4948/src/common/run_command.cpp#L29 I.e. the other 8 are unused. This PR currently removes 3 of the 8 unused ones -- the remaining 5 (cwd, bufsize, environment, close_fds, session_leader) seemed to be common enough to be useful at some point in the future, but my choice is more based on gut feeling than anything else, as I don't know what future use-cases could be. Concrete suggestions are very welcome. For example, setting environment variables seems like it could be useful, but the code is quite complex for Windows.

Concept ACK, but could it be that we could need some of these? Eg close_fds sounds like what's described in #22417

Good question. Note that close_fds is currently not removed.

laanwj commented at 2:45 PM on April 15, 2024: member

For example, setting environment variables seems like it could be useful, but the code is quite complex for Windows.

Agree that that seems useful. I vaguely remember we even had a use-case for that once, but don't remember it clearly, we did give it up because it was too hard to do in windows.

Good question. Note that close_fds is currently not removed.

Oh, whoops, sorry, didn't notice that.

FWIW I think it's a valid choice to remove what we're not using and re-introduce it when we do, the code is out there there's little point in keeping unused code in the repository.

hebasto commented at 9:09 AM on April 16, 2024: member

FWIW I think it's a valid choice to remove what we're not using and re-introduce it when we do, the code is out there there's little point in keeping unused code in the repository.

I agree.

remove unneeded shell option from cpp-subprocess

We don't use this option and it's not supported on Windows anyways,
so we can get as well rid of it.

633e45b2e2

remove unneeded preexec function option from cpp-subprocess

We don't seem to ever need this, so remove it.

cececad7b2

remove unneeded defer_spawn option from cpp-subprocess

For our use-case, there doesn't seem to be any need for this.

80d008c66d

remove unneeded session_leader option from cpp-subprocess 62db8f8e5a

remove unneeded close_fds option from cpp-subprocess 79c3036373

remove unneeded bufsize option from cpp-subprocess 03ffb09c31

remove unneeded cwd option from cpp-subprocess 2088777ba0

theStack force-pushed on Apr 16, 2024

theStack commented at 12:28 PM on April 16, 2024: contributor

Thanks a lot for you feedback @laanwj, @hebasto, I've now removed support for all options that we don't use. Especially for the environment option, lots of Windows-specific code can be removed.

hebasto commented at 4:02 PM on April 18, 2024: member

Approach ACK 0f847336428153042dbf226caf52ecd43188f22e.

:rocket:

I have reviewed all commits except the last one (it's still in progress).

The CI run of the combined branch [ this PR + #29868 ] is available here: https://github.com/hebasto/bitcoin/actions/runs/8740663547/job/23984855961

in src/util/subprocess.hpp:1213 in 0f84733642 outdated

1210 | @@ -1336,11 +1211,6 @@ inline void Popen::execute_process() noexcept(false)
1211 |  {
1212 |  #ifdef __USING_WINDOWS__
1213 |    void* environment_string_table_ptr = nullptr;

achow101 commented at 5:38 PM on April 18, 2024:

In 0f847336428153042dbf226caf52ecd43188f22e "remove unneeded environment option from cpp-subprocess"

This variable seems to also be unused now as it is never set to anything. It's only passed in as an argument to CreateProcessW which could take the nullptr directly.

theStack commented at 1:03 PM on April 23, 2024:

Thanks, done. I passed NULL rather than nullptr, cause that's what's used for the other null-parameter as well, but it shouldn't make a difference.

hebasto approved

hebasto commented at 9:29 AM on April 19, 2024: member

ACK 0f847336428153042dbf226caf52ecd43188f22e.

I'm happy to re-ACK if the #29865 (review) will be addressed.

DrahtBot requested review from TheCharlatan on Apr 19, 2024

DrahtBot requested review from laanwj on Apr 19, 2024

luke-jr commented at 2:09 AM on April 21, 2024: member

This seems likely to have a high maintenance cost. What's the benefit?

theStack force-pushed on Apr 23, 2024

in src/util/subprocess.hpp:1259 in 899be1a764 outdated

1255 | @@ -1549,7 +1256,7 @@ inline void Popen::execute_process() noexcept(false)
1256 |                              NULL,         // primary thread security attributes
1257 |                              TRUE,         // handles are inherited
1258 |                              creation_flags,	// creation flags
1259 | -                            environment_string_table_ptr,  // use provided environment
1260 | +                            NULL,         // use environment of the calling process

hebasto commented at 1:07 PM on April 23, 2024:

nit: Maybe a wording more consistent with other comment:

                            NULL,         // use parent's environment

theStack commented at 1:11 PM on April 23, 2024:

Thanks, done.

hebasto approved

hebasto commented at 1:07 PM on April 23, 2024: member

re-ACK 899be1a7643040c32510903dab06cea5ef7a5ffd.

theStack commented at 1:09 PM on April 23, 2024: contributor

Force-pushed with the last commit changed to tackle #29865 (review).

<details> <summary>Diff to the previous version</summary>

diff --git a/src/util/subprocess.hpp b/src/util/subprocess.hpp
index c7f9a685d1..a85c0b0bff 100644            
--- a/src/util/subprocess.hpp                                                                            
+++ b/src/util/subprocess.hpp         
@@ -1210,8 +1210,6 @@ inline void Popen::kill(int sig_num)
 inline void Popen::execute_process() noexcept(false)     
 {   
 #ifdef __USING_WINDOWS__                                                                                
-  void* environment_string_table_ptr = nullptr;
-                                              
   if (exe_name_.length()) {                                                                             
     this->vargs_.insert(this->vargs_.begin(), this->exe_name_);
     this->populate_c_argv();
@@ -1258,7 +1256,7 @@ inline void Popen::execute_process() noexcept(false)
                             NULL,         // primary thread security attributes
                             TRUE,         // handles are inherited
                             creation_flags,    // creation flags
-                            environment_string_table_ptr,  // use provided environment
+                            NULL,         // use environment of the calling process
                             NULL,         // use parent's current directory
                             &siStartInfo, // STARTUPINFOW pointer
                             &piProcInfo); // receives PROCESS_INFORMATION

</details> [@luke-jr](/bitcoin-bitcoin/contributor/luke-jr/): > This seems likely to have a high maintenance cost. What's the benefit?

Do you ask what's the benefit of using cpp-subprocess in general (instead of boost::process) or the benefit of removing code that we don't use? If it's the latter and you're worried that this makes backporting upstream fixes harder, I'd argue that there is hardly any activity upstream, the most recent fixes have been done by @hebasto.

remove unneeded environment option from cpp-subprocess 13adbf733f

theStack force-pushed on Apr 23, 2024

hebasto approved

hebasto commented at 1:11 PM on April 23, 2024: member

re-ACK 13adbf733f09c73c3cf0025d94c52f9cec5dba3b.

hebasto commented at 1:20 PM on April 23, 2024: member

This seems likely to have a high maintenance cost.

As discussed in #28981 and during the IRC meeting, we are going to treat the new code as our own:

... 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 ?

14:16 <fanquake> Obviously having a static header in our repo is an improvement, similar to nanobench/tinyformat

achow101 commented at 4:58 PM on April 23, 2024: member

ACK 13adbf733f09c73c3cf0025d94c52f9cec5dba3b

achow101 merged this on Apr 23, 2024

achow101 closed this on Apr 23, 2024

theStack deleted the branch on Apr 23, 2024

hebasto commented at 5:26 PM on April 23, 2024: member

#29910 is the next one :)

bitcoin locked this on Apr 23, 2025