-
Notifications
You must be signed in to change notification settings - Fork 159
Support for 16KB page sizes #69
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
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks Rami, added few comments.
build_ssl.sh
Outdated
config_params=( "${build_type}" "shared" "android-${arch}" | ||
"-U__ANDROID_API__" "-D__ANDROID_API__=${ANDROID_API}" ) | ||
echo "Configuring OpenSSL $ssl_version with NDK $ndk" | ||
"-U__ANDROID_API__" "-D__ANDROID_API__=${ANDROID_API}") |
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.
this line has only whitespace changes, can be reverted to where it was
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.
Thx for the comment, done on next version
README.md
Outdated
@@ -1,5 +1,5 @@ | |||
# Android OpenSSL support for Qt | |||
OpenSSL scripts and binaries for Android (useful for Qt Android apps) | |||
OpenSSL scripts and binaries for Android - useful with Qt for Android apps. |
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.
I think 'for' fits better than 'with' here: "..., useful for Qt Android apps"
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.
Thx for the comment. The "Qt for Android" is coming from the Qt documentation terminology https://doc.qt.io/qt-6/android.html, so would like to keep that. Re-phrased sentence a bit shorter and simpler for next version.
build_ssl.sh
Outdated
"-U__ANDROID_API__" "-D__ANDROID_API__=${ANDROID_API}") | ||
if [ "$arch" = "arm64" -o "$arch" = "x86_64" ]; then | ||
if [ "${ndk:0:2}" -ge 25 ]; then | ||
config_params+=("LDFLAGS="$LDFLAGS -Wl,-z,max-page-size=16384"") |
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.
we could add the flags only for ssl 3, since that's the one used for Qt 6.5+ and since ssl 1.1 is used prior to that and those Qt versions don't really support Android 16, arguably we don't need to change them.
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.
Thx for the comment. We could, pre-patch had condition for SSL 3 as well, but as it is used with NDK 25 only decided to remove it, and use only ABI and NDK. Thus would keep it.
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.
could you leave out the binary updating from the PR?
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.
Thx for the comment. Done, if/as the policy is that maintainers will update them.
This patch adds the linker flags for the 64-bit ABI's and relevant NDK's to build 16KB page size binaries. Resolves KDAB#68
This patch adds needed linker flags for FFmpeg Android compilation to get FFmpeg binary to be compliant for 16KB page size. 16KB page size is compliant with 4KB page size. This patch updates Android OpenSSL to be version 3.0.7, which is build with two NDK's: NDK27c and NDK29-beta2. Both with 16KB page size support. They are build with KDAB/android_openssl#69 just changing OpenSSL version from 3.1.1 to 3.0.7 and NDK version from 25.2.9519653 to 27.2.12479018 (27c) and another build with 29.0.13599879 (beta2). The both build artefacts are added to CI-files to be usable. As the 16KB page size is applicable only to 64-bit ABI's this patch adds the variable to 90-install-ffmpeg.sh files for android-x86_64 and android-arm64. Change is picked to all branches using Android 15 or newer as Android 15 introduced the 16KB page size support. Fixes: QTBUG-139762 Pick-to: 6.10 6.9 6.8 6.5 Change-Id: I50bab81b97dad3e9d8c1e834c1928949d1e19687 Reviewed-by: Tero Heikkinen <tero.heikkinen@qt.io>
This patch adds needed linker flags for FFmpeg Android compilation to get FFmpeg binary to be compliant for 16KB page size. 16KB page size is compliant with 4KB page size. This patch updates Android OpenSSL to be version 3.0.7, which is build with two NDK's: NDK27c and NDK29-beta2. Both with 16KB page size support. They are build with KDAB/android_openssl#69 just changing OpenSSL version from 3.1.1 to 3.0.7 and NDK version from 25.2.9519653 to 27.2.12479018 (27c) and another build with 29.0.13599879 (beta2). The both build artefacts are added to CI-files to be usable. As the 16KB page size is applicable only to 64-bit ABI's this patch adds the variable to 90-install-ffmpeg.sh files for android-x86_64 and android-arm64. Change is picked to all branches using Android 15 or newer as Android 15 introduced the 16KB page size support. Fixes: QTBUG-139762 Pick-to: 6.9 6.8 6.5 Change-Id: I50bab81b97dad3e9d8c1e834c1928949d1e19687 Reviewed-by: Tero Heikkinen <tero.heikkinen@qt.io> (cherry picked from commit 875297e) Reviewed-by: Rami Potinkara <rami.potinkara@qt.io>
@@ -1,5 +1,5 @@ | |||
# Android OpenSSL support for Qt | |||
OpenSSL scripts and binaries for Android (useful for Qt Android apps) | |||
OpenSSL scripts and binaries - supports Qt for Android apps. |
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.
this change is not related to 16kb support, could you leave it out under a separate PR?
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.
Thx for the comment. It is not, it is also quite small. But I'll create a ticket for that and update it as separate patch
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.
Within #70
rm -fr "$output_dir" | ||
mkdir -p "$output_dir" || exit 1 | ||
|
||
build_ssl ${log_file} |
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.
this change is not related to 16kb support, could you leave it out under a separate PR?
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.
Thx for the comment. I can, the point being is that the change is needed to make 16KB export to work as well by having the correct header files. So that change is needed together with the new binaries.
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.
Within #70
# Clean include folder | ||
find "$output_dir/../" -name "*.in" -delete | ||
find "$output_dir/../" -name "*.def" -delete | ||
find "$output_dir/../" -name "*.in" -print -delete |
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.
why do we need -print, we're deleting the input files
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.
Thx for the comment. That shows what files are about to be deleted, it helps for the debugging purposes. Relates to change what gets headers on right place.
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.
Within #70
build_ssl.sh
Outdated
strip_libs | ||
copy_build_artefacts ${output_dir} |
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.
this two could move up as well to be called after build_ssl and leave the copying of includes at the end. In general this also could be improvement by calling make install_sw
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.
Within #70 - seems working
"-U__ANDROID_API__" "-D__ANDROID_API__=${ANDROID_API}" ) | ||
echo "Configuring OpenSSL $ssl_version with NDK $ndk" | ||
if [ "$arch" = "arm64" -o "$arch" = "x86_64" ]; then | ||
if [ "${ndk:0:2}" -ge 25 ]; then |
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.
we don't really need to check for this, and the full LDFLAGS
to support even older NDKs we could use
-Wl,-z,max-page-size=16384 -Wl,-z,common-page-size=16384
and in general since OpenSSL 3 is the one we want to enable 16kb for, we need to do it for that only, and explicitly instead of implicitly here with the check for NDK 25.
And to do that explicitly, I suggest these edits to this commit:
From 5d216493a92c8e786278df41710f7f6ac1c69eb7 Mon Sep 17 00:00:00 2001
From: Assam Boudjelthia <assam.boudjelthia@qt.io>
Date: Wed, 1 Oct 2025 12:42:07 +0300
Subject: [PATCH] Support 16kb page sizes for OpenSSL 3
---
build_ssl.sh | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/build_ssl.sh b/build_ssl.sh
index 238a630..21c8390 100755
--- a/build_ssl.sh
+++ b/build_ssl.sh
@@ -117,6 +117,7 @@ configure_ssl() {
fi
done
+ version_config_params=
case $ssl_version in
1.1.*)
ANDROID_API=21
@@ -137,6 +138,9 @@ EOF
;;
3.*)
ANDROID_API=23
+ if [ "$arch" = "arm64" -o "$arch" = "x86_64" ]; then
+ version_config_params=( "-Wl,-z,max-page-size=16384" "-Wl,-z,common-page-size=16384" )
+ fi
# use suffix _3.so with OpenSSL 3.1.x (Qt 6.5.0 and above)
patch -p0 <<EOF
--- Configurations/15-android.conf
@@ -153,11 +157,21 @@ EOF
;;
esac
- config_params=( "${build_type}" "shared" "android-${arch}"
- "-U__ANDROID_API__" "-D__ANDROID_API__=${ANDROID_API}" )
- echo "Configuring OpenSSL $ssl_version with NDK $ndk"
- echo "Configure parameters: ${config_params[@]}"
+ echo "Configuring OpenSSL $ssl_version for ABI $arch with NDK $ndk"
+ config_params=
+
+ if [ -n "$build_type" ]; then
+ config_params+=( "${build_type}" )
+ fi
+ config_params+=( "shared" "android-${arch}" "-U__ANDROID_API__"
+ "-D__ANDROID_API__=${ANDROID_API}" )
+
+ if [ -n "${version_config_params[*]}" ]; then
+ config_params+=( "${version_config_params[@]}" )
+ fi
+
+ echo "Configure parameters: ${config_params[@]}"
./Configure "${config_params[@]}" 2>&1 1>${log_file} | tee -a ${log_file} || exit 1
make depend
}
--
2.50.1 (Apple Git-155)
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.
Without the check for ABI's also the 32-bit ABI's got binaries re-build as 16KB page sized. I checked that with helper script. Do we really want that? It might not make harm though.
16KB checking in Qt for Android and the instructions from Google mention NDK as the factor, not OpenSSL version. I would prefer to keep the NDK there. Like discussed it is a bit matter of taste in case of Qt as the supported Qt 6 versions only support NDK25 and higher and also OpenSSL 3.
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.
hmm, I don't see how this code above script diff would produce 16kb aligned 32-bit libs, there's an explicit check the for arm64 and x64, and locally I'm getting the expected result as well.
This patch - Add missing headers to output folder - Nits README.md terminology Resolves KDAB#70
Support building it with 16KB page sizes. Changes to build script build_ssl.sh, fine tuning to README.md and updated binaries