-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
C#: Fallback to CoreCLR/MonoVM hosting APIs when hostfxr/NativeAOT fails #88803
Conversation
@@ -58,6 +58,9 @@ dependencies { | |||
if (pluginsBinaries != null && pluginsBinaries.size() > 0) { | |||
implementation files(pluginsBinaries) | |||
} | |||
|
|||
// .NET dependencies | |||
implementation files('../../../../modules/mono/thirdparty/libSystem.Security.Cryptography.Native.Android.jar') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like including the .jar like this. This .jar comes from the .NET runtime pack, but we don't have it at the time of building the export template and adding a .jar when exporting the project doesn't seem to be possible without forcing the user to enable Use Gradle Build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the comment about using flavor
to guard the dependency, the jar file also needs to be within the app
directory otherwise this will break editor gradle builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For gradle builds we probably want to get the jar file from the built project. But for now I can just move the jar to the app
directory, should I put it in any specific subdirectory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would typically go into the libs
directory, but the contents of that directory get deleted during a clean build, so let's create a new monoLibs
directory and include the jar there.
For gradle builds we probably want to get the jar file from the built project
How would that work? Does the user project include the jar file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would that work? Does the user project include the jar file?
When the C# project is built, during the restore step it will retrieve all the dependencies packages. For android builds, this means it will retrieve the Mono runtime package (e.g.: Microsoft.NETCore.App.Runtime.Mono.android-arm for android-arm32
). This package contains the jar file for the runtime your project targets and we can retrieve it from your NuGet packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the assemble
gradle task for building and it's structured like this: assemble[flavors...][buildtypes...]
.
So by default if you only specify the assemble
task, it builds all the flavors and build types; if you specify the build type (e.g: assembleDebug
), then it builds all the flavors; and so on.. So if you don't change anything, the mono
flavor will be included as part of the build, and if you only want to build the mono
flavor, you can add it as a prefix to the assemble
task.
After you configure the flavor, you can use the Gradle tool window (or the following command) to see the tasks that are being added.
And I guess I have to add a standard flavor for the non-mono version?
Yes you would need to add a standard
flavor as an alternative to the mono
flavor.
Do I have to duplicate every copy{Target}BinaryToBin task for the mono flavor?
As currently setup, you would need to duplicate the copy tasks. Let me take a look and see if I can clean that setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me take a look and see if I can clean that setup.
@raulsntos I've cleaned the gradle build setup in #91271. Feel free to comment on the PR if you have questions on the update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, is something like raulsntos@ad0f8fe what you had in mind? Or should the generateBuildTasks
function take an extra edition
parameter to only include a single edition build task? If so, should I create a new generateGodotTemplatesMono
task for this or can the generateGodotTemplates
task use parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with the second approach and have generateBuildTasks
take an extra edition
parameter and add a generateGodotMonoTemplates
task that invokes it.
That matches our current build setup, where we have separate sections for standard and mono builds. And it allows users to only build the edition they need.
cc @akien-mga as this'll affect the android build scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a PR in the godot-build-scripts
repo:
platform/android/java/lib/src/org/godotengine/godot/GodotLib.java
Outdated
Show resolved
Hide resolved
350eb39
to
3dc32ce
Compare
@raulsntos We'll need to create a separate The addition of that flavor means we'll be able to guard mono specific logic in the java code by checking for the flavor:
This will also result in the creation of additional build templates specifically for the mono builds. @akien-mga we'll need to update the build tools logic for the mono build templates. |
d832b70
to
e79e5ba
Compare
Could you explain what this means and how long to wait for corrections, and then review? |
e79e5ba
to
cc86826
Compare
@Zamir7 As explained in the PR description, this adds support for 32-bit Android architectures and Android OS APIs (mainly crypto, see the linked issue) to C# projects. This PR is marked as a draft, which means it's still a work-in-progress. It is tentatively planned for 4.4 (as you can see in the assigned milestone). |
cc86826
to
29e1d0a
Compare
29e1d0a
to
d2956c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good!
I haven't been able to test, so additional testing and validation would be needed, but that should be the easy part :)
There's one more set of changes that'll be needed to support gradle builds.
The crypto jar file will also need to be copied to the build directory after it's downloaded. Alternatively if you know the path where it'll be downloaded, then |
d2956c7
to
7c80ec4
Compare
It should be copied because this file is retrieved when building the .NET project and it's stored in a temporary directory that will be deleted after the export. We can copy it as part of the It looks like I can check if it's a gradle build with |
@felipejfc Thanks for testing. From that stack trace, it looks like it's trying to use hostfxr to initialize the .NET runtime which shouldn't be possible since the Android export shouldn't contain I'd like to see the csproj so I can check if there's anything that could affect the publish process. For example, something like And I'd like to see the APK to check what native libraries were included in the
I don't think so. It looks like the error is caused by failing to initialize the .NET runtime, not sure what may be causing it. I'd like to see what native libraries are included in the APK. And, if |
Sadly I can't share the whole files since this is a private company project, however, I can provide all info you asked for:
This is the content of android.targets:
This should only be used if NativeAOT + Android build which I'm not setting right now as you can see, only for iOS, but it's there as a placeholder once I want to start building android with it. As for the libs, seems to be there: More logging from debugging with android studio:
Same csproj etc works with 4.3, by just reimporting |
Thanks for sharing, I don't see anything in the csproj or android.targets files that seems problematic. From the screenshot you shared of the APK, I'd say the publish process was successful. It's weird that it's trying to use hostfxr since there's no By any chance have you exported this game before with a previous version of Godot? Since Godot extracts the contents of the APK to a cache directory, it's possible that previously cached files are still there causing issues. It looks like the cache path is |
Spot on! Deleted cache and uninstalled the app, installed it again and it works! We will do good then adding this disclaimer that the application needs to be installed with a fresh cache if the player had a version generated with an old godot build installed When building for armv7 I got the following error Which was fixed by dotnet/android#8170 (comment) Tomorrow I will test on the armv7 device and report result |
We'll fix it so it automatically clears those cached files if the extracted
Thanks a lot, that's really interesting. As mentioned in that issue, I suspect your referenced projects (i.e.: We set |
They are not! Should I reference it? |
I want to mention that the Windows Single File export feature, which involves embedding script files, is currently encountering a similar issue. Specifically, the single-file executable stops working after switching to AOT because the outdated managed assemblies inside the cache directory are loaded again upon launching the new AOT executable. |
@felipejfc If the workaround mentioned in the issue you linked doesn't work, you could try doing that. Ideally you shouldn't have to do anything manually.
@Delsin-Yu Yeah, this is an existing issue. It's reported in #96299. |
@raulsntos Tested in a Motorola ARMv7 device with Adreno GPU 1 - Build crashed with Mobile rendered (because of vulkan I assume)
2 - When built with OpenGL the game worked fine! Idk if the crash has to do with godot or with vulkan driver incompatibility but I would guess the later |
I would not expect to see the Footnotes |
c9702c4
to
f78d359
Compare
Rebased and made new custom builds to test, the cached data should no longer be an issue since #96301 was merged:
And the Android templates you'll need to use to export:
Note Remember to push the provided nupkgs to a local NuGet source so .NET can find the custom packages, see the documentation for more details. |
I’m curious, after the PR is merged, can I still use this change #86791 and export it to Android via aot? |
NativeAOT should not be affected by this PR. If your csproj contains Keep in mind NativeAOT for Android is still experimental upstream, and will likely require more changes to make it work properly but that's outside of the scope of this PR which only focuses on enabling the Mono runtime. You may also try to enable MonoAOT with |
Any chance we backport this to 4.3? Using native binding for handling http calls in android is a hassle |
I don't think so, this is a pretty big change to C# Android exports that builds on top of a number of other changes to the Android platform that I don't expect to see backported to 4.3 either. Issues with crypto functions not being available in 4.3 can be worked around by avoiding crypto APIs from the BCL or thirdparty C# libraries, and instead relying on the crypto APIs provided by Godot (i.e.: Use Alternatively, you can include the required native libraries ( |
Needs rebase after #96967. |
Some platforms don't support hostfxr but we can use the coreclr/monosgen library directly to initialize the runtime. Android exports now use the `android` runtime identifier instead of `linux-bionic`, this removes the restrictions we previously had: - Adds support for all Android architectures (arm32, arm64, x32, and x64), previously only the 64-bit architectures were supported. - Loads `System.Security.Cryptography.Native.Android` (the .NET library that binds to the Android OS crypto functions).
f78d359
to
0aa46e1
Compare
Thanks! Amazing work 🎉 |
Custom builds to test this PR can be downloaded from #88803 (comment)
Some platforms don't support hostfxr but we can use the coreclr/monosgen library directly to initialize the runtime.
Android exports now use the
android
runtime identifier instead oflinux-bionic
, this removes the restrictions we previously had:System.Security.Cryptography.Native.Android
(the .NET library that binds to the Android OS crypto functions).