-
Notifications
You must be signed in to change notification settings - Fork 36.3k
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.
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
build: make BITCOIN_QT_PATH_PROGS always check $PATH before failure #29653
Conversation
`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`.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. |
cc @0xB10C |
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:
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 Exhibits:
Alternative solutionWas able to resolve the situation through:
Possible remaining actionOne could still improve the bitcoin/build-aux/m4/bitcoin_qt.m4 Line 33 in 015ac13
with
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? |
This Bitcoin Core repo might be a good place for a In seriousness, I support you moving it from a gist to a repo under your account which individuals like me might submit PRs to. |
BITCOIN_QT_PATH_PROGS
should conform to it's documentation for parameter$3
:Prior to the change, when
$3
was specified, the function would not check$PATH
. Fixing this resolves the case where thelrelease
,lupdate
andlconvert
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 fornixpkgs.qt5.qtbase
andnixpkgs.qt5.qttools
, causing./configure
to not allow buildingbitcoin-qt
.(This issue is GUI-related but involves the build system).
Testing
nix
package manager (https://nixos.org/download/).shell.nix
from https://gist.github.com/cbergqvist/7d9ce7663414214bb991007ab8138725 into the repo directorynix-shell
from the CLI inside the repo directory.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 changeExpected output from
./configure
with the change