-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Upgrade Android NDK to r23 LTS #61691
Conversation
b1272bd
to
f6ab377
Compare
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'll leave getting Android builds working on Windows for a separate PR
Is there a reason for separating the PRs instead of having them together? I'm worried this would lead to broken workflows for some developers, and a flurry of bugs that would be moot on the second PR lands.
|
||
|
||
def get_android_ndk_root(env): | ||
return env["ANDROID_SDK_ROOT"] + "/ndk/" + get_ndk_version() |
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.
Is there a reason for using get_env_android_sdk_root()
in some places and env["ANDROID_SDK_ROOT"]
in others?
Building Android on Windows is not working at the moment (or at least I'm unable to build Android on Windows) so this PR won't break any workflows that aren't broken already.
|
I'm doing Android development (e.g: building Godot Android shared libraries, apk & build templates, developing and testing the Godot Editor) on Windows (and MacOs) without any issues. What build issues are you running into? |
When I run scons platform=android I get:
I initially assumed that it simply wasn't working on Windows, because of the mix between forward slashes and backward slashes in the command. However, looking into it a bit further, I've noticed a few things:
So just having a path to the Android SDK root isn't sufficient. I'll update this PR to get it to work with Windows, and include some improvements to fix or at least provide better error messages for these issues too. I don't have a Macos machine, but, unless you tell me otherwise, from what I can tell, I assume it works the same as a Linux install. |
Thanks a lot for working on this - being stuck on r21 was starting to be a bother and the re-implemented NDK build config in |
5b730ba
to
feb6224
Compare
Updated to also compile on Windows. |
For the record, the timing is a bit short as we're already at 3.5 RC 4, but I'm thinking that it would be worth spending the time to get this reviewed, tested and merged for 3.5, together with #51815 which is needed for new Google Play requirements (notably for target SDK 31, but also past requirements for general storage support). I'm testing an update of the official build containers to use NDK r23 and Android 32, so far I'm running into issues building Mono which might complicate things. I'll update when I've spent some more time on it. Then I'm planning to make a test build with #44055 + #51815 and ask Android users to test this thoroughly ASAP so we can assess if it's worth delaying 3.5 to make sure we get those in (so first a custom testing build, then if it works we merge and release 3.5 RC 5). So far I can confirm that this PR builds fine on Linux for me without any custom change to my SDK/NDK setup (the new NDK is downloaded by gradle as expected). |
I'm running into some issues getting godot-mono-builds to compile Mono for Android with NDK r23, as it also has some references to old |
@madmiraal The update works fine on Windows, but I'm running into build (linking) errors on macos:
|
@m4gr3d I don't have a Macos machine to test this. To help troubleshoot what's missing, please provide the link call that is failing i.e. using the |
I made a test build of the So that confirms that it works fine for the official buildsystem once upgraded to the latest NDK and build tools. Now we have to see if the binaries actually work as intended (and it's only the standard build since so far we couldn't build Mono with the new NDK, which might be a deal breaker for 3.5-stable). |
As suggested by @neikeq, it seems we can keep building Mono for Android with NDK r21 and build Godot with NDK r23. I did a test build with that setup and it seems functional in a quick test: https://downloads.tuxfamily.org/godotengine/testing/3.5-rc-android-scoped%2bndk23/mono/ So that shouldn't be a blocker anymore. We just need to figure out the macOS linking issue that @m4gr3d reported. |
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.
See my comments, for a possible fix to the MacOS issue.
platform/android/detect.py
Outdated
env["CC"] = compiler_path + "/clang" | ||
env["CXX"] = compiler_path + "/clang++" | ||
env["AR"] = compiler_path + "/llvm-ar" | ||
env["AS"] = bin_utils_path + "/as" |
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.
env["CC"] = compiler_path + "/clang" | |
env["CXX"] = compiler_path + "/clang++" | |
env["AR"] = compiler_path + "/llvm-ar" | |
env["AS"] = bin_utils_path + "/as" | |
env["CC"] = toolchain + "/bin/clang" | |
env["CXX"] = toolchain + "/bin/clang++" | |
env["AR"] = toolchain + "/bin/llvm-ar" | |
env["AS"] = toolchain + "/bin/llvm-as" | |
env["LD"] = toolchain + "/bin/llvm-ld" | |
env["STRIP"] = toolchain + "/bin/llvm-strip" | |
env["RANLIB"] = toolchain + "/bin/llvm-ranlib" |
See the android part of: godotengine/godot-cpp#762
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.
Yes, it was defaulting to use the builtin ranlib
:
ranlib core/libcore.android.debug.armv8.a
warning: /Applications/Xcode_13.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: archive library: core/libcore.android.debug.armv8.a the table of contents is empty (no object file members in the library define global symbols)
There appears to be a known issue with using XCode's ranlib
against a library created using llvm-ar
: llvm/llvm-project#34156
feb6224
to
48efd56
Compare
That should only happen if you pass the |
I've updated the build to use:
I've tested it across all three operating systems (ubuntu, windows and macos), across all four android architectures and both debug and release builds and in all 24 cases it now compiles successfully: https://github.com/madmiraal/godot/actions/runs/2561015681 |
I highly advise adding the other executables too ( |
It is already using the same
There are numerous other llvm programmes that we're not using, but there doesn't appear to be an |
You are right, there there is only If it's using |
I can confirm the latest updates fixed the build issue on macos! |
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.
Changes look great, it's a much needed cleanup and modernization of the SCons setup, aside from adding support for NDK r23. Works well both locally on Mageia 9 and on the official buildsystem on Fedora 36.
Thanks a lot for working on this!
Thanks! |
That's weird there's no llvm-ld in the NDK, I was pretty sure I checked
that before making the other PR, will double check when I get home.
Anyway, if scons uses clang directly (probably set up by the "clang" tool)
then that will wrap around the correct LD program as long as the output
file suffix is correct.
…On Sat, Jun 25, 2022, 19:03 Marcel Admiraal ***@***.***> wrote:
It is already using the same clang++ used for compiling for linking:
/usr/local/lib/android/sdk/ndk/23.2.8568313/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ -o bin/libgodot.android.opt.armv7.so -target armv7a-linux-androideabi24 -Wl,--gc-sections -Wl,--no-undefined -Wl,-z,now -Wl,-soname,libgodot_android.so -shared platform/android/os_android.os platform/android/android_input_handler.os platform/android/file_access_android.os platform/android/audio_driver_opensl.os platform/android/dir_access_jandroid.os platform/android/tts_android.os platform/android/thread_jandroid.os platform/android/net_socket_android.os platform/android/java_godot_lib_jni.os platform/android/java_class_wrapper.os platform/android/java_godot_wrapper.os platform/android/java_godot_view_wrapper.os platform/android/java_godot_io_wrapper.os platform/android/jni_utils.os platform/android/android_keys_utils.os platform/android/display_server_android.os platform/android/plugin/godot_plugin_jni.os platform/android/vulkan/vulkan_context_android.os thirdparty/misc/ifaddrs-android.os main/libmain.android.opt.armv7.a modules/libmodules.android.opt.armv7.a modules/libmodule_webxr.android.opt.armv7.a modules/libmodule_websocket.android.opt.armv7.a modules/libmodule_webrtc.android.opt.armv7.a modules/libmodule_webp.android.opt.armv7.a modules/libmodule_vorbis.android.opt.armv7.a modules/libmodule_visual_script.android.opt.armv7.a modules/libmodule_vhacd.android.opt.armv7.a modules/libmodule_upnp.android.opt.armv7.a modules/libmodule_theora.android.opt.armv7.a modules/libmodule_tga.android.opt.armv7.a modules/libmodule_text_server_fb.android.opt.armv7.a modules/libmodule_text_server_adv.android.opt.armv7.a modules/libmodule_svg.android.opt.armv7.a modules/libmodule_squish.android.opt.armv7.a modules/libmodule_regex.android.opt.armv7.a modules/libmodule_ogg.android.opt.armv7.a modules/libmodule_noise.android.opt.armv7.a modules/libmodule_navigation.android.opt.armv7.a modules/libmodule_msdfgen.android.opt.armv7.a modules/libmodule_mobile_vr.android.opt.armv7.a modules/libmodule_minimp3.android.opt.armv7.a modules/libmodule_meshoptimizer.android.opt.armv7.a modules/libmodule_mbedtls.android.opt.armv7.a modules/libmodule_lightmapper_rd.android.opt.armv7.a modules/libmodule_jsonrpc.android.opt.armv7.a modules/libmodule_jpg.android.opt.armv7.a modules/libmodule_hdr.android.opt.armv7.a modules/libmodule_gridmap.android.opt.armv7.a modules/libmodule_gltf.android.opt.armv7.a modules/libmodule_glslang.android.opt.armv7.a modules/libmodule_gdscript.android.opt.armv7.a modules/libmodule_freetype.android.opt.armv7.a modules/libmodule_enet.android.opt.armv7.a modules/libmodule_dds.android.opt.armv7.a modules/libmodule_csg.android.opt.armv7.a modules/libmodule_bmp.android.opt.armv7.a modules/libmodule_basis_universal.android.opt.armv7.a platform/libplatform.android.opt.armv7.a drivers/libdrivers.android.opt.armv7.a scene/libscene.android.opt.armv7.a servers/libservers.android.opt.armv7.a core/libcore.android.opt.armv7.a modules/freetype/libfreetype_builtin.android.opt.armv7.a modules/msdfgen/libmsdfgen_builtin.android.opt.armv7.a modules/text_server_adv/libharfbuzz_builtin.android.opt.armv7.a modules/text_server_adv/libgraphite_builtin.android.opt.armv7.a modules/text_server_adv/libicu_builtin.android.opt.armv7.a -lOpenSLES -lEGL -lGLESv2 -landroid -llog -lz -ldl
There are numerous other llvm programmes that we're not using, but there
doesn't appear to be an llvm-ld (It appears to just use ld) So I don't
see any reason for just adding llvm-strip if it's not being used.
—
Reply to this email directly, view it on GitHub
<#61691 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM4C3XVCBTTXPBEBVFEUVLVQ43VDANCNFSM5X3WD5TA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Upgrades Android NDK to the current supported LTS version: r23c.
There have been some significant changes over the years; so I've used the Build System Maintainers Guide to get everything working again; and ultimately simplify things considerably. The key changes needed include:
as
.sysroot
; so it's no longer necessary to includesysroot
header and library directories explicitly.libc++
STL, and build systems should prefer to let Clang link the STL.-mfpu=neon
is not needed when compiling (only-mfpu=vfpv3-d16
is needed if NEON is not required) Note: NDK r23 is the last version that will support disabling NEON. This option has already been removed from master, but it's still available on the 3.x branch.I've also made some additional changes:
I'll leave getting Android builds working on Windows for a separate PR.Edit: PR updated to include building Android on Windows.create(env)
, which was only being used by Android.CCACHE
environmental variable.ANDROID_HOME
.ANDROID_NDK_ROOT
as an option.True
variablecan_vectorize
arm64v8
to match the version used in the Java code.-mstackrealign
compile flag to properly align stacks for global constructors.-Oz
not-Os
optimization mode is used. Note: Clang's-Oz
is similar to GCC's-Os
. Clang's-Os
provides a more middle ground option between size and speed.--fix-cortex-a8
for armv7 builds, because it only applied to Thumb builds, which are not been created, and, as far a I know, Clang would now automatically apply this fix anyway.-Wl,-z,noexecstack
and-Wl,-z,relro
linker options, because they are the default with LLD.Closes #44055.