Skip to content

Conversation

@ArchLinus
Copy link
Contributor

I had trouble figuring out why I couldn't compile for android and realized I didn't have the right NDK version installed. So I figured I would add an error message to make it more obvious.

@ArchLinus ArchLinus requested a review from a team as a code owner December 29, 2023 14:55
@dsnopek
Copy link
Collaborator

dsnopek commented Dec 29, 2023

Does this actually lead to the message getting printed in your testing?

In looking into this previously, this code:

def exists(env): return get_android_ndk_root(env) is not None 

... was preventing the generate() from running at all, and so any helpful message there wouldn't get shown (at least this was the case the last time I was looking into this problem).

See #791

@dsnopek
Copy link
Collaborator

dsnopek commented Dec 29, 2023

Yeah, actually, if you look at the top of the generate() function, it already has a helpful error message, the problem is that generate() doesn't get run if the Android NDK isn't setup.

@ArchLinus
Copy link
Contributor Author

Does this actually lead to the message getting printed in your testing?

Yes, the printing worked, tested on Windows:

$ scons platform=android scons: Reading SConscript files ... Building for architecture arm64 on platform android ERROR: Could not find NDK toolchain at C:/Users/<User>/AppData/Local/Android/Sdk/ndk/23.2.8568313/toolchains/llvm/prebuilt/windows-x86_64. Make sure NDK version 23.2.8568313 is installed. 

whereas before I was just getting linker errors:

... scons: *** [src\variant\vector4i.android.template_debug.arm64.o] The system cannot find the file specified scons: *** [gen\src\variant\string.android.template_debug.arm64.o] The system cannot find the file specified scons: *** [gen\src\variant\string_name.android.template_debug.arm64.o] The system cannot find the file specified scons: building terminated because of errors. 

Yeah, actually, if you look at the top of the generate() function, it already has a helpful error message, the problem is that generate() doesn't get run if the Android NDK isn't setup.

For me it was getting run. The problem is that it doesn't actually check if the files exist on disk, all it does is construct a path to the NDK. I could modify it to do the exists check in get_android_ndk_root if you think that would be a better solution.

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, so this is handling the case where ANDROID_HOME or ANDROID_NDK_ROOT are set, but NDK can't actually be found at the specified location?

That makes sense! And it seems to be working in my testing :-)

tools/android.py Outdated
env.Append(LINKFLAGS=["-shared"])

if not os.path.exists(toolchain):
print("ERROR: Could not find NDK toolchain at " + toolchain + ".")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print("ERROR: Could not find NDK toolchain at " + toolchain + ".")
print("ERROR: Could not find NDK toolchain at " + toolchain + ".")

Trailing whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@dsnopek dsnopek merged commit b1769a7 into godotengine:master Jan 4, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Jan 4, 2024

Thanks! And congrats for your first merged Godot contribution 🎉

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 22, 2024

Cherry-picking to 4.2 in #1372

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 22, 2024

Cherry-picking to 4.1 in #1373

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform:android topic:buildsystem Related to the buildsystem or CI setup

3 participants