depends, doc: List packages required to build `qt` package separately #31192

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:241031-dep-doc changing 1 files +8 −2

hebasto commented at 1:20 pm on October 31, 2024: member

Suggested in #30997 (review):

There’s probably enough GUI-only stuff here, i.e bison, ninja-build, python3, xz-utils, that this could be moved to it’s own #### Gui section.
hebasto added the label Docs on Oct 31, 2024
hebasto added the label Build system on Oct 31, 2024
DrahtBot commented at 1:21 pm on October 31, 2024: 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/31192.

Reviews

See the guideline for information on the review process.

Type Reviewers

ACK fanquake

Concept ACK Sjors

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:
- #30997 (build: Switch to Qt 6 by hebasto)
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.
in depends/README.md:50 in e4a458b913 outdated
46+ 47+#### GUI 48+ 49+Skip the following packages if you don't intend to use the GUI and will build with [`NO_QT=1`](#dependency-options): 50+ 51+ apt install bison pkg-config python3 xz-utils
fanquake commented at 1:23 pm on October 31, 2024:

pkg-config can only be moved here after #31181?

hebasto commented at 1:25 pm on October 31, 2024:

For make -C depends MULTIPROCESS=1 NO_QT=1 the pkg-config package seems not required though.

fanquake commented at 2:50 pm on October 31, 2024:

It would be more of a convinience. I guess this doesn’t make too much difference as 31181 is likely to be merged soon in any case.

Sjors commented at 6:10 pm on October 31, 2024:

I suggest either leaving out apt install or also naming brew install and friends, to make it clear this applies to all platforms.

hebasto commented at 6:55 pm on October 31, 2024:

I wouldn’t expect that all packages have the same names across Ubuntu/Debian and Homebrew repositories.

~~I agree that a macOS-specific section can be added here, but this is not a goal of this PR.~~

hebasto commented at 10:37 am on November 1, 2024:

I’ve double-checked prerequisites required for building depends on macOS, and there are no additional requirements specific to depends beyond those listed in https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md#3-install-required-dependencies.
DrahtBot added the label CI failed on Oct 31, 2024
DrahtBot commented at 2:54 pm on October 31, 2024: contributor
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32336801754

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
- Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
- A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
- An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Sjors commented at 6:09 pm on October 31, 2024: member

Concept ACK.
DrahtBot removed the label CI failed on Oct 31, 2024
hebasto force-pushed on Nov 1, 2024
fanquake commented at 9:35 am on November 4, 2024: member

Additionally, the cmake has been removed from the required packages, as it is no longer specific to depends.

If cmake is needed to build depends it should remain listed. The same as make, or a compiler.
hebasto force-pushed on Nov 4, 2024
hebasto commented at 11:08 am on November 4, 2024: member

Additionally, the cmake has been removed from the required packages, as it is no longer specific to depends.

If cmake is needed to build depends it should remain listed. The same as make, or a compiler.

Thanks! Reworked per feedback.
depends, doc: List packages required to build `qt` package separately 4747f03095
in depends/README.md:50 in 4772e9f655 outdated
46+ 47+#### GUI 48+ 49+Skip the following packages if you don't intend to use the GUI and will build with [`NO_QT=1`](#dependency-options): 50+ 51+ apt install bison g++ pkg-config python3 xz-utils
fanquake commented at 11:19 am on November 4, 2024:

If you’re adding g++ here, you can remove it from the macOS section below.

hebasto commented at 11:23 am on November 4, 2024:

Thanks! Removed.

laanwj commented at 11:43 am on November 4, 2024:

Its… imo a bit strange to mention skipping g++ as it’s so clearly a dependency for building bitcoin core itself. While correct in a strict sense, we know having it not installed is going to fail later. Also the multiprocess lib (and deps) require a C++ compiler.

hebasto commented at 11:46 am on November 4, 2024:

The user can use a different compiler to build depends, but Qt 5 still insists on GCC for building the native qmake.

laanwj commented at 11:50 am on November 4, 2024:

Okay, sure, then it makes sense.
hebasto force-pushed on Nov 4, 2024
fanquake approved
fanquake commented at 4:12 pm on November 5, 2024: member

ACK 4747f030956d6876ec7cef4e2500a8579bc82146
DrahtBot requested review from Sjors on Nov 5, 2024
fanquake merged this on Nov 5, 2024
fanquake closed this on Nov 5, 2024
hebasto deleted the branch on Nov 5, 2024

Type	Reviewers
ACK	fanquake
Concept ACK	Sjors

Contributors
hebasto DrahtBot fanquake Sjors laanwj

Review Requested
Sjors

Labels
Docs Build system

depends, doc: List packages required to build qt package separately #31192

Code Coverage & Benchmarks

Reviews

Conflicts

depends, doc: List packages required to build `qt` package separately #31192