Skip to content

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Aug 21, 2025

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 21, 2025

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33233.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko, janb84
Stale ACK ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Copy link
Contributor

@janb84 janb84 left a comment

Choose a reason for hiding this comment

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

ACK 2a815d1

PR updates dependencies document to point to version bump PR of capnproto.

  • "code" review ✅

@maflcko
Copy link
Member

maflcko commented Aug 21, 2025

lgtm ACK 2a815d1

@ryanofsky
Copy link
Contributor

ryanofsky commented Aug 21, 2025

Code review ACK 2a815d1

Maybe we want to keep this PR open a little longer in case there are other small documentation updates like this? Seems fine to merge now though if preferred.

May also want to include "doc:" in PR title

Another little thing I noticed is that italics should be removed from these files in files.md since they are now included in binary releases:

bitcoin/doc/files.md

Lines 153 to 154 in 7d97894

| *libexec/bitcoin-gui* | IPC-enabled alternative to `bitcoin-qt` |
| *libexec/bitcoin-node* | IPC-enabled alternative to `bitcoind` |

@Sjors
Copy link
Member Author

Sjors commented Aug 21, 2025

Maybe we want to keep this PR open a little longer in case there are other small documentation updates like this?

Yes, I was assuming there would be more things showing up over the next few days. Though I can always open another PR.

@maflcko
Copy link
Member

maflcko commented Aug 21, 2025

Also, on Fedora you need both packages, see maflcko/b-c-nightly@14d34ab.

Suggested patch:

diff --git a/doc/build-unix.md b/doc/build-unix.md
index 38b4496687..04f757ba52 100644
--- a/doc/build-unix.md
+++ b/doc/build-unix.md
@@ -122,7 +122,7 @@ User-Space, Statically Defined Tracing (USDT) dependencies:
 
 Cap'n Proto is needed for IPC functionality (see [multiprocess.md](multiprocess.md)):
 
-    sudo dnf install capnproto
+    sudo dnf install capnproto capnproto-devel
 
 Compile with `-DENABLE_IPC=OFF` if you do not need IPC functionality.
 

@fanquake
Copy link
Member

You'll also need to add the capnp install instructions for Alpine and Arch in build-unix.md.

@fanquake fanquake added this to the 30.0 milestone Aug 21, 2025
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
@Sjors
Copy link
Member Author

Sjors commented Aug 21, 2025

Arch instruction taken from: https://capnproto.org/install.html

For Alpine I added capnproto and capnproto-dev since they both exist, but didn't test if the latter is needed.

@Sjors Sjors force-pushed the 2025/08/pr-31802-followups branch from 8245640 to de65c86 Compare August 21, 2025 16:29
@maflcko
Copy link
Member

maflcko commented Aug 21, 2025

For Alpine I added capnproto and capnproto-dev since they both exist, but didn't test if the latter is needed.

dev installs the other: https://cirrus-ci.com/task/6500081345495040?logs=install#L0

@maflcko
Copy link
Member

maflcko commented Aug 21, 2025

lgtm ACK de65c86

@DrahtBot DrahtBot requested review from ryanofsky and janb84 August 21, 2025 16:32
Copy link
Contributor

@janb84 janb84 left a comment

Choose a reason for hiding this comment

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

re ACK de65c86

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

LGTM, though suggest renaming the PR title to

doc: follow-ups to "Add bitcoin-{node,gui} to release binaries for IPC"

@Sjors Sjors changed the title IPC followups for PR 31802 doc: follow-ups to "Add bitcoin-{node,gui} to release binaries for IPC" Aug 22, 2025
@DrahtBot DrahtBot added the Docs label Aug 22, 2025
@Sjors
Copy link
Member Author

Sjors commented Aug 22, 2025

@jonatack done, although I wasn't expecting there to be only doc changes.

@fanquake
Copy link
Member

If there are more changes, there can be more PRs. We can fixup the docs now, and open more PRs when needed.

@fanquake fanquake merged commit 02f6758 into bitcoin:master Aug 22, 2025
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants