doc: Improve dependencies documentation

NicolaLS commented at 12:39 pm on January 10, 2025: none

Initially there was a distinction between the compiler dependencies and other required dependencies (refs #23565) but the distinction was removed (refs #24585) which is why having two distinct tables could lead to confusion now.

DrahtBot commented at 12:39 pm on January 10, 2025: contributor

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/31634.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	maflcko, hodlinator

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28710 (Remove the legacy wallet and BDB dependency by achow101)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

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.

DrahtBot added the label Docs on Jan 10, 2025

in doc/dependencies.md:15 in 08c74105fd outdated

18 | --- | --- | --- | --- | --- |
19+| Clang | [link](https://clang.llvm.org) | N/A | [16.0](https://github.com/bitcoin/bitcoin/pull/30263) | No |
20+| CMake | [link](https://cmake.org/) | N/A | [3.22](https://github.com/bitcoin/bitcoin/pull/30454) | No |
21+| GCC | [link](https://gcc.gnu.org) | N/A | [11.1](https://github.com/bitcoin/bitcoin/pull/29091) | No |
22+| Python (scripts, tests) | [link](https://www.python.org) | N/A | [3.10](https://github.com/bitcoin/bitcoin/pull/30527) | No |
23+| [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 |

maflcko commented at 1:09 pm on January 10, 2025:

this is optional

hodlinator commented at 1:11 pm on January 10, 2025:

(systemtap) :+1:

NicolaLS commented at 1:15 pm on January 10, 2025:

should I add another sub-header (### Tracing) in the ## Optional section and put it there ?

maflcko commented at 1:25 pm on January 10, 2025:

Yeah, either a new section, or merge the tables below as well into one:

Dependency	Releases	Version used	Minimum required	Runtime
Fontconfig (GUI)	link	2.12.6	2.6	Yes
FreeType (GUI)	link	2.11.0	2.3.0	Yes
qrencode (GUI QR codes)	link	4.1.1		No
Qt (GUI)	link	5.15.14	5.11.3	No
ZeroMQ (Notifications)	link	4.3.4	4.0.0	No
Berkeley DB (legacy wallet)	link	4.8.30	4.8.x	No
SQLite (wallet)	link	3.38.5	3.7.17	No

maflcko commented at 1:10 pm on January 10, 2025: member

lgtm ACK 08c74105fd65b6356fcdb9d01972fd19f160b588, otherwise

hodlinator commented at 1:12 pm on January 10, 2025: contributor

Would be good to move discussion/question-type commentary out of the PR summary into its own comment as it becomes part of the commit message, regarding the “I was a bit confused … please let me know …”. Alternatively re-word it.

fanquake commented at 1:24 pm on January 10, 2025: member

Not sure. Re-arranging this to put two different compilers under “required” doesn’t seem less confusing. If anything, I think the tables should be left as-is, and a “developer tools” header added, if there is any confusion.

NicolaLS commented at 1:54 pm on January 10, 2025: none

Not sure. Re-arranging this to put two different compilers under “required” doesn’t seem less confusing. If anything, I think the tables should be left as-is, and a “developer tools” header added, if there is any confusion.

With a “development tools” header it would still be unclear which development tools are required (cmake, ~~python~~), optional (systemtap) or mutual substitutes (gcc or clang) and since required ones would mixed with the two different compilers in the same table it would have the same issue that you pointed out imo.

Would it make sense to add a “Compiler” header with a paragraph that one of them is required and a table containing both. And then merge the other tables as @maflcko suggested ?

NicolaLS force-pushed on Jan 11, 2025

NicolaLS commented at 3:24 pm on January 11, 2025: none

I agree that the previous version or this PR did not make sense for clearing up the confusion. I force pushed a new approach and amended the commit message / PR description.

(Preview)

NicolaLS renamed this:
~~doc: merge required dependency tables~~
doc: improve dependencies documentation
on Jan 11, 2025

NicolaLS force-pushed on Jan 11, 2025

in doc/dependencies.md:24 in d0ca5c5363 outdated

22+| CMake | [link](https://cmake.org/) | N/A | [3.22](https://github.com/bitcoin/bitcoin/pull/30454) | No |
23+| Python (scripts, tests) | [link](https://www.python.org) | N/A | [3.10](https://github.com/bitcoin/bitcoin/pull/30527) | No |
24 | [Boost](../depends/packages/boost.mk) | [link](https://www.boost.org/users/download/) | [1.81.0](https://github.com/bitcoin/bitcoin/pull/26557) | [1.73.0](https://github.com/bitcoin/bitcoin/pull/29066) | No |
25 | [libevent](../depends/packages/libevent.mk) | [link](https://github.com/libevent/libevent/releases) | [2.1.12-stable](https://github.com/bitcoin/bitcoin/pull/21991) | [2.1.8](https://github.com/bitcoin/bitcoin/pull/24681) | No |
26 | glibc | [link](https://www.gnu.org/software/libc/) | N/A | [2.31](https://github.com/bitcoin/bitcoin/pull/29987) | Yes |
27 | Linux Kernel | [link](https://www.kernel.org/) | N/A | [3.17.0](https://github.com/bitcoin/bitcoin/pull/27699) | Yes |

hodlinator commented at 8:53 pm on January 11, 2025:

nit: Not yet touched by this PR, but could optionally add some context.

0| Linux Kernel (if building that platform) | [link](https://www.kernel.org/) | N/A | [3.17.0](https://github.com/bitcoin/bitcoin/pull/27699) | Yes |

hodlinator approved

hodlinator commented at 9:00 pm on January 11, 2025: contributor

ACK d0ca5c53635916868e7d5da913faf3d9a8d706b7

Good to clarify that only one of the compilers is required, and re-introduce that section!

Thanks for updating the PR summary.

Commit message

Suggest removing repetition of initial sentence and fixing grammar of last line, + less important other changes.

 0-doc: improve dependencies documentation
 1+doc: Improve dependencies documentation
 2 
 3-This commit improves the dependencies documentation by:
 4-- moving "Clang" and "GCC" from the table to a new "Compiler" heading.
 5-- moving "Python" and "Cmake" into the required table.
 6-- merging the optional dependencies into one table. Removed sub-headers
 7+- Move "Clang" and "GCC" from the table to a new "Compiler" heading, indicating either is required.
 8+- Move "Python" and "CMake" into the required table.
 9+- Merge the optional dependencies into one table. Removed sub-headers
10   are put into parentheses behind the dependency name in the first
11   column.
12-- replacing the whitespace in the "Minimum required" column of
13+- Replace the whitespace in the "Minimum required" column of
14   "qrencode" with "N/A" for consistency.
15-- adding missing info in for the "systemtap" dependency.
16+- Add missing info for the "systemtap" dependency.

DrahtBot requested review from maflcko on Jan 11, 2025

NicolaLS force-pushed on Jan 12, 2025

NicolaLS renamed this:
~~doc: improve dependencies documentation~~
doc: Improve dependencies documentation
on Jan 12, 2025

hodlinator approved

hodlinator commented at 9:35 am on January 13, 2025: contributor

re-ACK 1fcc1ce0edcc8ae522ab9103ad79adf58fc0fe0e

Thanks for applying my suggestions.

in doc/dependencies.md:20 in 1fcc1ce0ed outdated

18 ## Required
19 
20 | Dependency | Releases | Version used | Minimum required | Runtime |
21 | --- | --- | --- | --- | --- |
22+| CMake | [link](https://cmake.org/) | N/A | [3.22](https://github.com/bitcoin/bitcoin/pull/30454) | No |
23+| Python (scripts, tests) | [link](https://www.python.org) | N/A | [3.10](https://github.com/bitcoin/bitcoin/pull/30527) | No |

fanquake commented at 10:12 am on January 13, 2025:

Python isn’t required.

NicolaLS commented at 10:56 am on January 13, 2025:

ah yes..thanks. should I move Linux Kernel to optional too then ?

in doc/dependencies.md:13 in 1fcc1ce0ed outdated

11 | --- | --- |
12 | [Clang](https://clang.llvm.org) | [16.0](https://github.com/bitcoin/bitcoin/pull/30263) |
13-| [CMake](https://cmake.org/) | [3.22](https://github.com/bitcoin/bitcoin/pull/30454) |
14 | [GCC](https://gcc.gnu.org) | [11.1](https://github.com/bitcoin/bitcoin/pull/29091) |
15-| [Python](https://www.python.org) (scripts, tests) | [3.10](https://github.com/bitcoin/bitcoin/pull/30527) |
16-| [systemtap](https://sourceware.org/systemtap/) ([tracing](tracing.md))| N/A |

rkrux commented at 5:34 pm on January 13, 2025:

This doesn’t show up as a table when I preview the md file.

rkrux commented at 5:35 pm on January 13, 2025:

Needs a blank line after line 9.

NicolaLS commented at 9:07 am on January 14, 2025:

thanks, I was wondering if there should be a blank line. for me it shows the table correctly (firefox, gh mobile) so I thought when in doubt just keep it compact.

weird that it displays correctly for me but not for you though (also tested with gh-markdown-preview locally).

NicolaLS force-pushed on Jan 14, 2025

NicolaLS commented at 9:12 am on January 14, 2025: none

added a blank line between the paragraph and the table in ## Compiler and moved Python to the ## Optional table. thanks for the help everyone.

I think Linux Kernel should be moved to optional as well then but since it was in the required table before and I’m not sure if I should change more stuff after receiving acks I just left it as is now.

maflcko commented at 9:24 am on January 14, 2025: member

Maybe a separate table for runtime dependencies makes more sense. It is pretty clear that runtime dependencies don’t have a “Version used”. An alternative would be to just remove the “Runtime” column, as it can be derived from the “Version used” column.

NicolaLS commented at 9:49 am on January 14, 2025: none

Maybe a separate table for runtime dependencies makes more sense. It is pretty clear that runtime dependencies don’t have a “Version used”. An alternative would be to just remove the “Runtime” column, as it can be derived from the “Version used” column.

tbh. I don’t understand what “version used” really means in this context, looked at a few linked PR’s but they went over my head…

but anyways I think this is out of scope for this PR / could be addressed in another PR. I only wanted to clear up some confusion I had because of the two separate tables.

maflcko commented at 9:55 am on January 14, 2025: member

“Version used” means the version used in Bitcoin Core’s depends system, which compiles the version and statically links it into the release binaries.

in doc/dependencies.md:20 in 05a008c1d0 outdated

18 
19 ## Required
20 
21 | Dependency | Releases | Version used | Minimum required | Runtime |
22 | --- | --- | --- | --- | --- |
23+| CMake | [link](https://cmake.org/) | N/A | [3.22](https://github.com/bitcoin/bitcoin/pull/30454) | No |

rkrux commented at 2:55 pm on January 14, 2025:

Doesn’t CMake dependency have a “version used”?

hebasto commented at 3:43 pm on January 14, 2025:

CMake 3.24.2 is used for release binaries in the Guix environment.

hodlinator commented at 3:28 pm on January 16, 2025:

In thread #31634 (review):

The plot thickens - reference to lower version than minimum under depends/: https://github.com/bitcoin/bitcoin/blob/df8bf657450d5383870d40790ea9f4fdb03c360d/depends/hosts/linux.mk#L44-L45

I agree with the author in #31634 (comment) that it’s fine to limit the scope of the current PR.

hebasto commented at 5:03 pm on January 16, 2025:

If you imply that 3.17.0 is a CMake version, it is not. It’s CMAKE_SYSTEM_VERSION.

NicolaLS commented at 7:16 am on January 17, 2025:

thank you. so the PR to reference for the “version used” would be #27172 correct ?

manifest uses ((gnu packages cmake) #:select (cmake-minimal)) which is why we can find the version (relevant comment in cmake.scm)

hebasto commented at 8:28 am on January 17, 2025:

so the PR to reference for the “version used” would be #27172 correct ?

That refers to a Darwin-specific change. #29725 is a PR, which made cmake-minimal a global requirement.

However, in the future, updates like #30730 might bump the cmake-minimal version.

hodlinator approved

hodlinator commented at 3:34 pm on January 16, 2025: contributor

re-ACK 05a008c1d07a38f589536661822d5450aa4e9438

Thanks to @rkrux for spotting the formatting issue I missed.
SQLite now categorized as (wallet). :+1:

DrahtBot added the label Needs rebase on Jan 20, 2025

doc: Improve dependencies documentation

- Move "Clang" and "GCC" from the table to a new "Compiler" heading,
  indicating either is required.
- Move "CMake" into the required table.
- Move "Python" into the optional table.
- Merge the optional dependencies into one table. Removed sub-headers
  are put into parentheses behind the dependency name in the first
  column.
- Replace the whitespace in the "Minimum required" column of "qrencode"
  with "N/A" for consistency.
- Add missing info for the "systemtap" dependency.
- Add context for "Linux Kernel" dependency in parentheses behind the
  dependency name.

a759ea3e92

NicolaLS force-pushed on Jan 20, 2025

DrahtBot removed the label Needs rebase on Jan 20, 2025

doc: Improve dependencies documentation #31634

Code Coverage & Benchmarks

Reviews

Conflicts

Commit message