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

Display a warning if device CPU architecture is not active in the export preset. #88611

Merged

Conversation

Alex2782
Copy link
Contributor

Fixes #86284

Editor crash / freeze (with arm32 devices) are prevented by selecting the correct device CPU architecture. It should also be more user-friendly. #86284 (comment)

@Alex2782 Alex2782 requested a review from a team as a code owner February 20, 2024 23:16
@Alex2782 Alex2782 force-pushed the force_device_cpu_architecture branch from 5a98ce2 to b58ba6e Compare February 20, 2024 23:18
@akien-mga akien-mga added this to the 4.3 milestone Feb 20, 2024
platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
@Alex2782 Alex2782 force-pushed the force_device_cpu_architecture branch from b58ba6e to 293c34a Compare February 23, 2024 00:37
@Alex2782 Alex2782 changed the title Force device CPU architecture for Android Remote Debug. Display a warning if device CPU architecture is not active in the export preset. Feb 23, 2024
@Alex2782
Copy link
Contributor Author

Alex2782 commented Feb 23, 2024

@m4gr3d
image

I also tried to intercept the 'freeze' somehow. It gets stuck at fgets(buf, 65535, f) (src), because adb install also gets stuck in the terminal window, does not give any errors. (on my arm32 devices)

image

logcat

02-23 01:06:37.626  2915  3058 W NativeHelper: Failure copying native libraries [errorCode=-113]
02-23 01:06:37.626  2915  3058 E PackageInstaller: Commit of session 786393543 failed: Failed to extract native libraries, res=-113

@Alex2782 Alex2782 requested a review from m4gr3d February 23, 2024 00:47
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.

This updated approach looks good to me!

I wonder if it's worth adding an additional Fix and run button to the confirm dialog to automatically add the missing architecture as your previous approach was doing, though that may make the UX looks busy.

cc @Calinou Any thoughts on the UX used in this PR?

@Calinou
Copy link
Member

Calinou commented Feb 23, 2024

I wonder if it's worth adding an additional Fix and run button to the confirm dialog to automatically add the missing architecture as your previous approach was doing, though that may make the UX looks busy.

This should be done if we want to provide a "Run Remote Debug anyway" button, as the button is unlikely to work otherwise. This is even more the case with Android 14 and later which is no longer able to run 32-bit ARM apps on 64-bit ARM devices (to my knowledge).

@Alex2782
Copy link
Contributor Author

I think adb should somehow be executed in a Thread so that the cancel button works properly. The progress window looks incorrect in the first 2 seconds because rendering is blocked. Which godot function could be used as an example?

@akien-mga akien-mga merged commit e7bf883 into godotengine:master Feb 26, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Alex2782
Copy link
Contributor Author

Fix and run button maybe later (new PR).

@Alex2782 Alex2782 deleted the force_device_cpu_architecture branch February 26, 2024 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants