-
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
[qtbase] enable ci for android #35845
base: master
Are you sure you want to change the base?
Conversation
vcpkg/ports/qtbase/cmake/qt_install_submodule.cmake Lines 11 to 13 in 7b3ea91
The SDK is not installed from what I can see (only the NDK). vcpkg/scripts/azure-pipelines/android/Dockerfile Lines 10 to 16 in 7b3ea91
We could:
|
I added the android sdk. It probably needs more to install specific packages with the sdkmanager (tests will tell). |
74ac394
to
e320000
Compare
Either I am missing something here (I just looked at how |
I guess the docker file is only regenerated once a month. |
Thanks for the hint. |
b0926a3
to
5a7757f
Compare
Looks like the toolchain file is not loaded at the point where the check happens. |
7d74cef
to
a8938b6
Compare
if(VCPKG_TARGET_IS_ANDROID AND NOT ANDROID_SDK_ROOT) | ||
message(FATAL_ERROR "${PORT} requires ANDROID_SDK_ROOT to be set. Consider adding it to the triplet." ) | ||
if(VCPKG_TARGET_IS_ANDROID AND NOT ANDROID_SDK_ROOT AND NOT DEFINED ENV{ANDROID_SDK_ROOT}) # The scripts/toolchains/android.cmake file handles writing the env var to a cmake var | ||
message(FATAL_ERROR "${PORT} requires ANDROID_SDK_ROOT to be set. Set ANDROID_SDK_ROOT as environment variable or in the triplet." ) |
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.
qt_install_submodule operates in script mode. It doesn't see results from the toolchain, and AFAICS it uses a cmake variable ANDROID_SDK_ROOT
at the moment, not an environment variable. So it would need to be set e.g. in the triplet file.
You may change this file to use the environment variable to initialize the cmake variable iff unset. It is the only use of ANDROID_SDK_ROOT
.
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.
Triplets is what I do in my own repo, but I was wondering if we could do this transparently (after all, the ndk path is also picked up from the environment without triplet involved).
But it would certainly be easier to do with the triplet.
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.
That's why I suggested that the portfile could pick the environement variable. (Fully switching to the environment variable might break users.)
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.
Setting it in the triplet ensures the desired (?!) effect on ABI hashes, but is more intrusive on the user side.
The NDK environment variable already affects ABI hashing via the compiler binaries hashes.
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.
Mabye useful in the future: microsoft/vcpkg-tool#1315. This would hash the extra script, and the triplet could remain unchanged.
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.
That's why I suggested that the portfile could pick the environement variable. (Fully switching to the environment variable might break users.)
If it's set to an absolute path, yes. But that would mean every user has to provide a local triplet. And if it's set via env var (as done with the latest iteration of this PR) it means that the ABI hash of the triplet is not really reliable.
c09d856
to
e3dfa2b
Compare
triplets/arm-neon-android.cmake
Outdated
@@ -4,3 +4,4 @@ set(VCPKG_LIBRARY_LINKAGE static) | |||
set(VCPKG_CMAKE_SYSTEM_NAME Android) | |||
set(VCPKG_MAKE_BUILD_TRIPLET "--host=armv7a-linux-androideabi") | |||
set(VCPKG_CMAKE_CONFIGURE_OPTIONS -DANDROID_ABI=armeabi-v7a -DANDROID_ARM_NEON=ON) | |||
set(ANDROID_SDK_ROOT $ENV{ANDROID_SDK_ROOT}) |
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 doesn't help with ABI hashes: The content of the triplet files remains independent of the content of ENV{ANDROID_SDK_ROOT}
.
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.
There is https://learn.microsoft.com/en-us/vcpkg/users/triplets#vcpkg_env_passthrough, but it is documented "for Windows"...
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 only way I know to bring this into ABI hashes is by (a) writing it into an overlay triplet, but as you mentioned, this is intrusive on user side.
Or we respect environment variables, either by inspecting them (b) in the triplet (done currently) or (c) in the android toolchain (done before) or (d) in the port and forward it to cmake configure from there.
And if I understand correctly, your preference is (d)?
793dc60
to
d0e9db8
Compare
I'm afraid I am hitting a wall with installing the SDK through |
@dan-shaw @BillyONeal what is the best way to get an update into the android ci docker image? I assume the current changes should be sufficient. NB: happy to update the version as soon as the path forward is clear, there are currently other PRs waiting for qtbase which are probably merged earlier. |
Please try m-kuhn#1 |
Separate script to acquire SDK, limit CI
Hm, still not successful in CI.
|
Is network connectivity blocked except through vcpkg tool? |
IDK. Let's try m-kuhn#2. |
Fetch packages directly
Good.
Okay, the CI script sets |
I tried to implement this in the port, but it seems |
Enable support for feature
egl
forandroid
forqtbase
.Specifying it as default-feature for android but at the same time specifying the feature as not supported is inconsistent.