refactor: remove remaining unused code from cpp-subprocess #29961

pull theStack wants to merge 4 commits into bitcoin:master from theStack:202404-further-subprocess-removals changing 1 files +18 −96

theStack commented at 3:56 PM on April 25, 2024: contributor

This PR removes remaining code that is unused within the cpp-subprocess module (templates and commented out code). Happy to add more removals if anyone finds more unused parts. Note that there are some API functions of the Popen class that we don't use, e.g. wait(), pid(), poll(), kill(), but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.

remove unused templates from cpp-subprocess

The templates `is_ready`, `param_pack`, and `has_type` are not used
anywhere, so let's remove them.

ff79adbe05

remove commented out code in cpp-subprocess 908c51fe4a
DrahtBot commented at 3:56 PM on April 25, 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 fjahr, hebasto, achow101

Stale ACK Sjors

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:
- #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 Refactoring on Apr 25, 2024
hebasto commented at 4:22 PM on April 25, 2024: member

Concept ACK.
fanquake commented at 7:59 AM on April 26, 2024: member

Note that there are some API functions of the Popen class that we don't use, e.g. wait(), pid(), poll(), kill(), but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.

Deleting as much as possible seems fine. If code turns out to be needed in future (although I wouldn't think we are going to expand this interface), it can be retrieved from the git history.
Sjors commented at 8:41 AM on April 26, 2024: member

As long as #29868 works on top of this, it's fine by me. I'll do some testing. Not that I would expect this to break anything as long as it still compiles.

Tested the combination on Ubuntu 23.10 and Windows.

tACK 908c51fe4afeba0af500c6275027b1afa1b3bd19
hebasto approved
hebasto commented at 9:16 AM on April 26, 2024: member

ACK 908c51fe4afeba0af500c6275027b1afa1b3bd19. It is compatible with ~https://github.com/bitcoin/bitcoin/pull/29961~ #29868.

I'd suggest to remove the mentioning of the check_output function from all comments. That function was not even pulled from the upstream.
hebasto commented at 9:17 AM on April 26, 2024: member

Note that there are some API functions of the Popen class that we don't use, e.g. wait(), pid(), poll(), kill(), but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.

Deleting as much as possible seems fine. If code turns out to be needed in future (although I wouldn't think we are going to expand this interface), it can be retrieved from the git history.

I agree to the deletion.

UPD. See #29961 (review).
theStack commented at 12:48 PM on April 27, 2024: contributor

Thanks for review and the feedback. I added commits to remove unused Popen methods (poll and kill; note that pid and wait are used internally) and another one that removes obsolete check_output comments and also gets rid of the Popen API numbering.
in src/util/subprocess.h:907 in ed39d261dc outdated
```
 903 | @@ -904,8 +904,6 @@ class Streams
 904 |   * 3. wait()             - Wait for the child to exit.
 905 |   * 4. retcode()          - The return code of the exited child.
 906 |   * 5. pid()              - PID of the spawned child.
 907 | - * 6. poll()             - Check the status of the running child.
 908 | - * 7. kill(sig_num)      - Kill the child. SIGTERM used by default.
 909 |   * 8. send(...)          - Send input to the input channel of the child.
```
fjahr commented at 8:30 PM on April 27, 2024:

The numbering of this list is off after this change. That might be confusing to someone in the future. I would suggest either updating the following numbering or something like.

* 6. (deleted) * 7. (deleted)
theStack commented at 12:51 AM on April 28, 2024:

See the last commit where the numbering is removed. Updating them is also possible of course, but it seems that these numbers don't provide any value.

fjahr commented at 1:47 PM on April 28, 2024:

Ok, strange, I guess I was looking at an outdated version of the code or something.
remove unused method `Popen::kill` from cpp-subprocess 97f159776e

Type	Reviewers
ACK	fjahr, hebasto, achow101
Stale ACK	Sjors

update comments in cpp-subprocess (check_output references)

Remove obsolete `check_output` references in the comments and remove
the numbering of the Popen API methods, as they don't seem to provide a
value and just make diffs larger for future changes.

8b52e7f628

in src/util/subprocess.h:1027 in 7ebddf5254 outdated
```
 968 |  
 969 |    int retcode() const noexcept { return retcode_; }
 970 |  
 971 |    int wait() noexcept(false);
 972 |  
 973 | -  int poll() noexcept(false);
```
hebasto commented at 5:43 AM on April 28, 2024:

ed39d261dc056e700b79da769e085c8af389451d:

The Popen::poll() function is used in #29868. I hope that an improved/fixed Windows implementation of the Popen::retcode(), which I don't have now, will allow to drop it.

theStack commented at 12:25 PM on April 28, 2024:

Oh sorry, I missed that. Force-pushed now with a commit where only Popen::kill() is removed (and Popen::poll() is kept as-is).
theStack force-pushed on Apr 28, 2024
fjahr commented at 2:06 PM on April 28, 2024: contributor

ACK 908c51f. It is compatible with #29961. @hebasto #29961 is this PR here, did you mean #29868?
fjahr commented at 2:07 PM on April 28, 2024: contributor

Code review ACK 8b52e7f628304e83b0e36fd97e617de0f71c5a62
DrahtBot requested review from hebasto on Apr 28, 2024
DrahtBot requested review from Sjors on Apr 28, 2024
hebasto approved
hebasto commented at 4:33 PM on April 28, 2024: member

ACK 8b52e7f628304e83b0e36fd97e617de0f71c5a62.
achow101 commented at 8:17 PM on May 2, 2024: member

ACK 8b52e7f628304e83b0e36fd97e617de0f71c5a62
achow101 merged this on May 2, 2024
achow101 closed this on May 2, 2024
hebasto commented at 12:58 PM on May 8, 2024: member

#29961 (review):

The Popen::poll() function is used in #29868. I hope that an improved/fixed Windows implementation of the Popen::retcode(), which I don't have now, will allow to drop it.

That is no longer the case with current implementation of #29868.
theStack deleted the branch on May 8, 2024
fanquake referenced this in commit b47c393d8a on May 11, 2024
bitcoin locked this on May 8, 2025

Contributors

Review Requested
Sjors

Labels

Refactoring

Linked (view graph)

#30081 refactor: Remove unused code from `subprocess.h` header #30756 run_command: Close non-std fds when execing slave processes #32343 common: Close non-std fds before exec in RunCommandJSON

refactor: remove remaining unused code from cpp-subprocess #29961

Code Coverage

Reviews

Conflicts