Skip to content

Android: fix GUI not loading on Qt 5.15 #504

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
Closed
wants to merge 2 commits into from
Closed

Android: fix GUI not loading on Qt 5.15 #504

wants to merge 2 commits into from

Conversation

icota
Copy link
Contributor

@icota icota commented Dec 12, 2021 •
edited
Loading

Qt 5.14 introduced certain breaking changes to the way it parses AndroidManifest.xml metadata.

This PR also adds an explicit QAndroidPlatformIntegrationPlugin import to bitcoin.cpp (in line with other platforms). I'm not sure why Android GUI worked without this before but it certainly doesn't on Qt 5.15.

We could do without Q_IMPORT_PLUGIN before because we were manually pulling a JNI_Onload function to the linkage.

@icota icota mentioned this pull request Dec 12, 2021
Merged
@icota
Copy link
Contributor Author

icota commented Dec 12, 2021

cc @hebasto

@hebasto
Copy link
Member

hebasto commented Dec 12, 2021

Concept ACK.

@hebasto
Copy link
Member

hebasto commented Dec 12, 2021

https://cirrus-ci.com/task/5550307582148608:

ld: error: duplicate symbol: JNI_OnLoad
>>> defined at qjnionload.cpp
>>>            qjnionload.o:(JNI_OnLoad) in archive /tmp/cirrus-ci-build/depends/aarch64-linux-android/lib/libQt5Core_arm64-v8a.a
>>> defined at androidjnimain.cpp
>>>            androidjnimain.o:(.text.JNI_OnLoad+0x0) in archive /tmp/cirrus-ci-build/depends/aarch64-linux-android/plugins/platforms/libplugins_platforms_qtforandroid_arm64-v8a.a
clang-12: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [Makefile:6462: qt/test/test_bitcoin-qt] Error 1

@hebasto
Copy link
Member

hebasto commented Dec 12, 2021

Tested f2f924e (configured with --disable-tests):

Screenshot_1639304796

@icota
Copy link
Contributor Author

icota commented Dec 12, 2021

I've updated the PR description to explain why we could get away with not using Q_IMPORT_PLUGIN.

Broken qt_test linkage in CI is related. I'm trying to figure out a nice way to fix this.

@hebasto
Copy link
Member

hebasto commented Dec 12, 2021 •
edited
Loading

I've updated the PR description to explain why we could get away with not using Q_IMPORT_PLUGIN.

Broken qt_test linkage in CI is related. I'm trying to figure out a nice way to fix this.

The following patch fixes link error:

