doc: Improve dependencies documentation #31634

NicolaLS · 2025-01-10T12:39:15Z

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 · 2025-01-10T12:39:18Z

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
ACK	hodlinator, rkrux, achow101
Stale ACK	maflcko

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)

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.

doc/dependencies.md

maflcko · 2025-01-10T13:10:00Z

lgtm ACK 08c74105fd65b6356fcdb9d01972fd19f160b588, otherwise

hodlinator · 2025-01-10T13:12:48Z

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 · 2025-01-10T13:24:22Z

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 · 2025-01-10T13:54:30Z

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 · 2025-01-11T15:24:06Z

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)

hodlinator

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.

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

doc/dependencies.md

hodlinator

re-ACK 1fcc1ce0edcc8ae522ab9103ad79adf58fc0fe0e

Thanks for applying my suggestions.

fanquake · 2025-01-13T10:12:40Z

doc/dependencies.md

Python isn't required.

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

doc/dependencies.md

NicolaLS · 2025-01-14T09:12:02Z

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 · 2025-01-14T09:24:53Z

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 · 2025-01-14T09:49:50Z

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 · 2025-01-14T09:55:20Z

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

rkrux · 2025-01-14T14:55:09Z

doc/dependencies.md

Doesn't CMake dependency have a "version used"?

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

In thread #31634 (comment):

The plot thickens - reference to lower version than minimum under depends/:

bitcoin/depends/hosts/linux.mk

Lines 44 to 45 in df8bf65

# Refer to doc/dependencies.md for the minimum required kernel.

linux_cmake_system_version=3.17.0

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

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

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)

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

re-ACK 05a008c1d07a38f589536661822d5450aa4e9438

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

hodlinator · 2025-01-16T15:28:05Z

doc/dependencies.md

In thread #31634 (comment):

The plot thickens - reference to lower version than minimum under depends/:

bitcoin/depends/hosts/linux.mk

Lines 44 to 45 in df8bf65

# Refer to doc/dependencies.md for the minimum required kernel.

linux_cmake_system_version=3.17.0

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

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

hodlinator

re-ACK a759ea3

git range-diff master 05a008c a759ea3

Resolved conflict with recently merged Qt depends upgrade, #30774.

DrahtBot added the Docs label Jan 10, 2025

maflcko reviewed Jan 10, 2025

View reviewed changes

doc/dependencies.md Outdated Show resolved Hide resolved

NicolaLS force-pushed the doc-required-dependencies branch 3 times, most recently from c428bdb to ec60b6a Compare January 11, 2025 15:20

NicolaLS changed the title ~~doc: merge required dependency tables~~ doc: improve dependencies documentation Jan 11, 2025

NicolaLS force-pushed the doc-required-dependencies branch from ec60b6a to d0ca5c5 Compare January 11, 2025 15:35

hodlinator approved these changes Jan 11, 2025

View reviewed changes

doc/dependencies.md Outdated Show resolved Hide resolved

DrahtBot requested a review from maflcko January 11, 2025 21:00

NicolaLS force-pushed the doc-required-dependencies branch from d0ca5c5 to 1fcc1ce Compare January 12, 2025 13:33

NicolaLS changed the title ~~doc: improve dependencies documentation~~ doc: Improve dependencies documentation Jan 12, 2025

hodlinator approved these changes Jan 13, 2025

View reviewed changes

fanquake reviewed Jan 13, 2025

View reviewed changes

rkrux reviewed Jan 13, 2025

View reviewed changes

doc/dependencies.md Outdated Show resolved Hide resolved

NicolaLS force-pushed the doc-required-dependencies branch from 1fcc1ce to 05a008c Compare January 14, 2025 09:09

rkrux reviewed Jan 14, 2025

View reviewed changes

hodlinator approved these changes Jan 16, 2025

View reviewed changes

This was referenced Jan 16, 2025

build: Switch to Qt 6 #30997

Merged

depends: Qt 5.15.16 #30774

Merged

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

This was referenced Jan 17, 2025

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

Multiprocess bitcoin #10102

Draft

DrahtBot added the Needs rebase label Jan 20, 2025

NicolaLS force-pushed the doc-required-dependencies branch from 05a008c to a759ea3 Compare January 20, 2025 15:15

DrahtBot removed the Needs rebase label Jan 20, 2025

DrahtBot mentioned this pull request Jan 21, 2025

Remove the legacy wallet and BDB dependency #28710

Merged

hodlinator approved these changes Jan 27, 2025

View reviewed changes

	# Refer to doc/dependencies.md for the minimum required kernel.
	linux_cmake_system_version=3.17.0

doc: Improve dependencies documentation #31634

doc: Improve dependencies documentation #31634

Uh oh!

Conversation

NicolaLS commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

maflcko commented Jan 10, 2025

Uh oh!

hodlinator commented Jan 10, 2025

Uh oh!

fanquake commented Jan 10, 2025

Uh oh!

NicolaLS commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolaLS commented Jan 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Commit message

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

NicolaLS Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NicolaLS commented Jan 14, 2025

Uh oh!

maflcko commented Jan 14, 2025

Uh oh!

NicolaLS commented Jan 14, 2025

Uh oh!

maflcko commented Jan 14, 2025

Uh oh!

rkrux Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodlinator Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

NicolaLS Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

hodlinator Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

NicolaLS commented Jan 10, 2025 •

edited

Loading

DrahtBot commented Jan 10, 2025 •

edited

Loading

NicolaLS commented Jan 10, 2025 •

edited

Loading

NicolaLS commented Jan 11, 2025 •

edited

Loading

hebasto Jan 14, 2025 •

edited

Loading

hodlinator Jan 16, 2025 •

edited

Loading

hodlinator Jan 16, 2025 •

edited

Loading