-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
enable android build #715
enable android build #715
Conversation
libprotobuf-lite is a static lib, you don't need to call target_link_libraries on it. Therefore, I don't think you need to change protobuf's source code. |
@snnn Thanks for your review. Even though protobuf-lite is a static lib, the compiler flag will be spread to the executable by cmake. And what is your opinion on protocolbuffers/protobuf#2719? |
a54537a
to
a8f04fe
Compare
@snnn I have rebased this branch on master. However I believe A possible solution is that since the minimum cmake version is 3.13 now, we can set CMP0079 to NEW so that we can add dependency to protobuf-lite in onnxruntime's cmake. |
In CMakeLists.txt, you may add "log" to onnxruntime_EXTERNAL_LIBRARIES. |
Hi @manashgoswami, could you find someone verifying this PR? Thanks |
@snnn Thanks! I have updated my PR according to your advice. Now the tests compile. |
Are you doing cross-compiling or native ? |
@snnn cross-compiling via android ndk. |
Any suggestion? |
@snnn My PR determines the architecture by CMAKE_ANDROID_ARCH_ABI, which is the variable used by CMake. This is what "cmake official way" means :) Reference: The cmake manual for Android and CMAKE_ANDROID_ARCH_ABI. |
@@ -581,6 +581,10 @@ else() | |||
set(onnxruntime_EXTERNAL_LIBRARIES_DEBUG ${onnxruntime_EXTERNAL_LIBRARIES}) | |||
endif() | |||
|
|||
if (CMAKE_SYSTEM_NAME STREQUAL "Android") | |||
list(APPEND onnxruntime_EXTERNAL_LIBRARIES log) |
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's this library for?
Hi @snnn @yufenglee , is there still something I need to do for this PR? :) |
@daquexian can you please check-in a CI yaml file to ensure future changes don't break the Android build? |
@pranavsharma Hi, I have uploaded the new ci pipeline, however, I haven't tested it and I don't know how to enable it. I build binary directly by cmake instead of build.py, so the python part is not needed. If you approve this way, I will remove all the code about python in android pipeline. |
9a287e5
to
74e4e2e
Compare
926aa19
to
a1c3e3a
Compare
I have tested this PR using my own azure account and verified the CI works(log: https://dev.azure.com/daquexian/onnxruntime/_build/results?buildId=14 for commit 1acfb67. I have reverted two commits which are only for my own azure pipeline). Now it can be tested in this Microsoft repo as long as an admin starts the CI. @linkerzhang @snnn @pranavsharma And @linkerzhang , could I get the privilege to start the CI of this repo for the future development? |
/AzurePipelines run |
Azure Pipelines successfully started running 7 pipeline(s), but failed to run 1 pipeline(s). |
@pranavsharma Thanks! Could you please enable the new Android CI for this PR? The yaml is tools/ci_build/github/azure-pipelines/android-arm64-crosscompile-ci-pipeline.yml |
@linkerzhang Thanks! |
Fix #271, tested with android ndk r19b.
I don't use build.sh but only cmake
My changes:
x86_64-unknown-linux-gnu
for-dumpmachine
on my PC, so the arch should be determined by the cmake official wayAndroid NDK doesn't support filesystem so far (std::filesystem support android/ndk#609), so I set the two flags explicitly for Android. What's more, theFilesystem is not needed since fb2a44f.header_files_test
should be disabled if filesystem is not supported.The result of
onnxruntime_test_all
on Android: