doc: Improve dependencies.md#31895
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/31895. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
hodlinator
left a comment
There was a problem hiding this comment.
ACK f3d2e9acbf3d9407101290ca86f014e82344cfce
Concept
Separates build and runtime dependencies into separate tables which makes more sense.
Nits
-
Slight preference of using
### Buildover**Build:**, but it's subjective. -
Maybe 32bdc7dc47bcd67458bc8f687fcf8659ec3f0b1e could also include a link to https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/cmake.scm?id=53396a22afc04536ddf75d8f82ad2eafa5082725#n169 in the commit message, since it's a bit opaque otherwise?
Thanks. Fixed both nits, I also think headlines are better. |
There was a problem hiding this comment.
re-ACK 8da43955376bde65fa517af462fd2a1514764d2b
PR addresses feedback from at least 3 long-time contributors (see comment links in PR description).
Changes: Addressed nits from first ACK.
Nice bonus that one can now link directly to a specific sub-heading.
https://github.com/bitcoin/bitcoin/blob/8da43955376bde65fa517af462fd2a1514764d2b/doc/dependencies.md#build-1
jonatack
left a comment
There was a problem hiding this comment.
Swung by here a couple times previously and this pull is looking better now.
ACK fefe8fc3c38701bdc3a3b1c6e4d0c8885f94e75f modulo question below
|
ACK 4b6171982a20d736b2c627ab9b7ea788b06af457 |
hodlinator
left a comment
There was a problem hiding this comment.
re-ACK 4b6171982a20d736b2c627ab9b7ea788b06af457
Changes since previous ACK: Addressed feedback (#31895 (comment) #31895 (comment)).
d68a5bd to
810916f
Compare
|
Squashed and reworded commits. |
Linux Kernel is neither an optional nor a required dependency for Bitcoin Core.
The `dependencies.md` should mention that it is possible to self-compile the dependencies and reference `depends/README.md` for instructions. Also mention full path to `/doc/build-*.md` for clarity.
- Remove the "Version used" column from the dependencies tables as the depends package definition which defines the version used is already linked. In case a developer is interested in which PR introduced this file/version they can use `git blame` on the package definition as usual. This removes doc. maintenance overhead and eliminates the risk of stale information about the "Version used", see comment: bitcoin#31895 (comment) - Separate dependency tables into build-time and run-time tables for easier distinction of the two and to avoid repeating the same information ("No"/"Yes") for better readability. - Order dependencies alphabetically
|
(fixed typo in commit message) |
|
lgtm ACK e62423d 🛄 Show signatureSignature: |
| | Dependency | Releases | Minimum required | | ||
| | --- | --- | --- | | ||
| | [Boost](../depends/packages/boost.mk) | [link](https://www.boost.org/users/download/) | [1.73.0](https://github.com/bitcoin/bitcoin/pull/29066) | | ||
| | CMake | [link](https://cmake.org/) | [3.22](https://github.com/bitcoin/bitcoin/pull/30454) | |
There was a problem hiding this comment.
nit:
| | CMake | [link](https://cmake.org/) | [3.22](https://github.com/bitcoin/bitcoin/pull/30454) | | |
| | CMake | [link](https://cmake.org/download/) | [3.22](https://github.com/bitcoin/bitcoin/pull/30454) | |
There was a problem hiding this comment.
Anything here seems fine, we don't link to https://sqlite.org/download.html or https://freetype.org/download.html.
There was a problem hiding this comment.
Anything here seems fine, we don't link to https://sqlite.org/download.html or https://freetype.org/download.html.
But we do link to https://www.boost.org/users/download/ and https://github.com/libevent/libevent/releases.
I’ve ACKed the current branch and am content with it as is.
| | [Fontconfig](../depends/packages/fontconfig.mk) (gui) | [link](https://www.freedesktop.org/wiki/Software/fontconfig/) | 2.6 | | ||
| | [FreeType](../depends/packages/freetype.mk) (gui) | [link](https://freetype.org) | 2.3.0 | |
There was a problem hiding this comment.
nit: From #30997 (comment):
Starting with Qt 6.5.0, the
libxcb-cursor0package is required to be installed at runtime.
There was a problem hiding this comment.
I don't any x11 packages are listed here, so it seems unrelated?
There was a problem hiding this comment.
It was a nit. I am OK with the current branch.
Small improvements to the
dependencies.mddocumentation as a follow-up for #31634.Linux Kernel does not need to be in the dependencies as it is not required for cross-compiling from other systems, and users building on Linux should not expect they can build using any EOL kernel, see: #31634 (comment)
Runtime dependencies can be in a separate table to improve readability. See: #31634 (comment)
Version used is redundant as the depends package definition is already linked in the table and can thus be removed, see: #31895 (comment)