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 option to install android build template for export #85819

Merged

Conversation

Malcolmnixon
Copy link
Contributor

This PR adds a new "--export-android-build-template" command-line option which causes the android build template to be installed before exporting the project.

@BastiaanOlij
Copy link
Contributor

That's pretty cool, definitely helps when people download projects or templates that require this to be installed.

Copy link
Contributor

@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.

This is a great idea!

I tested this by removing the android/build directory in a Quest project, and then running godot --headless --export-debug "Quest" --export-android-build-template and it correctly recreated the android/build directory before rebuilding the APK file. (I also tried the same test but omitting the --export-android-build-template and it fails with the expected message about the build templates not being installed in the project.)

I also skimmed the code, and it looks good to me!

@akien-mga
Copy link
Member

The feature sounds good. I'm not fully sold on the name, as it starts with --export but it actually performs a setup/install. Maybe --setup-android-build-template would be better? The description of the option should also clarify that it has to be used together with one of the actual --export-* options, otherwise it's a no-op from what I can tell (didn't test, but from reading the code).

Architecturally though, I'm not fully satisfied with the implementation, as it breaks platform encapsulation, adding Android specific code in main.cpp and editor_node.cpp, notably in the generic EditorNode::export_preset. Other platforms in the future may also need pre-export steps to be performed and it would be better to have a system for this so that any export preset can specify what steps should be ensured before an export, either automatically or optionally with a command line argument.

That being said, the issue pre-exists this PR, since install_android_template() is a method of the generic ExportTemplateManager, and we expose a dedicated option in the editor to install the Android template. So that's not something that needs to be fixed in this PR, but I do think we need to rethink this stuff in the future.

This PR adds a new "--install-android-build-template" command-line option which causes the android build template to be installed before exporting the project.
@Malcolmnixon Malcolmnixon force-pushed the export-android-build-template branch from 7910b5d to 988c1bf Compare December 8, 2023 14:00
@Malcolmnixon
Copy link
Contributor Author

In the previous version I used the name --export-android-build-template because I wanted an implicit association with the other export options (--export-release, --export-debug, --export-pack), but you're right - it is better to have the dependency on performing an export explicitly stated rather than trying to use implicit name-association.

How's about --install-android-build-template as that's the exact text of the UI/Menu option, so no new terminology (Export / Setup) is introduced. The help now shows:

  --install-android-build-template  Install the android build template. Used in conjunction with --export-release or --export-debug.

Copy link
Member

@akien-mga akien-mga 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 to me.

I still have architectural concerns with the way we do this as pointed above, but this predates this PR.

@YuriSizov YuriSizov merged commit 9dce1a4 into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants