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

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

1. Install NixOS or the nix package manager (https://nixos.org/download/).
2. Clone the Bitcoin Core repo.
3. Copy shell.nix from https://gist.github.com/cbergqvist/7d9ce7663414214bb991007ab8138725 into the repo directory
4. Run nix-shell from the CLI inside the repo directory.
5. 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

checking for Qt5Core >= 5.11.3... yes
checking for Qt5Gui >= 5.11.3... yes
checking for Qt5Widgets >= 5.11.3... yes
checking for Qt5Network >= 5.11.3... yes
checking for Qt5Test >= 5.11.3... yes
checking for Qt5DBus >= 5.11.3... yes
checking for static Qt... no
checking whether -fPIE can be used with this Qt config... no
checking for moc-qt5... no
checking for moc5... no
checking for moc... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/moc
checking for uic-qt5... no
checking for uic5... no
checking for uic... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/uic
checking for rcc-qt5... no
checking for rcc5... no
checking for rcc... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/rcc
checking for lrelease-qt5... no
checking for lrelease5... no
checking for lrelease... no
configure: WARNING: LRELEASE not found; bitcoin-qt frontend will not be built
checking whether to build Bitcoin Core GUI... no

Expected output from ./configure with the change

checking for Qt5Core >= 5.11.3... yes
checking for Qt5Gui >= 5.11.3... yes
checking for Qt5Widgets >= 5.11.3... yes
checking for Qt5Network >= 5.11.3... yes
checking for Qt5Test >= 5.11.3... yes
checking for Qt5DBus >= 5.11.3... yes
checking for static Qt... no
checking whether -fPIE can be used with this Qt config... no
checking for moc-qt5... no
checking for moc5... no
checking for moc... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/moc
checking for uic-qt5... no
checking for uic5... no
checking for uic... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/uic
checking for rcc-qt5... no
checking for rcc5... no
checking for rcc... /nix/store/78yxzzk2fb6am5v4dihalw9hrrj0rg8l-qtbase-5.15.12-dev/bin/rcc
checking for lrelease-qt5... no
checking for lrelease5... no
checking for lrelease... no
checking for lrelease-qt5... no
checking for lrelease5... no
checking for lrelease... /nix/store/4dc19x6k1ma72s13i4gs75amgsxz5jmy-qttools-5.15.12-dev/bin/lrelease
checking for lupdate-qt5... no
checking for lupdate5... no
checking for lupdate... no
checking for lupdate-qt5... no
checking for lupdate5... no
checking for lupdate... /nix/store/4dc19x6k1ma72s13i4gs75amgsxz5jmy-qttools-5.15.12-dev/bin/lupdate
checking for lconvert-qt5... no
checking for lconvert5... no
checking for lconvert... no
checking for lconvert-qt5... no
checking for lconvert5... no
checking for lconvert... /nix/store/4dc19x6k1ma72s13i4gs75amgsxz5jmy-qttools-5.15.12-dev/bin/lconvert
checking whether to build Bitcoin Core GUI... yes (Qt5)

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

cc @0xB10C

@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:

checking for lrelease-qt5... no
checking for lrelease5... no
checking for lrelease... no
checking for lrelease-qt5... no
checking for lrelease5... no
checking for lrelease... /usr/bin/lrelease
checking for lupdate-qt5... no
checking for lupdate5... no
checking for lupdate... /home/hebasto/git/bitcoin/depends/x86_64-pc-linux-gnu/native/bin/lupdate
checking for lconvert-qt5... no
checking for lconvert5... no
checking 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 :)

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

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:

1. https://github.com/NixOS/nixpkgs/pull/280943/files#diff-1842f86517293a0958f8b03fd0c28149b86c88bcd32276f9565b334c8f523610R39-R41
2. https://github.com/NixOS/nixpkgs/blob/9af9c1c87ed3e3ed271934cb896e0cdd33dae212/pkgs/tools/graphics/gnuplot/default.nix#L44-L45
3. https://github.com/NixOS/nixpkgs/blob/9af9c1c87ed3e3ed271934cb896e0cdd33dae212/pkgs/tools/graphics/nifskope/qttools-bins.patch#L18-L19

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

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

Closing as cleaner solution not requiring repo modifications was found.

(Comment is still wrong but just a nit).

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

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.