util: remove unused cpp-subprocess options #29865

1. 
   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.
2. 
   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.

3. DrahtBot added the label Utils/log/libs on Apr 13, 2024
4. 
   TheCharlatan commented at 10:58 am on April 13, 2024: contributor

   Nice, Concept ACK.
5. 
   hebasto commented at 7:21 pm on April 13, 2024: member

   Concept ACK.
6. 
   hebasto commented at 7:29 pm on April 13, 2024: member

   remove unused cpp-subprocess options

   Are all remaining options used?

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

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

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

11. 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
12. remove unneeded preexec function option from cpp-subprocess

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

    cececad7b2
13. remove unneeded defer_spawn option from cpp-subprocess

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

    80d008c66d
14. remove unneeded session_leader option from cpp-subprocess 62db8f8e5a
15. remove unneeded close_fds option from cpp-subprocess 79c3036373
16. remove unneeded bufsize option from cpp-subprocess 03ffb09c31
17. remove unneeded cwd option from cpp-subprocess 2088777ba0
18. theStack force-pushed on Apr 16, 2024
19. 
    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.
20. 
    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

21. 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.
22. hebasto approved
23. 
    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.

24. DrahtBot requested review from TheCharlatan on Apr 19, 2024
25. DrahtBot requested review from laanwj on Apr 19, 2024
26. 
    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?
27. theStack force-pushed on Apr 23, 2024
28. 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:

    0                            NULL,         // use parent's environment
    

    ---

    

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

    Thanks, done.
29. hebasto approved
30. 
    hebasto commented at 1:07 pm on April 23, 2024: member

    re-ACK 899be1a7643040c32510903dab06cea5ef7a5ffd.
31. 
    theStack commented at 1:09 pm on April 23, 2024: contributor

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

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

    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.

32. remove unneeded environment option from cpp-subprocess 13adbf733f
33. theStack force-pushed on Apr 23, 2024
34. hebasto approved
35. 
    hebasto commented at 1:11 pm on April 23, 2024: member

    re-ACK 13adbf733f09c73c3cf0025d94c52f9cec5dba3b.
36. 
    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

37. 
    achow101 commented at 4:58 pm on April 23, 2024: member

    ACK 13adbf733f09c73c3cf0025d94c52f9cec5dba3b
38. achow101 merged this on Apr 23, 2024
39. achow101 closed this on Apr 23, 2024

    ---

40. theStack deleted the branch on Apr 23, 2024
41. 
    hebasto commented at 5:26 pm on April 23, 2024: member

    #29910 is the next one :)

Contributors
theStack DrahtBot TheCharlatan hebasto laanwj achow101 luke-jr

Review Requested
laanwj TheCharlatan

Labels
Utils/log/libs