Skip to content

cmake: Fix toolchain.cmake for *BSD systems #292

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
Merged
merged 1 commit into from
Aug 2, 2024
Merged

cmake: Fix toolchain.cmake for *BSD systems #292

merged 1 commit into from
Aug 2, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Aug 1, 2024 •
edited
Loading

For *BSD systems, the qt and qrencode packages are not built, even without the NO_QT or NO_QR variables defined, because these package are not available for the host systems:

bitcoin/depends/packages/packages.mk

Lines 7 to 13 in 19d4d92

qrencode_linux_packages = qrencode
qrencode_darwin_packages = qrencode
qrencode_mingw32_packages = qrencode
qt_linux_packages:=qt expat libxcb xcb_proto libXau xproto freetype fontconfig libxkbcommon libxcb_util libxcb_util_render libxcb_util_keysyms libxcb_util_image libxcb_util_wm
qt_darwin_packages=qt
qt_mingw32_packages=qt

We already handle the exact logic for the systemtap (USDT) package. Therefore, it is reasonable to use this working logic uniformly for all cases.

Additionally, this PR correctly handles cases when a *_packages_ variable contains only spaces rather than being empty.

How to test this PR on a *BSD system:

  1. Build depends following the build guides (depends/README.md, doc/build-*bsd.md)
  2. Configure the main build system using the generated toolchain file:
cmake -B build --toolchain depends/<your-platform>/toolchain.cmake

It fails.

With this PR it configures successfully.

@hebasto hebasto added the bug Something isn't working label Aug 1, 2024
@hebasto
Copy link
Owner Author

hebasto commented Aug 1, 2024 •
edited
Loading

Friendly ping @theStack @vasild :)

vasild
vasild approved these changes Aug 1, 2024
View reviewed changes
Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 20ef783

... because these package are not available for the host systems ...

Why? FreeBSD has packages for qt and qrencode.

@hebasto
Copy link
Owner Author

hebasto commented Aug 1, 2024

... because these package are not available for the host systems ...

Why? FreeBSD has packages for qt and qrencode.

I did not mean the package availabilty via the system package manager, but rather how our depends build sub-system is configured.

@hebasto
Copy link
Owner Author

hebasto commented Aug 1, 2024 •
edited
Loading

@vasild

Testing instructions have been added to the PR description per your request.

@vasild
Copy link

vasild commented Aug 2, 2024

I tested this on FreeBSD: on cmake-staging it fails with:

-- Checking for module 'libqrencode'
--   Package 'libqrencode' not found
CMake Error at /usr/local/share/cmake/Modules/FindPkgConfig.cmake:634 (message):
  The following required packages were not found:

   - libqrencode

and succeeds with the changes from this branch.

Unrelated to this PR: depends/README.md mentions to run make followed by cmake -B build ... but those have to be run from different directories - make has to be in the depends/ subdirectory. On FreeBSD make spits a ton of syntax errors because depends/Makefile is gnu-make specific, so I used gmake -C depends from the top source directory.

@hebasto hebasto merged commit f63b200 into cmake-staging Aug 2, 2024
40 checks passed
theStack
theStack reviewed Aug 4, 2024
View reviewed changes
Copy link

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

post-merge ACK 20ef783, tested on OpenBSD 7.5
(note that I had to cherry-pick the zeromq build fix for OpenBSD [bitcoin#30565, 89b1d5c] first, as it's seemingly not in the cmake staging branch yet)

@theStack theStack mentioned this pull request Aug 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@theStack theStack theStack left review comments

@vasild vasild vasild approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hebasto @vasild @theStack