Skip to content
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

Allow defining the ANDROID_ABI and WAMR_BUILD_TARGET for Android building #3238

Closed
daniel7grant opened this issue Mar 18, 2024 · 8 comments
Closed
Labels
done The feature/issue was implemented/resolved enhancement New feature or request platform core/shared/platform

Comments

@daniel7grant
Copy link

For our use-case, we need to build WAMR for multiple Android platforms, which means that we have to change the ANDROID_ABI and WAMR_BUILD_TARGET variables. Currently these can only be set by modifying CMakeLists.txt in product-mini/platforms/android. This is harder to do when it comes to automatic building (CI), parallelization, etc.

I'd make two modification to be able to set these parameters with -D defines:

if (NOT DEFINED ANDROID_ABI)
    set (ANDROID_ABI "x86")
endif ()

and:

if (NOT DEFINED WAMR_BUILD_TARGET)
    set (WAMR_BUILD_TARGET    "X86_32")
endif ()

With these changes, the cross compile could be called without ever changing the files:

cmake -DANDROID_ABI=armeabi-v7a -DWAMR_BUILD_TARGET=ARMV7A

What do you think? I could create a Pull Request for this and the documentation update.

@wenyongh
Copy link
Contributor

Hi, yes, it is not a good way to hardcode the configurations in the cmake file, I remember it was written that way due to the GitHub CI which only supports android 32-bit. I submitted PR #3239 to rewrite the cmake file and CI file, could you help have a view? Thanks.

@wenyongh wenyongh added enhancement New feature or request platform core/shared/platform labels Mar 19, 2024
@daniel7grant
Copy link
Author

Thanks for your work, I tried and it seems good to me.

Simultaneously I started working on this problem (main...daniel7grant:wasm-micro-runtime:feature/android-build-options), but your CMakeLists.txt seems to be more useful. The only thing I would add from my branch is the documentation: to add a note in the platform-mini/README.md that the flags should be used instead of modifying the CMakeLists.txt file.

@wenyongh
Copy link
Contributor

OK, I updated the document according to you branch and made small changes. Please check whether more need to be added or changed?

@daniel7grant
Copy link
Author

I don't see anything else. Thank you for your help.

@wenyongh
Copy link
Contributor

Welcome.

@daniel7grant
Copy link
Author

One more thing that I just noticed: if the WAMR_BUILD_TARGET is not set (i.e. running without any -D-s), it fails to compile for me, because the CMAKE_SYSTEM_PROCESSOR is not set. It currently breaks the default example code. Probably it would be a good idea to keep the current behaviour and build for a default platform. Otherwise, there should be a better error message, for example "Build target cannot be determined, set WAMR_BUILD_TARGET."

@wenyongh
Copy link
Contributor

Yes, I did some test, it is not easy to set WAMR_BUILD_TARGET according to the CMAKE_SYSTEM_PROCESSOR: CMAKE_SYSTEM_PROCESSOR is available only when project (iwasm) is added in the CMakeLists.txt, but if the latter is added before setting ANDROID_ABI and CMAKE_TOOLCHAIN_FILE, the c/c++ compiler will be set to GNU compiler instead of NDK's compiler, and compilation errors will be reported. So I updated the PR to enforce the developer to set WAMR_BUILD_TARGET explicitly, and use it to set ANDRDOI_ABI if it isn't passed from the command line, and then pass it android.toolchain.cmake, which checks ANDRDOI_ABI and sets CMAKE_ANDROID_ARCH_ABI accordingly:
https://android.googlesource.com/platform/ndk/+/master/build/cmake/android.toolchain.cmake#110

Please help review and try again, let's merge the PR if it works fine for you.

@daniel7grant
Copy link
Author

I tried it and it seems to be working and producing valid libs on every platform. Thank you, feel free to merge it.

@wenyongh wenyongh added the done The feature/issue was implemented/resolved label Mar 21, 2024
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this issue May 27, 2024
Don't hardcode the cmake configurations in the Android platform's CMakeLists.txt.

Fixes bytecodealliance#3238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
done The feature/issue was implemented/resolved enhancement New feature or request platform core/shared/platform
Projects
None yet
Development

No branches or pull requests

2 participants