cmake: Build `test_bitcoin-qt` executable by hebasto · Pull Request #101 · hebasto/bitcoin

hebasto · 2024-02-11T16:06:21Z

A new configuration option is BUILD_GUI_TESTS.

Native Windows / MSVC Notes:

vcpkg configures Qt with -opengl dynamic, which makes the "minimal" platform plugin unusable due to internal Qt bugs.
ctest workarounds this issue by setting the environment variable QT_QPA_PLATFORM=windows.
This approach works nice in the CI for both x64-windows and x64-windows-static triplets. Please note that test_bitcoin-qt.exe fails to run in the CI environment on the master branch.
One who is willing to debug Qt with the "minimal" platform plugin could run the test_bitcoin-qt.exe directly (not driven by ctest).

theuni

I noticed while poking around that cmake adds:
CMakeFiles/test_bitcoin-qt.dir/__/__/__/depends/x86_64-pc-linux-gnu/lib/cmake/Qt5Gui/Qt5Gui_QXcbIntegrationPlugin_Import.cpp.o

The contents of Qt5Gui_QXcbIntegrationPlugin_Import.cpp are:

#include <QtPlugin>
Q_IMPORT_PLUGIN(QXcbIntegrationPlugin)

So I think we're doing that twice?

It seems we should either turn that off somehow, or remove the import from our code?

theuni · 2024-02-23T18:21:20Z

      - name: Test Release configuration
        run: |
-          ctest --test-dir build -j $env:NUMBER_OF_PROCESSORS -C Release
+          ctest --test-dir build -j $env:NUMBER_OF_PROCESSORS -C Release --exclude-regex test_bitcoin-qt


Why? Because it requires a gui? Could you please add a comment?

A "TODO" comment with links to the issues has been added.

hebasto · 2024-02-24T18:42:42Z

I noticed while poking around that cmake adds: CMakeFiles/test_bitcoin-qt.dir/__/__/__/depends/x86_64-pc-linux-gnu/lib/cmake/Qt5Gui/Qt5Gui_QXcbIntegrationPlugin_Import.cpp.o

The contents of Qt5Gui_QXcbIntegrationPlugin_Import.cpp are:
#include <QtPlugin>
Q_IMPORT_PLUGIN(QXcbIntegrationPlugin)
So I think we're doing that twice?

It seems we should either turn that off somehow, or remove the import from our code?

Nice catch! It happens on the staging branch as well. Investigating...

hebasto · 2024-02-25T10:10:35Z

I noticed while poking around that cmake adds: CMakeFiles/test_bitcoin-qt.dir/__/__/__/depends/x86_64-pc-linux-gnu/lib/cmake/Qt5Gui/Qt5Gui_QXcbIntegrationPlugin_Import.cpp.o

The contents of Qt5Gui_QXcbIntegrationPlugin_Import.cpp are:
#include <QtPlugin>
Q_IMPORT_PLUGIN(QXcbIntegrationPlugin)
So I think we're doing that twice?

It seems we should either turn that off somehow, or remove the import from our code?

@theuni

Thank you for this insight! Please consider #103.

This PR has been rebased on #103 and drafted for now.

hebasto · 2024-03-04T16:45:41Z

On Windows, the statically linked test_bitcoin.exe crashes with the minimal platform plugin, which is used by default.

Specifically, WalletTests and AddressBookTests fail due to an unhandled exception in the QMessageBox class.

This behavior differs from our custom static Qt build in the master branch, which does not crash :)

I believe that the root of the difference in behaviour lies in the following Qt configuration options:

master branch: -opengl desktop
vcpkg: -opengl dynamic (a recommended choice)

There are two obvious workarounds for this unfortunate issue:

Keep using our custom static Qt build for statically linked executables.
Disable WalletTests and AddressBookTests for the minimal platform plugin as we do on macOS.

Thought?

cc @sipsorcery

sipsorcery · 2024-03-04T20:18:45Z

+1 to disable the tests for the dynamically linked version.

Reasoning being that the Windows build is for "developers only" and tracking down Qt bugs for a non-production build doesn't seem to offer a good effort to reward tradeoff.

pablomartin4btc

tACK cb1a44f

Tested on Ubuntu 22.04.

...
Tests:
  test_bitcoin ........................ ON
  test_bitcoin-qt ..................... ON
...

./build/src/qt/test/test_bitcoin-qt

Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
********* Start testing of AppTests *********
Config: Using QtTest library 5.15.3, Qt 5.15.3 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 11.3.0), ubuntu 22.04
...

All tests passed.

~InitExecutor : Stopping thread
~InitExecutor : Stopped thread

hebasto · 2024-03-05T07:59:14Z

#101 (comment):

Why? Because it requires a gui? Could you please add a comment?

A "TODO" comment with links to the issues has been added.

The mentioned issues have been fixed in bitcoin-core/gui#803. Please review it as well :)

Find out whether Qt is static explicitly as it's done in the master branch.

hebasto · 2024-03-09T12:30:06Z

Reworked. Rebased. Ready for review.

The PR description has been updated with Windows/MSVC specific notes.

Friendly ping @TheCharlatan @m3dwards @pablomartin4btc @vasild; and @sipsorcery (as a Windows expert) :)

sipsorcery · 2024-03-09T20:24:25Z

