The current documentation has multiple issues:
pycapnp module.python3-zmq is a package name in some Linux distros, rather than a Unix-wide convention.pyzmq from PyPI works in general, not only on macOS.lsof dependency is undocumented.This PR resolves these issues. Additionally, two minor improvements have been added (see commit messages).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35395.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Looks like you've just removed immediately usable OS specific instructions, and replaced them with general ones, just forcing devs to go and find the same info? i.e currently a FreeBSD dev could just copy pkg install python3 databases/py-sqlite3 net/py-pyzmq, now they'd have to go and find the package names themselves?
Looks like you've just removed immediately usable OS specific instructions, and replaced them with general ones, just forcing devs to go and find the same info? i.e currently a FreeBSD dev could just copy
pkg install python3 databases/py-sqlite3 net/py-pyzmq, now they'd have to go and find the package names themselves?
That's correct, it is a trade-off. While it does require a quick search from the developer, the alternative is duplicating every dependency instruction across all OS-specific notes, e.g., pkg install lsof or pip3 install pycapnp. As we saw with recent updates (#33201 and #34256), trying to maintain exact, OS-specific package names quickly becomes a fragile approach.
trying to maintain exact, OS-specific package names quickly becomes a fragile approach.
Not sure I agree. That's what we do in the rest of the build documentation, and it seems to work fine.
I haven't looked, but are the bsd docs largely different? if not, maybe they can be merged into a single build-bsd.md in the style of build-unix.md from #35402?
Going further, maybe we can just merge build-bsd.md and build-unix.md into a single build.md (and only keep build-osx and build-windows), or possibly merge even them into a separate section in a general build.md file?
Thanks for documenting these, concept ACK
42 | +##### `sqlite3` 43 | + 44 | +Required for wallet tests. 45 | + 46 | +Most Python installations include the [`sqlite3`](https://docs.python.org/3/library/sqlite3.html) optional module by default. 47 | +If that is not the case, look for documentation from your OS package manager, as it might be vendored as a separate package.
If FreeBSD is the only one sticking out here, it would be fine to briefly mention databases/py-sqlite3 for FreeBSD, but not strong opinion.
If FreeBSD is the only one sticking out here,
It isn't. I still don't quite understand how removing working instructions is an "improvement", especially given the rationale is that it's impossible to maintain individual package names (even though we do it everywhere else just fine).
I still don't quite understand how removing working instructions is an "improvement", especially given the rationale is that it's impossible to maintain individual package names (even though we do it everywhere else just fine).
I'm open to alternative suggestions on how to fix the issues mentioned in the PR description in way that prevents documentation from rotting in the future.
I don't mind either way. Just listing the Pip names is fine, or listing all.
If all should be listed, it can be done with a compact markdown table, like:
| Dependency | Python | zmq | capnp | sqlite3 |
| :--- | :--- | :--- | :--- | :--- |
| **Pip** | n/a | `pyzmq` | `pycapnp` | n/a |
| **Ubuntu/Debian** | `python3` | `python3-zmq` | n/a | n/a |
| **FreeBSD** | `python3` | `net/py-pyzmq` | n/a | `databases/py-sqlite3` |
| **NetBSD** | `python313` | `py313-zmq` | n/a | n/a |
| **OpenBSD** | `python` | `py3-zmq` | n/a | n/a |
Rendered:
| Test dependency | Python | zmq | capnp | sqlite3 |
|---|---|---|---|---|
| Pip | n/a | pyzmq |
pycapnp |
n/a |
| Ubuntu/Debian | python3 |
python3-zmq |
n/a | n/a |
| FreeBSD | python3 |
net/py-pyzmq |
n/a | databases/py-sqlite3 |
| NetBSD | python313 |
py313-zmq |
n/a | n/a |
| OpenBSD | python |
py3-zmq |
n/a | n/a |
35 | -- on Unix, run `sudo apt-get install python3-zmq` 36 | -- on mac OS, run `pip3 install pyzmq` 37 | +#### Python optional modules 38 | 39 | -The IPC functional test requires a python IPC library. `pip3 install pycapnp` may work, but if not, install it from source: 40 | +Some tests require optional modules. If a module is not installed, the tests will be skipped rather than failed.
Maybe add a sentence that the docs here only mention the pip package names, which can be installed via the system package manager (if they are offered), or via pip in a venv, or uv, or any other choice the user prefers?
Would you mind suggesting the exact wording?
Maybe this?
The optional modules can be installed with any suitable tool, like the system package manager, `pip` in a virtual environment, or `uv`.
58 | + 59 | +Required for IPC tests. 60 | + 61 | +The `capnp` module is generally available as the [`pycapnp`](https://pypi.org/project/pycapnp/) Python package. 62 | + 63 | +If installing the package via `pip` fails, install it from source:
Do we recall why this was needed and if it is still needed? If not, it can be removed?
I'm going to test a few platforms.
The package installation via pip works flawlessly: https://github.com/hebasto/github-actions/actions/runs/28876798270.
Should we remove the instructions then?
Yeah, should be fine to remove in a new commit. If it is ever needed in the future, it will be trivial to revert that commit.
On my local NetBSD 10.1, pip3 install pycapnp works just fine. However, on the same OS in the CI, a quirk is necessary:
CXXFLAGS="-DKJ_NO_EXCEPTIONS=0" pip3.13 install pycapnp
I'm fine with that for the following reasons:
capnp package.
Interesting. Though, I presume a NetBSD 10.1 error wasn't the reason why the section was originally added?
Also, I wonder if installing from source also requires enabling exceptions, or if this is just a pip package and NetBSD-package thing on NetBSD 10.1?
Also, I wonder what the error message is in case exceptions are disabled. If the error message literally says to set CXXFLAGS="-DKJ_NO_EXCEPTIONS=0", then it seems fine to just let the error message do the heavy lifting here instead of adding it to the docs?
Here are more details: https://github.com/bitcoin-core/libmultiprocess/pull/196#issuecomment-3684300640.
208 | @@ -209,7 +209,7 @@ pkill -9 bitcoind 209 | ``` 210 | 211 | 212 | -##### Data directory cache 213 | +#### Data directory cache 214 | 215 | A pre-mined blockchain with 200 blocks is generated the first time a
Can be removed after 8bfb422de83654c18cd341de8eb8e5351959d998 ?
(Happy to address the nits here in a separate pull, as they are somewhat unrelated)
(Happy to address the nits here in a separate pull, as they are somewhat unrelated)
Go for it :)
Actually, I think this refers to running tests individually, not to running the tests with the test_runner, so the docs here still apply and can be kept for now.
Feel free to resolve this thread and push your rebase, with or without the other nits. Happy to re-ack either way.
76 | -```bash 77 | -pkg install python3 databases/py-sqlite3 net/py-pyzmq 78 | -``` 79 | ---- 80 | +See [`test/README.md`](/test/README.md#dependencies-and-prerequisites). 81 |
nit in 807b77cc978d44a4b22267473283b4de6c43b62f: When adding those to *bsd.md, why not add them to *unix.md as well?
lgtm, just some nits
review ACK 61226c09c137cc552976cbc86fb69da3c12fc295 🐡
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 61226c09c137cc552976cbc86fb69da3c12fc295 🐡
sGJAuu2CrT3orY5rhqcct7ih5+h56G7KQYBzbPy1/WlHTDbf5BUktHfI2CcZB/tFBpFEXYbMTBukQf2RhFZLDQ==
</details>
Rebased and addressed some of the feedback.
nit in 4f6aa41e4b4219b56842cad7120f5289dbbede40: Bad rebase?
only change is rebase.
re-ACK 0e1e0d1d8573a581b7d0a61a5d9fef1525723425 🗨
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 0e1e0d1d8573a581b7d0a61a5d9fef1525723425 🗨
UhvUQl8X2jK/OK1ddcEmHhM2trWYI6rIKsc7KWTLBfbbzbC1t9KuP93GspZStR9Zhtln5586PX7w4dscaT2+Bw==
</details>
Additionally, switch the example code to PowerShell to match our other
Windows docs.
The current documentation has multiple issues:
1. OS-specific notes are missing for the `pycapnp` module.
2. `python3-zmq` is a package name in some Linux distros, rather than a
Unix-wide convention.
3. Installing `pyzmq` from PyPI works in general, not only on macOS.
Resolve these issues by consolidating and fixing the documentation.
Thx. Still curious about the pip error (https://github.com/bitcoin/bitcoin/pull/35395/changes/e25d9c8addd5ff489a7dac8b767e7901a5359f49#r3389204933), but this seems pre-existing and orthogonal.
review ACK c14f888996d45bc60af4a9e7023d836dff66bcb9 🏣
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK c14f888996d45bc60af4a9e7023d836dff66bcb9 🏣
TcQoY0WUul655XhC3MTHu6HevFIlTp+ndnyWgNaK6Mkrv6d/E+roJXDdFPCji6aVMYBoKpRYWQXTeEumDrVYCA==
</details>
97 | + 98 | +##### `lsof` 99 | + 100 | +Required for some network tests. 101 | + 102 | +The availability of the `lsof` command depends on your operating system.
What's the point of this sentence?
It's not available on OpenBSD.
45 | @@ -46,10 +46,10 @@ for additional features in later columns are optional. 46 | 47 | | Package manager | Required build dependencies | SQLite (wallet) | Cap'n Proto (IPC) | ZMQ <sup><a href="#note1">[1]</a></sup> | USDT | Qt and libqrencode (GUI) | 48 | | ----------------------- | --------------------------- | --------------- | ----------------- | --- | ---- | ------------------------ | 49 | -| Debian / Ubuntu (`apt`) | `build-essential cmake python3 libboost-dev` | `libsqlite3-dev` | `libcapnp-dev capnproto` | `libzmq3-dev pkgconf` | `systemtap-sdt-dev` | `qt6-base-dev qt6-tools-dev qt6-l10n-tools qt6-tools-dev-tools libgl-dev qt6-wayland libqrencode-dev` | 50 | -| Fedora (`dnf`) | `gcc-c++ cmake make python3 boost-devel` | `sqlite-devel` | `capnproto capnproto-devel` | `zeromq-devel pkgconf` | `systemtap-sdt-devel` | `qt6-qtbase-devel qt6-qttools-devel qt6-qtwayland qrencode-devel` | 51 | -| Alpine (`apk`) | `build-base cmake linux-headers python3 boost-dev` | `sqlite-dev` | `capnproto capnproto-dev` | `zeromq-dev` | Not supported | `qt6-qtbase-dev qt6-qttools-dev libqrencode-dev` | 52 | -| Arch (`pacman`) | `gcc make cmake python boost` | `sqlite` | `capnproto` | `zeromq` | `systemtap` | `qt6-base qt6-tools qt6-wayland qrencode` |
Now users building from source, who copy-paste the required deps from here, will get spurious warnings about a missing Python. That seems a bit worse than no warnings in the average case?
Fair point.