-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Android CI with JDK and SDK #42080
base: master
Are you sure you want to change the base?
Android CI with JDK and SDK #42080
Conversation
@m-kuhn Elapsed time to handle qtbase:x64-android: 4.2 min |
allegro5: Uses (downloads) gradle 5 which "supports running Gradle builds with JDK 11". Fails with:
|
7ba693a
to
c1181b6
Compare
c1181b6
to
4a69582
Compare
qtbase 6.7 silently overrides toolchain API level 21 with actual API level 23. qtbase also selects |
@@ -10,8 +10,8 @@ vcpkg_from_github( | |||
minimp3-fix.patch | |||
) | |||
|
|||
if(VCPKG_TARGET_IS_ANDROID AND NOT ENV{ANDROID_HOME}) | |||
message(FATAL_ERROR "${PORT} requires environment variable ANDROID_HOME to be set." ) | |||
if(VCPKG_TARGET_IS_ANDROID AND "$ENV{ANDROID_HOME}" STREQUAL "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first one does't work, the second does. (I was surprised, too.)
if(VCPKG_TARGET_IS_ANDROID) | ||
# Qt requires libc++_shared, cf. <qtbase>/cmake/QtPlatformAndroid.cmake | ||
# and https://developer.android.com/ndk/guides/cpp-support#sr | ||
vcpkg_check_linkage(ONLY_DYNAMIC_CRT) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modeling what is hard-coded in Qt.
Excludes it from vcpkg CI with its current Android configuration.
set(VCPKG_CRT_LINKAGE static) | ||
set(VCPKG_CRT_LINKAGE dynamic) | ||
set(VCPKG_LIBRARY_LINKAGE static) | ||
set(VCPKG_CMAKE_SYSTEM_NAME Android) | ||
set(VCPKG_MAKE_BUILD_TRIPLET "--host=aarch64-linux-android") | ||
set(VCPKG_CMAKE_CONFIGURE_OPTIONS -DANDROID_ABI=arm64-v8a) | ||
set(VCPKG_CMAKE_SYSTEM_VERSION 28) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not acceptable as part of this PR, but I strongly suggest these changes.
Runtime:
- Static CRT linkage chooses static C++ runtime, but this is recommend only if "all of your application's native code is contained in a single shared library". IMO this limits the applicability of the android triplets and CI too much.
- Qt only supports
c++_shared
, and the user's Qt app is a shared object dlopened by native Qt code. - linux and osx triplets also use dynamic CRT linkage.
- At the moment vcpkg initializes API level to 21. This excludes many ports from vcpkg CI. "Cumulative usage": 99.6%
- Qt 6.7 needs 23. "Cumulative usage": 98.6%
- Many POSIX APIs come with 24. "Cumulative usage": 97.2%
- Qt 6.8 (LTS) needs 28. "Cumulative usage": 91.7%
- I don't know if the default shall be changed in the triplet or in
android.cmake
. (In manifest mode with CMake, this should be passed through IMO.)
CC @BillyONeal for team feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More on Qt and API levels: https://www.qt.io/blog/qt-for-android-supported-versions-guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For an API level adjustment which is acceptable for this PR: the qtbase port could show a warning (or error) if there are incompatible API levels and suggest setting it in the triplet, similar to what's discussed here https://github.com/microsoft/vcpkg/pull/41435/files#diff-da814fe5d578d93392e698157af490aeb2f7994eecdf94b272356ae97661ddc4R15-R17
vcpkg/ports/qtbase/cmake/qt_install_submodule.cmake
Lines 15 to 17 in c2e8695
if(VCPKG_TARGET_IS_OSX AND (NOT VCPKG_OSX_DEPLOYMENT_TARGET OR VCPKG_OSX_DEPLOYMENT_TARGET VERSION_GREATER_EQUAL "15.0")) | |
message(WARNING "Qt6.7 does not yet support macOS 15.0, consider adding set(VCPKG_OSX_DEPLOYMENT_TARGET 14.0) or earlier to a custom triplet (https://learn.microsoft.com/en-us/vcpkg/users/examples/overlay-triplets-linux-dynamic#overriding-default-triplets).") | |
endif() |
A global vcpkg default system version policy similar or equal to the qt guideline could be discussed by the team in parallel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we can show an error for an insufficient API level from the portfile, or add an config-time failure to qtbase.
The osx case is a little bit different: The osx doesn't sufficiently control the deployment target, but silently takes the host osx version. Probably breaking vcpkg's ABI tracking promises.
For now I'm still assessing how much CI can be done for Qt on Android, and I tried to trigger the discussion about where to move the android CI to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next barrier: Pristine Qt doesn't support static library linkage for Android.
Linking apps fails with error: duplicate symbol: JNI_OnLoad
(Core lib, platform plugin).
https://bugreports.qt.io/browse/QTBUG-32618
https://bugreports.qt.io/browse/QTBUG-99108
Note that the older issue also links to another issue with androiddeployqt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is a patch for qtbase that has received positive feedback from the qtandroid team but didn't end up being merged (without visible reasons)
https://codereview.qt-project.org/c/qt/qtbase/+/279386
Reprise from #35845.