Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from May 2, 2024
Merged

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

merged 4 commits into from May 2, 2024

Conversation

theStack
Copy link
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.

@theStack
remove unused templates from cpp-subprocess
ff79adb 
The templates `is_ready`, `param_pack`, and `has_type` are not used
anywhere, so let's remove them.
@theStack
remove commented out code in cpp-subprocess
908c51f
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 25, 2024 •
edited

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.

@hebasto
Copy link
Member

hebasto commented Apr 25, 2024

Concept ACK.

@DrahtBot DrahtBot mentioned this pull request Apr 25, 2024
Open
@fanquake
Copy link
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
Copy link
Member

Sjors commented Apr 26, 2024 •
edited

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 908c51f

hebasto
hebasto approved these changes Apr 26, 2024
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 908c51f. It is compatible with #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
Copy link
Member

hebasto commented Apr 26, 2024 •
edited

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 (comment).

@theStack
Copy link
Contributor Author

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.

fjahr
fjahr reviewed Apr 27, 2024
View reviewed changes
src/util/subprocess.h Outdated Show resolved Hide resolved
@theStack
remove unused method Popen::kill from cpp-subprocess
97f1597
@theStack
update comments in cpp-subprocess (check_output references)
8b52e7f 
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.
@theStack theStack force-pushed the 202404-further-subprocess-removals branch from 7ebddf5 to 8b52e7f Compare April 28, 2024 12:24
@fjahr
Copy link
Contributor

fjahr commented Apr 28, 2024 •
edited

ACK 908c51f. It is compatible with #29961.

@hebasto #29961 is this PR here, did you mean #29868?

@fjahr
Copy link
Contributor

fjahr commented Apr 28, 2024

Code review ACK 8b52e7f

@DrahtBot DrahtBot requested review from hebasto and Sjors April 28, 2024 14:07
hebasto
hebasto approved these changes Apr 28, 2024
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8b52e7f.

@achow101
Copy link
Member

achow101 commented May 2, 2024

ACK 8b52e7f

@achow101 achow101 merged commit 81174d8 into bitcoin:master May 2, 2024
16 checks passed
@hebasto
Copy link
Member

hebasto commented May 8, 2024

#29961 (comment):

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 theStack deleted the 202404-further-subprocess-removals branch May 8, 2024 14:22
@hebasto hebasto mentioned this pull request May 10, 2024
Merged
fanquake added a commit that referenced this pull request May 11, 2024
@fanquake
Merge #30081: refactor: Remove unused code from subprocess.h header
b47c393 
5a11d30 refactor, subprocess: Remove unused stream API calls (Hennadii Stepanov)
05b6f87 refactor, subprocess: Remove unused `Popen::child_created_` data member (Hennadii Stepanov)
9e1ccf5 refactor, subprocess: Remove unused `Popen::poll()` (Hennadii Stepanov)
24b53fc refactor, subprocess: Remove `Popen::pid()` (Hennadii Stepanov)

Pull request description:

  This PR continues #29961.

  Please note that `Popen::poll()` is not required for #29868 anymore.

ACKs for top commit:
  theuni:
    Easy code review ACK 5a11d30 since it's all removals :)
  theStack:
    Code-review ACK 5a11d30

Tree-SHA512: 11f984f8384c337782d058afa80011e88087a1b5a3ed6e4678d492e6c232b706a26312463c5dd8c529aa477497c8afca9f54574e0e36e3edd5675bd5d8424bbb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjahr fjahr fjahr left review comments

@hebasto hebasto hebasto approved these changes

@Sjors Sjors Awaiting requested review from Sjors

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@theStack @DrahtBot @hebasto @fanquake @Sjors @fjahr @achow101