util: remove unused cpp-subprocess options #29865

pull theStack wants to merge 8 commits into bitcoin:master from theStack:202404-remove-unused-cpp-subprocess-options changing 1 files +9 −385
  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.

    image :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 :)
  42. bitcoin locked this on Apr 23, 2025


theStack DrahtBot TheCharlatan hebasto laanwj achow101 luke-jr


laanwj TheCharlatan

Labels
Utils/log/libs

github-metadata-mirror

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-10-31 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me