--- a/build-aux/m4/bitcoin_qt.m4
+++ b/build-aux/m4/bitcoin_qt.m4
@@ -162,6 +162,7 @@ AC_DEFUN([BITCOIN_QT_CONFIGURE],[
       AC_DEFINE([QT_QPA_PLATFORM_COCOA], [1], [Define this symbol if the qt platform is cocoa])
     elif test "$TARGET_OS" = "android"; then
       QT_LIBS="-Wl,--export-dynamic,--undefined=JNI_OnLoad -lplugins_platforms_qtforandroid_$ANDROID_ARCH -ljnigraphics -landroid -lqtfreetype_$ANDROID_ARCH $QT_LIBS"
+      QT_TEST_LIBS="-Wl,--export-dynamic,--undefined=JNI_OnLoad -lplugins_platforms_qtforandroid_$ANDROID_ARCH -ljnigraphics -landroid -lqtfreetype_$ANDROID_ARCH $QT_TEST_LIBS"
       AC_DEFINE([QT_QPA_PLATFORM_ANDROID], [1], [Define this symbol if the qt platform is android])
     fi
   fi

Not sure if it is optimal though.

UPDATE: See a more preferable patch in #504 (comment).

hebasto
hebasto reviewed Dec 12, 2021
View reviewed changes
src/qt/bitcoin.cpp
Comment on lines +68 to +69
#elif defined(QT_QPA_PLATFORM_ANDROID)
Q_IMPORT_PLUGIN(QAndroidPlatformIntegrationPlugin);
Copy link
Member

Choose a reason for hiding this comment

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

The same in qt/test/test_main.cpp?

@hebasto
Copy link
Member

hebasto commented Dec 12, 2021 •
edited
Loading

I've updated the PR description to explain why we could get away with not using Q_IMPORT_PLUGIN.
Broken qt_test linkage in CI is related. I'm trying to figure out a nice way to fix this.

The following patch fixes link error:

--- a/build-aux/m4/bitcoin_qt.m4
+++ b/build-aux/m4/bitcoin_qt.m4
@@ -162,6 +162,7 @@ AC_DEFUN([BITCOIN_QT_CONFIGURE],[
       AC_DEFINE([QT_QPA_PLATFORM_COCOA], [1], [Define this symbol if the qt platform is cocoa])
     elif test "$TARGET_OS" = "android"; then
       QT_LIBS="-Wl,--export-dynamic,--undefined=JNI_OnLoad -lplugins_platforms_qtforandroid_$ANDROID_ARCH -ljnigraphics -landroid -lqtfreetype_$ANDROID_ARCH $QT_LIBS"
+      QT_TEST_LIBS="-Wl,--export-dynamic,--undefined=JNI_OnLoad -lplugins_platforms_qtforandroid_$ANDROID_ARCH -ljnigraphics -landroid -lqtfreetype_$ANDROID_ARCH $QT_TEST_LIBS"
       AC_DEFINE([QT_QPA_PLATFORM_ANDROID], [1], [Define this symbol if the qt platform is android])
     fi
   fi

Not sure if it is optimal though.

A more optimal suggestion:

--- a/src/Makefile.qttest.include
+++ b/src/Makefile.qttest.include
@@ -52,7 +52,7 @@ if ENABLE_ZMQ
 qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)
 endif
 qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_CLI) $(LIBBITCOIN_COMMON) $(LIBBITCOIN_UTIL) $(LIBBITCOIN_CONSENSUS) $(LIBBITCOIN_CRYPTO) $(LIBUNIVALUE) $(LIBLEVELDB) \
-  $(LIBLEVELDB_SSE42) $(LIBMEMENV) $(BOOST_LIBS) $(QT_DBUS_LIBS) $(QT_TEST_LIBS) $(QT_LIBS) \
+  $(LIBLEVELDB_SSE42) $(LIBMEMENV) $(BOOST_LIBS) $(QT_LIBS) $(QT_DBUS_LIBS) $(QT_TEST_LIBS) \
   $(QR_LIBS) $(BDB_LIBS) $(MINIUPNPC_LIBS) $(NATPMP_LIBS) $(LIBSECP256K1) \
   $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS) $(SQLITE_LIBS)
 qt_test_test_bitcoin_qt_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(QT_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) $(PTHREAD_FLAGS)

UPDATE: Suggesting to move this PR into the main repo as it touches the code outside the src/qt.

hebasto
hebasto reviewed Dec 12, 2021
View reviewed changes
@hebasto hebasto mentioned this pull request Dec 13, 2021
Merged
@icota
Copy link
Contributor Author

icota commented Dec 13, 2021

UPDATE: Suggesting to move this PR into the main repo as it touches the code outside the src/qt.

Is there a nice way to do this? I'd like to preserve comments and stuff.

@hebasto
Copy link
Member

hebasto commented Dec 13, 2021

@icota

UPDATE: Suggesting to move this PR into the main repo as it touches the code outside the src/qt.

Is there a nice way to do this?

No, there is not, unfortunately.

I'd like to preserve comments and stuff.

A link to this PR in the OP will work fine.

@icota icota mentioned this pull request Dec 13, 2021
Merged
@icota
Copy link
Contributor Author

icota commented Dec 13, 2021

Continued in bitcoin/bitcoin#23757

@icota icota closed this Dec 13, 2021
fanquake added a commit that referenced this pull request Dec 15, 2021
@fanquake
Merge bitcoin/bitcoin#23757: [build] Android: fix GUI not loading on …
965ffe2 
…Qt 5.15

27f353d build, android: Fix Android GUI not loading on Qt 5.15 (Igor Cota)
6fc5c77 build, qt: use static QAndroidPlatformIntegrationPlugin (Igor Cota)

Pull request description:

  PR moved from #504 as it escaped the confines of `src/qt`.

ACKs for top commit:
  hebasto:
    re-ACK 27f353d
  promag:
    utACK 27f353d

Tree-SHA512: 4b6e6b2fb1923b89934f11caa8c05c6f340881689273f0c08916144e623f03fd5b781f1a53af83f6e87dce211fe02a1cb87e5943d13811c791cc8aa458184d9f
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Dec 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@icota @hebasto