util: remove unused cpp-subprocess options #29865

theStack · 2024-04-13T10:15:28Z

The newly introduced cpp-subprocess library provides a good number of options for the Popen class:

Lines 1009 to 1020 in 0de63b8

    
           void set_option(executable&& exe); 
        
           void set_option(cwd&& cwdir); 
        
           void set_option(bufsize&& bsiz); 
        
           void set_option(environment&& env); 
        
           void set_option(defer_spawn&& defer); 
        
           void set_option(shell&& sh); 
        
           void set_option(input&& inp); 
        
           void set_option(output&& out); 
        
           void set_option(error&& err); 
        
           void set_option(close_fds&& cfds); 
        
           void set_option(preexec_func&& prefunc); 
        
           void set_option(session_leader&& sleader);

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 · 2024-04-13T10:15:31Z

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.

TheCharlatan · 2024-04-13T10:58:12Z

Nice, Concept ACK.

hebasto · 2024-04-13T19:21:46Z

Concept ACK.

hebasto · 2024-04-13T19:29:55Z

remove unused cpp-subprocess options

Are all remaining options used?

laanwj · 2024-04-14T14:23:33Z

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

theStack · 2024-04-15T14:15:36Z

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):

bitcoin/src/common/run_command.cpp

Line 29 in 22c8614

    
           auto c = sp::Popen(str_command, sp::input{sp::PIPE}, sp::output{sp::PIPE}, sp::error{sp::PIPE});

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 · 2024-04-15T14:45:42Z

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 · 2024-04-16T09:09:27Z

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.

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

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

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

theStack · 2024-04-16T12:28:47Z

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

Approach ACK 0f84733.

🚀

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

src/util/subprocess.hpp

hebasto

ACK 0f84733.

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

luke-jr · 2024-04-21T02:09:35Z

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

hebasto

re-ACK 899be1a.

src/util/subprocess.hpp

theStack · 2024-04-23T13:09:36Z

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

Diff to the previous version

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

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

hebasto

re-ACK 13adbf7.

hebasto · 2024-04-23T13:20:11Z

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 · 2024-04-23T16:58:31Z

ACK 13adbf7

hebasto · 2024-04-23T17:26:37Z

#29910 is the next one :)

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

hebasto mentioned this pull request Apr 14, 2024

Reintroduce external signer support for Windows #29868

Merged

theStack added 7 commits April 16, 2024 14:25

remove unneeded shell option from cpp-subprocess

633e45b

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

remove unneeded preexec function option from cpp-subprocess

cececad

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

remove unneeded defer_spawn option from cpp-subprocess

80d008c

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

remove unneeded session_leader option from cpp-subprocess

62db8f8

remove unneeded close_fds option from cpp-subprocess

79c3036

remove unneeded bufsize option from cpp-subprocess

03ffb09

remove unneeded cwd option from cpp-subprocess

2088777

theStack force-pushed the 202404-remove-unused-cpp-subprocess-options branch from a5368c1 to 0f84733 Compare April 16, 2024 12:28

hebasto reviewed Apr 18, 2024

View reviewed changes

achow101 reviewed Apr 18, 2024

View reviewed changes

src/util/subprocess.hpp Outdated Show resolved Hide resolved

hebasto approved these changes Apr 19, 2024

View reviewed changes

DrahtBot requested review from TheCharlatan and laanwj April 19, 2024 09:29

hebasto mentioned this pull request Apr 19, 2024

refactor: Rename subprocess.hpp to follow our header name conventions #29910

Merged

theStack force-pushed the 202404-remove-unused-cpp-subprocess-options branch from 0f84733 to 899be1a Compare April 23, 2024 13:02

hebasto approved these changes Apr 23, 2024

View reviewed changes

src/util/subprocess.hpp Outdated Show resolved Hide resolved

remove unneeded environment option from cpp-subprocess

13adbf7

theStack force-pushed the 202404-remove-unused-cpp-subprocess-options branch from 899be1a to 13adbf7 Compare April 23, 2024 13:10

hebasto approved these changes Apr 23, 2024

View reviewed changes

hebasto mentioned this pull request Apr 23, 2024

Replace Boost.Process with cpp-subprocess #28981

Merged

achow101 merged commit 2cecbbb into bitcoin:master Apr 23, 2024

theStack deleted the 202404-remove-unused-cpp-subprocess-options branch April 23, 2024 17:05

laanwj mentioned this pull request Oct 24, 2024

run_command: Close non-std fds when execing slave processes #30756

Closed

bitcoin locked and limited conversation to collaborators Apr 23, 2025

	void set_option(executable&& exe);
	void set_option(cwd&& cwdir);
	void set_option(bufsize&& bsiz);
	void set_option(environment&& env);
	void set_option(defer_spawn&& defer);
	void set_option(shell&& sh);
	void set_option(input&& inp);
	void set_option(output&& out);
	void set_option(error&& err);
	void set_option(close_fds&& cfds);
	void set_option(preexec_func&& prefunc);
	void set_option(session_leader&& sleader);

util: remove unused cpp-subprocess options #29865

util: remove unused cpp-subprocess options #29865

Uh oh!

Conversation

theStack commented Apr 13, 2024

Uh oh!

DrahtBot commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

TheCharlatan commented Apr 13, 2024

Uh oh!

hebasto commented Apr 13, 2024

Uh oh!

hebasto commented Apr 13, 2024

Uh oh!

laanwj commented Apr 14, 2024

Uh oh!

theStack commented Apr 15, 2024

Uh oh!

laanwj commented Apr 15, 2024

Uh oh!

hebasto commented Apr 16, 2024

Uh oh!

theStack commented Apr 16, 2024

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Apr 21, 2024

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Apr 23, 2024

Uh oh!

hebasto commented Apr 23, 2024

Uh oh!

Uh oh!

DrahtBot commented Apr 13, 2024 •

edited

Loading

hebasto left a comment •

edited

Loading

theStack commented Apr 23, 2024 •

edited

Loading

hebasto commented Apr 23, 2024 •

edited

Loading