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).
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.
<!--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 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:
--perf profiling from functional test framework by theStack)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.
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?
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?
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>
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.