With a dynamic linked build on Windows I get:

PS C:\dev\github\hebasto-bitcoin\build> ctest -j $env:NUMBER_OF_PROCESSORS -C Release
...
...
99% tests passed, 1 tests failed out of 129

Total Test time (real) = 504.59 sec

The following tests FAILED:
         35 - fs_tests:fs_tests.cpp (Failed)
Errors while running CTest
Output from these tests are in: C:/dev/github/hebasto-bitcoin/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.

hebasto · 2024-03-09T21:09:14Z

With a dynamic linked build on Windows I get:

PS C:\dev\github\hebasto-bitcoin\build> ctest -j $env:NUMBER_OF_PROCESSORS -C Release
...
...
99% tests passed, 1 tests failed out of 129

Total Test time (real) = 504.59 sec

The following tests FAILED:
         35 - fs_tests:fs_tests.cpp (Failed)
Errors while running CTest
Output from these tests are in: C:/dev/github/hebasto-bitcoin/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.

Yes. That happens when a user has no privilege to create symbolic links. This issue is not specific to this PR change. However, it should be fixed.

sipsorcery · 2024-03-09T21:12:21Z

tACK 4da7634.

pablomartin4btc

re tACK e4b4c98

On Ubuntu 22.04.

./build/src/qt/test/test_bitcoin-qt

...

All tests passed.
...

ctest -j $(nproc)

...

100% tests passed, 0 tests failed out of 128

Total Test time (real) =  22.90 sec

pablomartin4btc

re tACK e4b4c98

Tested cross-building for Windows on WSL Ubuntu 22.04.

All tests passed. (running test_bitcoin-qt.exe successfully in both WSL and Windows 11 Pro)

593b130 cmake: Add `libqrencode` optional package support (Hennadii Stepanov) Pull request description: A new configuration option `WITH_QRENCODE` has been added. This PR, along with #101, concludes the GUI-specific part of the new CMake-based build system. ACKs for top commit: pablomartin4btc: tACK 593b130 Tree-SHA512: ba21d65ccff59e29585bd6ce092216034abcbd9866e5452fe6a1b1e2f0957dc3c48a204c170ce443888852ae165be7cb1846483b031b8cf49e6d986dec722f88

hebasto added the enhancement New feature or request label Feb 11, 2024

hebasto force-pushed the 240211-cmake-BP branch 2 times, most recently from 3c071c2 to 197fcce Compare February 11, 2024 17:37

hebasto marked this pull request as draft February 11, 2024 18:29

hebasto force-pushed the 240211-cmake-BP branch from 197fcce to a0ca82e Compare February 11, 2024 19:07

hebasto marked this pull request as ready for review February 11, 2024 19:32

theuni reviewed Feb 23, 2024

View reviewed changes

hebasto force-pushed the 240211-cmake-BP branch from ba5caff to 67ea3ab Compare February 24, 2024 17:38

hebasto mentioned this pull request Feb 25, 2024

cmake: Do not interfere with Qt plugin handling #103

Merged

hebasto force-pushed the 240211-cmake-BP branch from 67ea3ab to fcff961 Compare February 25, 2024 10:08

hebasto marked this pull request as draft February 25, 2024 10:10

hebasto force-pushed the 240211-cmake-BP branch from fcff961 to 31bbd21 Compare February 25, 2024 14:56

hebasto force-pushed the cmake-staging branch from 15edb82 to 9e07ce3 Compare March 1, 2024 15:33

hebasto force-pushed the 240211-cmake-BP branch from 31bbd21 to cb1a44f Compare March 1, 2024 21:22

pablomartin4btc reviewed Mar 5, 2024

View reviewed changes

hebasto force-pushed the cmake-staging branch from 2716ccf to 81f3d4f Compare March 8, 2024 15:22

hebasto marked this pull request as ready for review March 8, 2024 15:39

hebasto added 3 commits March 9, 2024 10:57

fixup! cmake: Build bitcoin-qt executable

4da7634

Find out whether Qt is static explicitly as it's done in the master branch.

cmake: Build test_bitcoin-qt executable

0be2225

fixup! cmake: Migrate Guix build scripts to CMake

e4b4c98

hebasto force-pushed the 240211-cmake-BP branch from cb1a44f to e4b4c98 Compare March 9, 2024 11:25

hebasto mentioned this pull request Mar 9, 2024

cmake: Add libqrencode optional package support #116

Merged

pablomartin4btc approved these changes Mar 9, 2024

View reviewed changes

pablomartin4btc approved these changes Mar 10, 2024

View reviewed changes

hebasto merged commit 83374fd into cmake-staging Mar 13, 2024

Conversation

hebasto commented Feb 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

theuni Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 24, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto commented Feb 24, 2024

Uh oh!

hebasto commented Feb 25, 2024

Uh oh!

hebasto commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipsorcery commented Mar 4, 2024

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 5, 2024

Uh oh!

hebasto commented Mar 9, 2024

Uh oh!

sipsorcery commented Mar 9, 2024

Uh oh!

hebasto commented Mar 9, 2024

Uh oh!

sipsorcery commented Mar 9, 2024

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hebasto commented Feb 11, 2024 •

edited

Loading

hebasto commented Mar 4, 2024 •

edited

Loading