doc: document capnproto and libmultiprocess deps in 29.x #33623
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33623. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
|
Concept ACK |
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK ad73a8e2540005f0d81310e10b61c4a61897b217. Thanks for the documentation! This change looks good as-is but i think it could also make sense to reference a version number instead of a commit hash so I left a suggestion below.
ad73a8e to
bf236d7
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK bf236d7a8af0826efc39a04113b79f96fe05f0c7. Looks good but left some more suggestions below. I think I might also adjust PR description not to say documented version is the latest version that will ever work (see below). I think it would be more accurate just to say that later versions are known not to work.
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use] |
|
Concept ACK - @willcl-ark did you want to address any of the suggestions here? |
bf236d7 to
1d22518
Compare
Sorry, PR updated now, taking @ryanofsky's suggestions. |
|
@ryanofsky can you circle back here? |
|
Code review 1d2251850dda2a2ae773029aeb32128b72c53f0b. Thanks for the update! This is mostly ok, but sorry my earlier suggestions were a little off, and I think this would be better to improve before merging. Would suggest: --- a/doc/dependencies.md
+++ b/doc/dependencies.md
@@ -37,6 +37,6 @@ Bitcoin Core requires one of the following compilers.
| Python (scripts, tests) | [link](https://www.python.org) | N/A | [3.10](https://github.com/bitcoin/bitcoin/pull/30527) | No |
| [systemtap](../depends/packages/systemtap.mk) ([tracing](tracing.md)) | [link](https://sourceware.org/systemtap/) | [4.8](https://github.com/bitcoin/bitcoin/pull/26945)| N/A | No |
| [capnproto](../depends/packages/capnp.mk) ([multiprocess](multiprocess.md)) | [link](https://capnproto.org/) | [1.2.0](https://github.com/bitcoin/bitcoin/pull/32760)| [0.7.0](https://github.com/bitcoin-core/libmultiprocess/pull/88) | No |
-| [libmultiprocess*](../depends/packages/libmultiprocess.mk) ([multiprocess](multiprocess.md)) | [link](https://github.com/bitcoin-core/libmultiprocess) | [5.0](https://github.com/bitcoin/bitcoin/pull/31740)| N/A | No |
+| [libmultiprocess](../depends/packages/libmultiprocess.mk) ([multiprocess](multiprocess.md)) | [link](https://github.com/bitcoin-core/libmultiprocess) | [5.0](https://github.com/bitcoin/bitcoin/pull/31945)| [v5.0-pre1](https://github.com/bitcoin/bitcoin/pull/31740)* | No |
-\* Libmultiprocess 6.0 and later versions are no longer compatible due to bitcoin-core/libmultiprocess#160.
+\* Libmultiprocess 5.x versions should be compatible, but 6.0 and later are not due to bitcoin-core/libmultiprocess#160.To fix current version not linking to the right PR, minimum version not being specified, clarify the maximum version comment, and move asterisk where it's more directly relevant |
These dependencies are both undocumented, and libmultiprocess has a relatively special requirement in that v6.0 and later are known to not work with v29.x of Bitcoin Core due to bitcoin-core/libmultiprocess#160
1d22518 to
2cf352f
Compare
|
Thanks @ryanofsky, I took your clarifying changes in 2cf352f |
Closes: #33576
These dependencies are both undocumented, and libmultiprocess has a relatively special requirement in that v6.0 and later are known to not work with v29.x of Bitcoin Core due to bitcoin-core/libmultiprocess#160