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

Add an error message if android NDK is not installed #1344

Merged
merged 1 commit into from
Jan 4, 2024

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
@@ -64,6 +64,12 @@ def generate(env):
elif sys.platform == "darwin":
toolchain += "darwin-x86_64"
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
12 checks passed
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants