Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

cbergqvist
Copy link
Contributor

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)

@cbergqvist
build: make BITCOIN_QT_PATH_PROGS always check $PATH before failure
7da77ee 
`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`.
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 14, 2024 •
edited

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.

@hebasto
Copy link
Member

hebasto commented Mar 15, 2024

cc @0xB10C

@hebasto
Copy link
Member

hebasto commented Mar 15, 2024

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

@cbergqvist
Copy link
Contributor Author

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 cbergqvist marked this pull request as draft March 15, 2024 15:08
@cbergqvist
Copy link
Contributor Author

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

bitcoin/build-aux/m4/bitcoin_qt.m4

Line 33 in 015ac13

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

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.

@cbergqvist
Copy link
Contributor Author

Closing as cleaner solution not requiring repo modifications was found.

(Comment is still wrong but just a nit).

@cbergqvist cbergqvist closed this Mar 19, 2024
@0xB10C
Copy link
Contributor

0xB10C commented Mar 19, 2024

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

@cbergqvist cbergqvist deleted the bitcoin_qt_path_fallback branch March 19, 2024 22:03
@cbergqvist
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cbergqvist @DrahtBot @hebasto @0xB10C