build: make BITCOIN_QT_PATH_PROGS always check $PATH before failure

cbergqvist commented at 10:33 pm on March 14, 2024: none

BITCOIN_QT_PATH_PROGS should conform to it’s documentation for parameter $3:

dnl Inputs: $3: Look for $2 here before $PATH

Prior to the change, when $3 was specified, the function would not check $PATH. Fixing this resolves the case where the lrelease, lupdate and lconvert binaries live in a different directory than the other QT Base binaries, hindering ./configure from detecting them. The issue was observed on Nix(OS) which has different binary folders for nixpkgs.qt5.qtbase and nixpkgs.qt5.qttools, causing ./configure to not allow building bitcoin-qt.

(This issue is GUI-related but involves the build system).

Testing

Install NixOS or the nix package manager (https://nixos.org/download/).
Clone the Bitcoin Core repo.
Copy shell.nix from https://gist.github.com/cbergqvist/7d9ce7663414214bb991007ab8138725 into the repo directory
Run nix-shell from the CLI inside the repo directory.
Run make clean && ./configure and observe change in output depending on whether this change is applied to the repo or not.

Expected output from `./configure` without the change

 0checking for Qt5Core >= 5.11.3... yes
 1checking for Qt5Gui >= 5.11.3... yes
 2checking for Qt5Widgets >= 5.11.3... yes
 3checking for Qt5Network >= 5.11.3... yes
 4checking for Qt5Test >= 5.11.3... yes
 5checking for Qt5DBus >= 5.11.3... yes
 6checking for static Qt... no
 7checking whether -fPIE can be used with this Qt config... no
 8checking for moc-qt5... no
 9checking for moc5... no
10checking for moc... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/moc
11checking for uic-qt5... no
12checking for uic5... no
13checking for uic... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/uic
14checking for rcc-qt5... no
15checking for rcc5... no
16checking for rcc... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/rcc
17checking for lrelease-qt5... no
18checking for lrelease5... no
19checking for lrelease... no
20configure: WARNING: LRELEASE not found; bitcoin-qt frontend will not be built
21checking whether to build Bitcoin Core GUI... no

Expected output from `./configure` with the change

 0checking for Qt5Core >= 5.11.3... yes
 1checking for Qt5Gui >= 5.11.3... yes
 2checking for Qt5Widgets >= 5.11.3... yes
 3checking for Qt5Network >= 5.11.3... yes
 4checking for Qt5Test >= 5.11.3... yes
 5checking for Qt5DBus >= 5.11.3... yes
 6checking for static Qt... no
 7checking whether -fPIE can be used with this Qt config... no
 8checking for moc-qt5... no
 9checking for moc5... no
10checking for moc... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/moc
11checking for uic-qt5... no
12checking for uic5... no
13checking for uic... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/uic
14checking for rcc-qt5... no
15checking for rcc5... no
16checking for rcc... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/rcc
17checking for lrelease-qt5... no
18checking for lrelease5... no
19checking for lrelease... no
20checking for lrelease-qt5... no
21checking for lrelease5... no
22checking for lrelease... /nix/store/4dc19x6k1ma72s13i4gs75amgsxz5jmy-qttools-5.15.12-dev/bin/lrelease
23checking for lupdate-qt5... no
24checking for lupdate5... no
25checking for lupdate... no
26checking for lupdate-qt5... no
27checking for lupdate5... no
28checking for lupdate... /nix/store/4dc19x6k1ma72s13i4gs75amgsxz5jmy-qttools-5.15.12-dev/bin/lupdate
29checking for lconvert-qt5... no
30checking for lconvert5... no
31checking for lconvert... no
32checking for lconvert-qt5... no
33checking for lconvert5... no
34checking for lconvert... /nix/store/4dc19x6k1ma72s13i4gs75amgsxz5jmy-qttools-5.15.12-dev/bin/lconvert
35checking whether to build Bitcoin Core GUI... yes (Qt5)

build: make BITCOIN_QT_PATH_PROGS always check $PATH before failure

`BITCOIN_QT_PATH_PROGS` should conform to it's documentation for parameter `$3`:
> dnl Inputs: $3: Look for $2 here before $PATH

Prior to the change, when `$3` was specified, the function would *not* check `$PATH`. Fixing this resolves the case where the `lrelease`, `lupdate` and `lconvert` binaries live in a different directory than the other QT Base binaries, hindering `./configure` from detecting them. The issue was observed on Nix(OS) which has different binary folders for `nixpkgs.qt5.qtbase` and `nixpkgs.qt5.qttools`, causing `./configure` to not allow building `bitcoin-qt`.

7da77ee17d

DrahtBot commented at 10:33 pm on March 14, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

DrahtBot added the label Build system on Mar 14, 2024

hebasto commented at 10:03 am on March 15, 2024: member

cc @0xB10C

hebasto commented at 10:27 am on March 15, 2024: member

@cbergqvist

Thank you for working on this!

The suggested approach seems fragile to me. For example, if our depends build system fails to build all qt tools, the main build system could silently pick them from the system packages:

 0checking for lrelease-qt5... no
 1checking for lrelease5... no
 2checking for lrelease... no
 3checking for lrelease-qt5... no
 4checking for lrelease5... no
 5checking for lrelease... /usr/bin/lrelease
 6checking for lupdate-qt5... no
 7checking for lupdate5... no
 8checking for lupdate... /home/hebasto/git/bitcoin/depends/x86_64-pc-linux-gnu/native/bin/lupdate
 9checking for lconvert-qt5... no
10checking for lconvert5... no
11checking for lconvert... /home/hebasto/git/bitcoin/depends/x86_64-pc-linux-gnu/native/bin/lconvert

We are close to migrating the build system to CMake. You are encouraged to review and test the staging branch on NixOS :)

