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

[DO NOT MERGE] Replace Boost.Process with cpp-subprocess #29490

Closed
wants to merge 4 commits into from
Closed

[DO NOT MERGE] Replace Boost.Process with cpp-subprocess #29490

wants to merge 4 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Feb 27, 2024

Differences from #28981:

  1. Dropped Windows-related changes, which allows to keep
  2. Zero diff for the subprocess.hpp
  3. Based on test: Remove Windows-specific code from system_tests/run_command #29489

More details to be added soon :)

@hebasto
test: Remove Windows-specific code from system_tests/run_command
51bc1c7 
This code has been dead since bitcoin#28967.

Required as a precondition for replacing Boost.Process with
cpp-subprocess to make diff for this code meaningful and reviewable.

The plan is to reintroduce Windows-specific code in this test
simultaneously with enabling Windows support in cpp-subprocess.
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 27, 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.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29479 (test: Refactor subtree exclusion in lint tests by BrandonOdiwuor)
  • #28981 (Replace Boost.Process with cpp-subprocess by hebasto)
  • #28286 (test: Minor fix in test - locale in terminal by crywolf)
  • #27897 (guix: use GCC 12.3.0 to build releases by fanquake)
  • #25972 (build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set by fanquake)
  • #22417 (util/system: Close non-std fds when execing slave processes by luke-jr)

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.

@fanquake
Copy link
Member

Is there a reason this is a new PR, rather than pushing to the current one? Can the other one be closed?

@hebasto
Copy link
Member Author

hebasto commented Feb 27, 2024

Is there a reason this is a new PR, rather than pushing to the current one?

To demonstrate the CI outcome for this approach to reviewers.

Can the other one be closed?

I'll close it once this one is undrafted.

@fanquake
Copy link
Member

fanquake commented Feb 27, 2024 •
edited

I don't really understand, because pushing to the other PR would also demonstrate the CI, and by switching to this PR, we'll now loose all the past discussion?

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/22040112760

hebasto and others added 3 commits February 28, 2024 00:16
@hebasto
Add cpp-subprocess header-only library
67ca33b 
Upstream repo: https://github.com/arun11299/cpp-subprocess
Commit: 4025693decacaceb9420efedbf4967a04cb028e7

The "Convenience Functions" section is unused in our codebase, so it has
been removed.
@theStack @hebasto
external_signer: replace boost::process with cpp-subprocess
1c5ad9a 
This primarily affects the `RunCommandParseJSON` utility function.
@theStack @hebasto
build: remove boost::process dependency for building external signer …
09d468e 
…support
This was referenced Feb 28, 2024
Merged
Merged
Closed
Merged
@hebasto hebasto closed this Feb 28, 2024
@DrahtBot DrahtBot mentioned this pull request Feb 28, 2024
Merged
@hebasto hebasto deleted the 240227-replace-bp branch February 28, 2024 10:38
@DrahtBot DrahtBot mentioned this pull request Feb 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hebasto @DrahtBot @fanquake @theStack