Use the editor-defined Android keystore for both debug and release mode #39207

Calinou · 2020-05-31T19:50:01Z

This paves the work for making release mode the default export option, since it performs better and results in smaller binaries.

The "debug" keystore option was renamed to "default" in the Editor Settings. People using older configuration files will have to update their editor settings to follow the rename.

The OpenJDK-related messages now specify that version 8 is required (and that more recent versions won't work).

I haven't been able to build for Android lately, so I'd like to have someone test this before it can be merged (just in case). cc @pouleyKetchoupp

PS: This change might be worth cherry-picking to the 3.2 branch as the keystore choice logic is a common point of confusion among newer Godot users.

This paves the work for making release mode the default export option, since it performs better and results in smaller binaries. The "debug" keystore option was renamed to "default" in the Editor Settings. People using older configuration files will have to update their editor settings to follow the rename. The OpenJDK-related messages now specify that version 8 is required (and that more recent versions won't work).

Calinou · 2020-05-31T19:52:33Z

platform/android/export/export.cpp

@@ -1764,16 +1764,16 @@ class EditorExportPlatformAndroid : public EditorExportPlatform {

 		if (!FileAccess::exists(js)) {
 			valid = false;
-			err += TTR("OpenJDK jarsigner not configured in the Editor Settings.") + "\n";
+			err += TTR("OpenJDK 8 jarsigner not configured in the Editor Settings.") + "\n";
 		}

 		String dk = p_preset->get("keystore/debug");


We don't know whether the user will be exporting in release mode at this point. How could error checking be improved to take into account a situation where only a release keystore is defined in the export preset, but no default keystore is defined in the Editor Settings? Right now, this will be considered an error.

I think it's fine to have an error in this case, given the way things are currently set up for debug and release.

pouleyKetchoupp

I like the idea of using a default keystore that can be used for both debug and release. It would be probably ok to cherry-pick for 3.2 as long as the documentation and error messages are explanatory.

I'm still compiling for Android, so I will finish the tests later, but in the meantime I've let some comments for a few things.

About making release the default export mode:
I'm not so sure about that part, since we might want to make sure users deploy a build with logs and errors by default.
Although, a nice alternative would be to be able choose between debug and release when clicking the android quick launch button.
Ideally, something like what Git Extensions use for Pull options would be great:

The button itself can default to the latest chosen option (or debug by default), and a drop-down list could allow launching debug/release.

edit: This comment about release as default export is only for the quick launch button. I think the changes in #39208 are fine as long as they affect only the export window.

pouleyKetchoupp · 2020-06-01T07:22:29Z

platform/android/export/export.cpp

@@ -1764,16 +1764,16 @@ class EditorExportPlatformAndroid : public EditorExportPlatform {

 		if (!FileAccess::exists(js)) {
 			valid = false;
-			err += TTR("OpenJDK jarsigner not configured in the Editor Settings.") + "\n";
+			err += TTR("OpenJDK 8 jarsigner not configured in the Editor Settings.") + "\n";


The documentation says JDK 6 or 8:
https://docs.godotengine.org/en/3.2/getting_started/workflow/export/exporting_for_android.html#setting-it-up-in-godot

Does it actually work only with version 8? Also I don't know if it's already in your plans, but the documentation could also be updated to explicitly say that versions above 8 would fail.

I don't know if OpenJDK 6 is still supported by Godot for Android signing. Either way, it's very old and definitely EOL (just like OpenJDK 8, but that one is still commonly used in companies).

pouleyKetchoupp · 2020-06-01T07:28:53Z

platform/android/export/export.cpp

@@ -1764,16 +1764,16 @@ class EditorExportPlatformAndroid : public EditorExportPlatform {

 		if (!FileAccess::exists(js)) {
 			valid = false;
-			err += TTR("OpenJDK jarsigner not configured in the Editor Settings.") + "\n";
+			err += TTR("OpenJDK 8 jarsigner not configured in the Editor Settings.") + "\n";
 		}

 		String dk = p_preset->get("keystore/debug");


I think it's fine to have an error in this case, given the way things are currently set up for debug and release.

pouleyKetchoupp · 2020-06-01T07:33:19Z

platform/android/export/export.cpp


+			if (p_debug) {
 				if (ep.step("Signing debug APK...", 103)) {


An explicit error message here when the keystore is not defined could help users, something like:
"No default keystore defined in Editor Settings and no debug keystore defined in Export settings, can't sign the debug APK."
(same thing for release)

pouleyKetchoupp · 2020-06-01T07:37:36Z

platform/android/export/export.cpp

+	EDITOR_DEF("export/android/default_keystore", "");
+	EditorSettings::get_singleton()->add_property_hint(PropertyInfo(Variant::STRING, "export/android/default_keystore", PROPERTY_HINT_GLOBAL_FILE, "*.keystore"));
+	EDITOR_DEF("export/android/default_keystore_user", "androiddebugkey");
+	EDITOR_DEF("export/android/default_keystore_pass", "android");


For some reason, when going to Editor Settings after compiling with these changes, I still have the old debug keystore settings showing on top of the new default keystore settings:

Maybe because of some cache?
That could be misleading for users at the moment this change happens, but I have no idea how to fix it.

I also saw this happening when testing. It seems there's no "revert" button for unknown settings anymore, unlike what https://github.com/godotengine/godot/issues/22099 says.

pouleyKetchoupp · 2020-06-01T07:58:41Z

cc. @m4gr3d in case he wants to share extra feedback.

m4gr3d · 2020-06-01T08:41:24Z

This paves the work for making release mode the default export option, since it performs better and results in smaller binaries.

The "debug" keystore option was renamed to "default" in the Editor Settings. People using older configuration files will have to update their editor settings to follow the rename.

The OpenJDK-related messages now specify that version 8 is required (and that more recent versions won't work).

I haven't been able to build for Android lately, so I'd like to have someone test this before it can be merged (just in case). cc @pouleyKetchoupp

PS: This change might be worth cherry-picking to the 3.2 branch as the keystore choice logic is a common point of confusion among newer Godot users.

@Calinou I'm not sure I follow the reason for the change so a clarification may help, but what I'm seeing so far doesn't seem like a good idea for the following reasons:

Exporting with debug builds is useful for development of the project for obvious reasons. For most professional developments, a release build is not generated until near completion of the project. In Android on particular, certain logcat messages are only available in debug builds.
With this change (and related changes) it seems we're optimizing for demo purposes at the expense of the development process. For example, as @pouleyKetchoupp mentioned, this change would affect the quick launch button, which would break the development flow for several developers, myself included.
Is this solving an actual issue? I don't see any attached to this PR or PR Export projects in release mode by default #39208. Especially, since getting access to a release build is rather trivial when going through the full export process.
In Android development, you never want to use a debug keystore for release builds. Even if it's inconvenient, we should force the use to go through the process of generating a keystore specifically for release build. This is a big DO NOT DO in Android development, and we should not encourage users by making that the default.

As I mentioned, it may be that I misunderstood the reasons for this PR. If so, I'll be glad to corrected, but as it stands, this PR and PR #39208 are problematic.

Calinou · 2020-06-01T10:22:31Z

PS: The points below would be better discussed in #39208, since this PR doesn't change the default export mode. If we end up not agreeing on exporting in release mode by default, this PR could still be merged without merging #39208.

Is this solving an actual issue? I don't see any attached to this PR or PR #39208. Especially, since getting access to a release build is rather trivial when going through the full export process.

Try searching around on Reddit or the Godot Q&A, and you'll find many people who ran into performance issues or unexpectedly large binaries because they accidentally exported a debug build. I think exporting in release mode by default is generally what's expected, especially by casual developers. A lot of people I know will never bother with logcat, so I'd say exporting a debug build is the exception at this point. I've probably seen this dozens of times over the years now.

I understand professional developers will want to export in debug mode most of the time, but this should be handled by one-click deploy during development. If not, just check the "Export in debug mode" checkbox in the export dialog. Hopefully, a professional developer will have a harder time missing it 🙂 (that checkbox often goes unnoticed)

Also, Godot has occasional issues with bugs popping up only in release mode, such as #28922. Exporting in release mode at the last minute may hide this kind of bug until you encounter it.

For example, as @pouleyKetchoupp mentioned, this change would affect the quick launch button

This change shouldn't affect one-click deploy in any way, as the debug argument is always true. One-click deploy should obviously keep using debug mode so that debug-only features can work there.

That said, we could consider adding a way to use one-click deploy in release mode for cases where you absolutely need high performance during testing (or to check for bugs that only appear in release mode, as mentioned above).

In Android development, you never want to use a debug keystore for release builds. Even if it's inconvenient, we should force the use to go through the process of generating a keystore specifically for release build. This is a big DO NOT DO in Android development, and we should not encourage users by making that the default.

While it obviously won't work when uploading the APK to Google Play, it should still work fine on a local device. You can still define a release keystore override in the export preset. I wish we could add documentation for individual export preset properties, so we could further clarify this…

vnen · 2020-06-01T17:39:35Z

I'm okay with making release the default mode to avoid the mistake of publishing debug builds. But we should also avoid the mistake of publishing with a debug keystore. The best way to manage this is to show an error message if the release keystore is missing and the user is trying to export as release.

In Android development, you never want to use a debug keystore for release builds. Even if it's inconvenient, we should force the use to go through the process of generating a keystore specifically for release build. This is a big DO NOT DO in Android development, and we should not encourage users by making that the default.

While it obviously won't work when uploading the APK to Google Play, it should still work fine on a local device. You can still define a release keystore override in the export preset. I wish we could add documentation for individual export preset properties, so we could further clarify this…

I see this point not in terms that it doesn't work, but more about adding a default setting that most likely will induce users into making a mistake (that is, releasing with a debug keystore). Showing an appropriate error message will avoid the mistake, so it's better being annoying in this case.

Calinou · 2020-06-01T17:43:11Z

As discussed with @m4gr3d on IRC, this is probably not a good idea so I'll close it. #39208 should be discussed separately, but he said he's OK with the idea as long as the release keystore requirement remains.

Calinou added enhancement platform:android topic:editor topic:porting labels May 31, 2020

Calinou added this to the 4.0 milestone May 31, 2020

Calinou commented May 31, 2020

View reviewed changes

Calinou mentioned this pull request May 31, 2020

Export projects in release mode by default #39208

Closed

pouleyKetchoupp reviewed Jun 1, 2020

View reviewed changes

m4gr3d self-requested a review June 1, 2020 08:42

Calinou closed this Jun 1, 2020

Calinou added the archived label Jun 1, 2020

Calinou deleted the export-android-default-keystore branch August 23, 2020 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the editor-defined Android keystore for both debug and release mode #39207

Use the editor-defined Android keystore for both debug and release mode #39207

Calinou commented May 31, 2020 •

edited

Loading

Calinou May 31, 2020 •

edited

Loading

pouleyKetchoupp Jun 1, 2020

pouleyKetchoupp left a comment •

edited

Loading

pouleyKetchoupp Jun 1, 2020

Calinou Jun 1, 2020 •

edited

Loading

pouleyKetchoupp Jun 1, 2020

pouleyKetchoupp Jun 1, 2020

pouleyKetchoupp Jun 1, 2020

Calinou Jun 1, 2020

pouleyKetchoupp commented Jun 1, 2020

m4gr3d commented Jun 1, 2020 •

edited

Loading

Calinou commented Jun 1, 2020 •

edited

Loading

vnen commented Jun 1, 2020

Calinou commented Jun 1, 2020

Use the editor-defined Android keystore for both debug and release mode #39207

Use the editor-defined Android keystore for both debug and release mode #39207

Conversation

Calinou commented May 31, 2020 • edited Loading

Calinou May 31, 2020 • edited Loading

Choose a reason for hiding this comment

pouleyKetchoupp Jun 1, 2020

Choose a reason for hiding this comment

pouleyKetchoupp left a comment • edited Loading

Choose a reason for hiding this comment

pouleyKetchoupp Jun 1, 2020

Choose a reason for hiding this comment

Calinou Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

pouleyKetchoupp Jun 1, 2020

Choose a reason for hiding this comment

pouleyKetchoupp Jun 1, 2020

Choose a reason for hiding this comment

pouleyKetchoupp Jun 1, 2020

Choose a reason for hiding this comment

Calinou Jun 1, 2020

Choose a reason for hiding this comment

pouleyKetchoupp commented Jun 1, 2020

m4gr3d commented Jun 1, 2020 • edited Loading

Calinou commented Jun 1, 2020 • edited Loading

vnen commented Jun 1, 2020

Calinou commented Jun 1, 2020

Calinou commented May 31, 2020 •

edited

Loading

Calinou May 31, 2020 •

edited

Loading

pouleyKetchoupp left a comment •

edited

Loading

Calinou Jun 1, 2020 •

edited

Loading

m4gr3d commented Jun 1, 2020 •

edited

Loading

Calinou commented Jun 1, 2020 •

edited

Loading