cbergqvist commented at 3:08 pm on March 15, 2024: none

Thanks for the timely feedback @hebasto! I’ll move this to a draft and see if I can figure out a cleaner approach.

Happy to test out the CMake branch when I get the time. :)

cbergqvist marked this as a draft on Mar 15, 2024

cbergqvist commented at 10:01 pm on March 15, 2024: none

My latest understanding is that this issue stems from an ongoing struggle waged by the nixpkgs maintainers. Whereas other distros install qtbase and qttools binaries in the same dir, Nix has them in separate directories and cannot have qttools modify qtbase. Possible approaches could be to have qttools be an optional part of qtbase, or make some kind of aggregation derivation with symlinks back to the the binaries of the two derivations.

Exhibits:

Alternative solution

Was able to resolve the situation through:

Undoing the change in this PR.
Modifying my shell.nix to specify both dirs to ./configure through --with-qt-bindir=${pkgs.qt5.qtbase.dev}/bin:${pkgs.qt5.qttools.dev}/bin (the linked gist in the summary has been updated).

Possible remaining action

One could still improve the BITCOIN_QT_PATH_PROGS documentation stating

https://github.com/bitcoin/bitcoin/blob/015ac13dcc964a31ef06dfdb565f88f901607f0e/build-aux/m4/bitcoin_qt.m4#L33

with

0dnl Inputs: $3: If specified, look for $2 here instead of $PATH

But especially as it is being phased out in favor of CMake, I’m fine with just closing this PR.

Will let it linger a few days in case anyone cares to comment.

cbergqvist commented at 8:12 pm on March 19, 2024: none

Closing as cleaner solution not requiring repo modifications was found.

(Comment is still wrong but just a nit).

cbergqvist closed this on Mar 19, 2024

0xB10C commented at 10:01 pm on March 19, 2024: contributor

Tangentially, @cbergqvist do you think moving the shell.nix gist into a repo for better collaboration would make sense?

cbergqvist deleted the branch on Mar 19, 2024

cbergqvist commented at 10:09 pm on March 19, 2024: none

Tangentially, @cbergqvist do you think moving the shell.nix gist into a repo for better collaboration would make sense?

This Bitcoin Core repo might be a good place for a shell.nix. Someday. :)

In seriousness, I support you moving it from a gist to a repo under your account which individuals like me might submit PRs to.

build: make BITCOIN_QT_PATH_PROGS always check $PATH before failure #29653