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

Fix Android export failing with custom keystores and no JDK setup in the OS environment #94809

Conversation

ChrisBase
Copy link
Contributor

@ChrisBase ChrisBase commented Jul 26, 2024

Test environment

Godot Engine v4.3.rc1.official
Windows 11 Pro, Version 23H2, Build 22631.3880

Issue

Export for Android fails on Windows if there is no path to a JDK set in the PATH environment variable and debug and/or release keystores have been set in the options dialg of the export window.
This issue should also affect other operating systems but i have only tested in windows.
The error message printed in the console looks like this:

Could not create child process: keytool -list -keystore ******** -storepass ******** -alias ********

Fix

The reason for this is that the static function has_valid_keystore_credentials in platform/android/export/export_plugin.cpp runs the hardcoded "keytool" command instead of using get_keytool_path to get the correct path that uses the export/android/java_sdk_path setting.
The proposed fix replaces all occurrences of the hardcoded "keytool" with a string returned by get_keytool_path.

@ChrisBase ChrisBase requested a review from a team as a code owner July 26, 2024 17:13
@AThousandShips AThousandShips changed the title Fixed Android export failing with custom keystores and no JDK setup in the OS environment Fix Android export failing with custom keystores and no JDK setup in the OS environment Jul 26, 2024
@AThousandShips
Copy link
Member

Please open a dedicated bug report to track this properly

@AThousandShips AThousandShips added bug platform:android topic:porting topic:export cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 26, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Jul 26, 2024
@ChrisBase
Copy link
Contributor Author

Related issue: #94815

@@ -2340,6 +2340,7 @@ static bool has_valid_keystore_credentials(String &r_error_str, const String &p_
}

bool EditorExportPlatformAndroid::has_valid_username_and_password(const Ref<EditorExportPreset> &p_preset, String &r_error) {
String keytool = get_keytool_path();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use get_keytool_path() directly in has_valid_keystore_credentials instead of passing it as an argument?

Copy link
Contributor Author

@ChrisBase ChrisBase Jul 26, 2024

Choose a reason for hiding this comment

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

Not without changing the signature of has_valid_keystore_credentials() which currently is static, so no access to the member function get_keytool_path() is given.

Copy link
Contributor

Choose a reason for hiding this comment

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

get_keytool_path() is also static but the issue seems to be because has_valid_keystore_credentials is not declared in the header file. Adding that declaration should resolve the access issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed get_keytool_path() beeing static. There's not even a need for declaring has_valid_keystore_credentials in the header file. Calling EditorExportPlatformAndroid::get_keytool_path() (added class specifier) is sufficient.
I will amend the pull request accordingly.

…nt and custom keystores have been set in the export dialog.
@ChrisBase ChrisBase force-pushed the fix_keytool_for_android_export_not_found branch from d4912ed to 7afefe6 Compare July 26, 2024 23:35
Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the fix!

@m4gr3d m4gr3d modified the milestones: 4.4, 4.3 Jul 26, 2024
@m4gr3d m4gr3d removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Jul 26, 2024
@ChrisBase
Copy link
Contributor Author

ChrisBase commented Jul 27, 2024

Looks good, thanks for the fix!

It is a pleasure to support such a great project!

@ChrisBase ChrisBase closed this Jul 27, 2024
@ChrisBase ChrisBase reopened this Jul 27, 2024
@akien-mga akien-mga merged commit 8239eac into godotengine:master Jul 28, 2024
36 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@ChrisBase ChrisBase deleted the fix_keytool_for_android_export_not_found branch July 28, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release platform:android topic:editor topic:export
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android export failing with custom keystores and no JDK setup in the OS environment
5 participants