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

Remove Android specific abis from the export preset feature list #84720

Merged

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Nov 10, 2023

The presence of those abis cause them to be included in the set of p_features passed to the gdextension_export_plugin#_export_file(...) method, which caused them to be lumped in the features_wo_arch set.
When trying to find the gdextension library path, we use a predicate with the following logic:

[features_wo_arch, arch_tag](String p_feature) { return features_wo_arch.has(p_feature) || (p_feature == arch_tag); }

For a gdextension config file like the one below, this causes the first android entry (android.armeabi-v7a = ...) to always be returned regardless of archs since it always satisfies the predicate.

[configuration]

entry_symbol = "example_library_init"
compatibility_minimum = 4.1

[libraries]
linux.x86_64 = "res://libgdexample.so"

android.armeabi-v7a = "res://libgdexample.android.template_release.armeabi-v7a.so"
android.arm32 = "res://libgdexample.android.template_release.armeabi-v7a.so"
android.x86 = "res://x86/libgdexample.android.template_release.x86.so"
android.x86_32 = "res://x86/libgdexample.android.template_release.x86.so"
android.x86_64 = "res://libgdexample.android.template_release.x86_64.so"
android.arm64-v8a = "res://libgdexample.android.template_release.arm64-v8a.so"
android.arm64 = "res://libgdexample.android.template_release.arm64-v8a.so"

Fixes #82727

@m4gr3d m4gr3d added bug topic:gdextension cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Nov 10, 2023
@m4gr3d m4gr3d added this to the 4.2 milestone Nov 10, 2023
@m4gr3d m4gr3d requested review from dsnopek and bruvzg November 10, 2023 15:07
@m4gr3d m4gr3d force-pushed the fix_missing_gdextension_so_files branch from 350527c to 15fa826 Compare November 10, 2023 15:09
@akien-mga
Copy link
Member

akien-mga commented Nov 10, 2023

These architectures/feature tags are meant to be the Godot-specific ones (arm32, arm64, x86_32, x86_64).

I'm not sure we should start adding all the architectures identifiers which may be used on specific platforms there (e.g. aarch64 on Linux, x86-64 on macOS, x64 on Windows, etc.). Keeping it standardized would be good. Maybe we just shouldn't add Android-specific arch identifiers as feature tags?

@bruvzg
Copy link
Member

bruvzg commented Nov 10, 2023

For a gdextension config file like the one below, this causes the first android entry (android.armeabi-v7a = ...) to always be returned regardless of archs since it always satisfies the predicate.

I would add a separate list of invalid architectures and skip ("unknown architecture" with error message) armeabi-v7a and arm64-v8a altogether. For consistency with other platforms (since everything else in Godot expect arm32 and arm64 instead).

@bruvzg
Copy link
Member

bruvzg commented Nov 10, 2023

Or maybe we should require to always specify architecture, currently it's not the case for macOS and iOS (since binary can be universal), but we can require to specify universal as architecture for these case and consider all entries without arch as invalid.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Nov 10, 2023

These architectures/feature tags are meant to be the Godot-specific ones (arm32, arm64, x86_32, x86_64).

I'm not sure we should start adding all the architectures identifiers which may be used on specific platforms there (e.g. aarch64 on Linux, x86-64 on macOS, x64 on Windows, etc.). Keeping it standardized would be good. Maybe we just shouldn't add Android-specific arch identifiers as feature tags?

That could be the approach; we add the Android specific ones automatically in the feature list when the Godot specific arch is selected, so we can either disable that logic or provide an error message to indicate to the user they can't be used in the gdextension config.

Screenshot from 2023-11-10 07-19-19

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Nov 10, 2023

For a gdextension config file like the one below, this causes the first android entry (android.armeabi-v7a = ...) to always be returned regardless of archs since it always satisfies the predicate.

I would add a separate list of invalid architectures and skip ("unknown architecture" with error message) armeabi-v7a and arm64-v8a altogether. For consistency with other platforms (since everything else in Godot expect arm32 and arm64 instead).

To @akien-mga's point, the list would need to include the invalid architectures for all supported platforms to be consistent.

The presence of those abis cause them to be included in the set of `p_features` passed to the `gdextension_export_plugin#_export_file(...)` method, which caused them to be lumped in the `features_wo_arch` set.
When trying to find the gdextension library path, we use a predicate with the following logic:

```
[features_wo_arch, arch_tag](String p_feature) { return features_wo_arch.has(p_feature) || (p_feature == arch_tag); }
```

For a `gdextension` config file like the one below, this causes the first android entry (`android.armeabi-v7a = ...`) to always be returned regardless of archs since it always satisfies the predicate.

```
[configuration]

entry_symbol = "example_library_init"
compatibility_minimum = 4.1

[libraries]
linux.x86_64 = "res://libgdexample.so"

android.armeabi-v7a = "res://libgdexample.android.template_release.armeabi-v7a.so"
android.arm32 = "res://libgdexample.android.template_release.armeabi-v7a.so"
android.x86 = "res://x86/libgdexample.android.template_release.x86.so"
android.x86_32 = "res://x86/libgdexample.android.template_release.x86.so"
android.x86_64 = "res://libgdexample.android.template_release.x86_64.so"
android.arm64-v8a = "res://libgdexample.android.template_release.arm64-v8a.so"
android.arm64 = "res://libgdexample.android.template_release.arm64-v8a.so"
```
@m4gr3d m4gr3d force-pushed the fix_missing_gdextension_so_files branch from 15fa826 to dce2686 Compare November 10, 2023 15:33
@m4gr3d m4gr3d requested a review from a team as a code owner November 10, 2023 15:33
@m4gr3d
Copy link
Contributor Author

m4gr3d commented Nov 10, 2023

I've updated the fix to remove the Android specific abis from the feature list.

@m4gr3d m4gr3d changed the title Fix GDExtension library resolution logic Remove Android specific abis from the export preset feature list Nov 10, 2023
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

If it's not used for something Android specific, it seems fine to me.

@akien-mga akien-mga merged commit 17a18fa into godotengine:master Nov 10, 2023
@akien-mga
Copy link
Member

Thanks!

@m4gr3d m4gr3d deleted the fix_missing_gdextension_so_files branch November 10, 2023 21:05
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

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.

Godot missing .so files in APK file using GDExtension
4 participants