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

Fix Android mono export with 2 or more cpu architectures fails #98066

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

TCROC
Copy link
Contributor

@TCROC TCROC commented Oct 10, 2024

Fixes: #98064
Bugsquad edit: Fixes #101948 for gradle builds.

When 2 or more architectures are configured for support in an android export, jars per architecture are attempted to be exported. This is due to mono / dotnet including a jar per cpu architecture. However, this causes problems because jars are cpu agnostic and result in conflicts during the export process. Here are some images illustrating the issue:

image

image

And then when exporting we get:

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':mergeExtDexMonoDebug'.
> A failure occurred while executing com.android.build.gradle.internal.tasks.DexMergingTaskDelegate
   > There was a failure while executing work items
      > A failure occurred while executing com.android.build.gradle.internal.tasks.DexMergingWorkAction
         > com.android.builder.dexing.DexArchiveMergerException: Error while merging dex archives: 
           Learn how to resolve the issue at https://developer.android.com/studio/build/dependencies#duplicate_classes.
           Type net.dot.android.crypto.DotnetProxyTrustManager is defined multiple times: /home/tcroc/dev/BlockyBallOT/blockyball-godot/android/app/build/build/intermediates/external_file_lib_dex_archives/monoDebug/0_jetified-libSystem.Security.Cryptography.Native.Android.jar:classes.dex, /home/tcroc/dev/BlockyBallOT/blockyball-godot/android/app/build/build/intermediates/external_file_lib_dex_archives/monoDebug/2_jetified-libSystem.Security.Cryptography.Native.Android.jar:classes.dex

To fix it, I added a HashSet to ExportPlugin.cs that makes sure multiple jars of the same name are not included in the export.

@TCROC TCROC requested a review from a team as a code owner October 10, 2024 16:06
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

It's fine for the exported APK to include the same .jar multiple times. As I understand it, .jar files can only be added to the APK when building the APK so the pre-built export template APK won't care about these .jar files that are added later, we already take care of adding the .jar file to the export template APK.

The problem you are having is likely exclusive to Gradle builds. The Gradle build likely includes these .jar files multiple times because ExportPlugin::add_shared_object adds the files to the android/build/libs directory and the Gradle build will take all the .jar files in there without de-duplicating them when building the APK.

Comment on lines 342 to 357
// Don't export the same jar twice. Otherwise we will have conflicts.
// This can happen when exporting for multiple architectures. Dotnet
// stores the jars in .godot/mono/temp/bin/Export[Debug|Release] per
// target architecture. Jars are cpu agnostic so only 1 is needed.
var jarName = Path.GetFileName(fileName);
if (exportedJars.Contains(jarName))
{
return false;
}
else
{
exportedJars.Add(jarName);
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

The comment is incorrect, there's no issue with having the same .jar file multiple times in the .godot directory. And returning false will still include the .jar file multiple times in the exported APK (just under a different directory).

Also, is the android/build/libs directory cleared for each export? Otherwise, I imagine this will still fail if you export with a different set of architectures at different times. For example, exporting only for the x86_64 architecture, then exporting only for the arm64 architecture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment is incorrect, there's no issue with having the same .jar file multiple times in the .godot directory.

Ah. I didn't mean to insinuate the issue presides in the .godot directory. I was trying to say that the issue resides with exporting multiple jars of the same name from the .godot directory. I will reword this.

And returning false will still include the .jar file multiple times in the exported APK (just under a different directory).

Ok so I tested this theory and learned something interesting. When returning false, it does indeed only include it once. Here are some screenshots showing it to be so:

x86_64 missing:

image

arm64 present:

image

Which now brings about the question: Why the conflict? They should be under different folders just as you said (when returning true). And this is what I found:

image

The error is occurring at the intermediates stage under external_file_lib_dex_archives/monoDebug.

Where is the logic that puts jars in the intermediates folder? I bet this is where we need to check for duplicates. NOT in the ExportPlugins.cs.

Also, is the android/build/libs directory cleared for each export? Otherwise, I imagine this will still fail if you export with a different set of architectures at different times. For example, exporting only for the x86_64 architecture, then exporting only for the arm64 architecture.

I do this manually in my build script / pipeline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here is the error confirming that it is at the intermediates folder:

Type net.dot.android.crypto.DotnetProxyTrustManager is defined multiple times: 
/home/tcroc/dev/BlockyBallOT/blockyball-godot/android/app/build/build/intermediates/external_file_lib_dex_archives/monoDebug/0_jetified-libSystem.Security.Cryptography.Native.Android.jar:classes.dex, 
/home/tcroc/dev/BlockyBallOT/blockyball-godot/android/app/build/build/intermediates/external_file_lib_dex_archives/monoDebug/2_jetified-libSystem.Security.Cryptography.Native.Android.jar:classes.dex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raulsntos I did a lot of research and tried many different things. Ultimately I almost had it right the first time. I had to move where it checked that the jar had already been exported so that the jar wasn't embedded inside the apk. I have since pushed the new code to this branch and updated the comment per your suggestion. Here is a summary of my findings:

  1. gradle does not distinguish between abis for .jar files the same way it does for .so files. It simply tries to compile any and all jars into the apk. So if there are 2 jars with the same classes for 2 different abis, only 1 can actually be included. Otherwise we will get conflicts.
  2. Microsoft distributes the jar in each nuget package for each abi. However, only one should be chosen. It does not matter which one we choose since they are identical. I verified this with android studio comparing the byte code:
    image
    As can be seen, the only difference is in the MSFTSIG.RSA. And this does not seem to have an effect on the built apk. I tested the universal apk on both an x86_64 emulator and an arm64 physical device. It ran fine on both and the https requests were successful.
  3. I don't think we need to distribute the https://github.com/godotengine/godot/blob/92e51fca7247c932f95a1662aefc28aca96e8de6/modules/mono/thirdparty/libSystem.Security.Cryptography.Native.Android.jar in the godot repo. This file is automatically pulled down in a nuget package. You can verify this by looking at your nuget cache:
    image
    • I believe the msbuild logs will also confirm that it gets pulled from there, but I'm struggling to find those logs on my system currently.
  4. Building on point 3, I don't believe we need these lines in our build.gradle for the same reason as point 3: https://github.com/godotengine/godot/blob/92e51fca7247c932f95a1662aefc28aca96e8de6/platform/android/java/app/build.gradle#L72C1-L76C6
  5. If you agree with points 3 and 4, I can put in a separate PR to clean that up after this PR goes through. I recommenced keeping this PR simple as it is a simple fix for a major issue. The more changes we make, the more discussion it might provoke and delay the PR from going through. Unless you disagree of course :)

Thanks again for the feedback! Do you have any other questions or anything else you would like me to change?

Copy link
Member

Choose a reason for hiding this comment

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

Gradle does not distinguish between abis for .jar files the same way it does for .so files. It simply tries to compile any and all jars into the apk. So if there are 2 jars with the same classes for 2 different abis, only 1 can actually be included. Otherwise we will get conflicts.

I'm not very familiar with the Android platform, but from what I understand, since the .jar is included multiples times under the res://android/build/libs directory (even if they are on different subdirectories), they all get included in the Gradle build because of this:

debugImplementation fileTree(dir: 'libs/debug', include: ['**/*.jar', '*.aar'])
devImplementation fileTree(dir: 'libs/dev', include: ['**/*.jar', '*.aar'])
releaseImplementation fileTree(dir: 'libs/release', include: ['**/*.jar', '*.aar'])

I don't know if there's a way to tell Gradle to de-duplicate or otherwise only include them once. If we have to do this ourselves in ExportPlugin.cs, then we should avoid placing the .jar under one of the architecture's subdirectories but I don't think EditorExportPlugin::add_shared_object allows this. Otherwise, we run into the issue I mentioned earlier about not clearing the res://android/build/libs directory after each build1.

I do this manually in my build script / pipeline.

I don't think it's acceptable for Godot to relay on users clearing this directory themselves, because most users won't know they may have to, and will be very confused.

Microsoft distributes the jar in each nuget package for each abi. However, only one should be chosen. It does not matter which one we choose since they are identical.

Is this documented somewhere? I'd like to have a guarantee that this won't change in a future version of .NET.

I don't think we need to distribute the libSystem.Security.Cryptography.Native.Android.jar in the godot repo. This file is automatically pulled down in a nuget package.

The .jar file is in the Godot repo for non-Gradle exports, so that it's included in the APK we build for the export templates. As far as I know, there's no way to add a .jar to a pre-built APK, but I'd love to be wrong, I don't like having the .jar in the Godot repo either.

Footnotes

  1. https://github.com/godotengine/godot/pull/98066#discussion_r1795854435

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not very familiar with the Android platform, but from what I understand, since the .jar is included multiples times under the res://android/build/libs directory (even if they are on different subdirectories), they all get included in the Gradle build because of this:

godot/platform/android/java/app/build.gradle

Lines 47 to 49 in 92e51fc
debugImplementation fileTree(dir: 'libs/debug', include: ['/.jar', '.aar'])
devImplementation fileTree(dir: 'libs/dev', include: ['
/.jar', '.aar'])
releaseImplementation fileTree(dir: 'libs/release', include: ['**/.jar', '.aar'])

Correct. That is what I have found to be the case after trying several different things over the last few days.

I don't know if there's a way to tell Gradle to de-duplicate or otherwise only include them once. If we have to do this ourselves in ExportPlugin.cs

I tried many different ways. Nothing seemed to work. Gradle always just includes the jar. I haven't been able to find any documentation from Google / Android on how to do this. Nor does anyone else seem to be trying to do this either. And when asking ChatGPT (yes I went this route) it tried its best but made note that "this is an unusual request" 😅. Ultimately to no avail.

then we should avoid placing the .jar under one of the architecture's subdirectories but I don't think EditorExportPlugin::add_shared_object allows this. Otherwise, we run into the issue I mentioned earlier about not clearing the res://android/build/libs directory after each build1.

Agreed. I can open up a separate issue for this if you want? That way this PR can go through with a simple fix and then an additional PR to clean up / improve. With this being one of those improvements.

I don't think it's acceptable for Godot to relay on users clearing this directory themselves, because most users won't know they may have to, and will be very confused.

100% agree

Is this documented somewhere? I'd like to have a guarantee that this won't change in a future version of .NET.

You can confirm yourself here at nuget: https://www.nuget.org/packages/Microsoft.NETCore.App.Runtime.Mono.android-arm64

Select "Open in Nuget Package Explorer"

image

Then runtime -> native -> libSystem.Security.Cryptography.Native.Android.jar

image

image

and then you can see where they compile it for the android runtime here: https://github.com/dotnet/runtime/blob/49006967613007f58daab31abab5316999aa7897/src/native/libs/System.Security.Cryptography.Native.Android/net/dot/android/crypto/DotnetProxyTrustManager.java

But to answer your question more explicitly:

Are there official Microsoft / dotnet / mono docs that say the libSystem.Security.Cryptography.Native.Android.jar will always be shipped with dotnet? Not that I have found.

BUT

All of the sources I linked above strongly suggest (at least to me) that this is how microsoft imlements cryptography for the Android runtime. I expect they will continue doing it this way until these dotnet runtime issues gets resolved:

dotnet/runtime#45741
dotnet/runtime#45737

Which looking at those, it appears they still have a ways to go.

The .jar file is in the Godot repo for non-Gradle exports, so that it's included in the APK we build for the export templates. As far as I know, there's no way to add a .jar to a pre-built APK, but I'd love to be wrong, I don't like having the .jar in the Godot repo either.

Since its included in the nuget package, we could just add some logic to fetch the nuget package and extract it from there. Then it wouldn't have to be included in the repo. This I would recommend be best fit for a separate PR so as to keep the complexity of this PR low and fix the more immediate issue of universal gradle builds failing.

Also to note: Fetching the jar from the nuget package rather than storing in the git repo should increase stability. Looking at the nuget versions: https://www.nuget.org/packages/Microsoft.NETCore.App.Runtime.Mono.android-arm64#versions-body-tab

It appears a new package gets shipped with each version of dotnet. Including patch releases.

Copy link
Member

Choose a reason for hiding this comment

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

Is this documented somewhere? I'd like to have a guarantee that this won't change in a future version of .NET.

You can confirm yourself here at nuget

But to answer your question more explicitly:

Are there official Microsoft / dotnet / mono docs that say the libSystem.Security.Cryptography.Native.Android.jar will always be shipped with dotnet? Not that I have found.

Cryptography is always done through OS functions in .NET because managed cryptography is not acceptable security. Therefore, there will always be a need to go through the JNI on Android to call those OS functions.

My question was more whether there's a guarantee that the .jar files for different architectures will always be the same or if they could be different at some point. I'm not really convinced we can just pick one at random and it will be fine.

Since its included in the nuget package, we could just add some logic to fetch the nuget package and extract it from there. Then it wouldn't have to be included in the repo.

I don't want to overcomplicate the process of building the templates, fetching from NuGet.org is not much different from doing it ourselves and including it in the repo like we do now.

Besides, at the moment of building the Android templates, we can't know what .NET version the user C# project will target. We already have to build two different flavors (standard and mono), I wouldn't want to add different mono templates for each .NET version.

For now, the .jar we have in the Godot repo is the one from .NET 8.0 and I hope that works well enough. But if the pre-built APK doesn't work, using Gradle builds should use the .jar from the dotnet publish output.

But I agree, we're getting a bit sidetracked with this, which is unrelated to the issue this PR is trying to fix. So we can continue this part of the conversation on the #dotnet channel if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My question was more whether there's a guarantee that the .jar files for different architectures will always be the same or if they could be different at some point. I'm not really convinced we can just pick one at random and it will be fine.

Ah I see what you are asking. Well I have not been able to find them commenting on this anywhere in an official capacity. BUT, in combination of deductive reasoning and inspecting their open source repos, I think we can come to a reasonable conclusion. Lets start on the deductive route:

image

In this screenshot, I have pulled 4 different nuget packages for 4 different Android abis / cpu architectures. As we can see, the only thing that differs between each jar is the MSFTSIG.RSA. If I were to guess, date / time is likely involved in this signature and that is what is different. I suspect the jar bytecode is identical. In fact, going back and enabling the "show equal files" setting in Android Studio confirms this:

image

Here we can see the compiled DotnetProxyTrustManager.class is identical between each jar. So at the very least dotnet version 8.0.10 has the same jar for each nuget package in regards to the java bytecode. Grabbing one at random should be fine for this version of dotnet. Lets go inspect their source code and see if they have any pipelines to confirm this for future dotnet versions.

Searching libSystem.Security.Cryptography.Native.Android within the dotnetruntime repo here: https://github.com/dotnet/runtime

We get 4 results. One of which appears to be a builder task! This is a great sign that there is only one! :)

image

Now lets go inspect this task and see what we find. I suspect we are going to find that it is the same for each abi, but lets confirm :)

It appears to be so at first glance! :) Here is where the java code itself is compiled:

https://github.com/dotnet/runtime/blob/main/src/tasks/AndroidAppBuilder/AndroidLibBuilderTask.cs

Here is where the Android sdk and toolsets are configured:

https://github.com/dotnet/runtime/blob/main/src/tasks/AndroidAppBuilder/ApkBuilder.cs

Which if you notice are mostly from environment variables. So if we search for those environment variables, we should see the environment variables being set to the same value for compiling or different values:

https://github.com/search?q=repo%3Adotnet%2Fruntime%20ANDROID_SDK_ROOT&type=code

Looking at the search results, there does not seem to me any discernable differences that would cause any issues! :) So I'm reasonably confident from all of these inspections that the jars are the same in each nuget package and will likely remain the same for each nuget package per dotnet version going forward for the foreseeable future.

To be entirely fair, I have not found where the nuget package is compiled for the specific android abis. BUT, searching for the key terms that should indicate differences seems to indicate the jar is compiled the same way throughout the dotnet runtime repo.

I hope this was helpful! :)

But I agree, we're getting a bit sidetracked with this, which is unrelated to the issue this PR is trying to fix. So we can continue this part of the conversation on the #dotnet channel if you want.

Sounds good! I'll ping you over there to discuss the additional topics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this one as well. Should I mark it as resolved?

@raulsntos raulsntos added this to the 4.x milestone Oct 10, 2024
@TCROC TCROC force-pushed the fix-android-mono-export branch from 07a4139 to 4217a10 Compare October 12, 2024 19:04
@TCROC TCROC requested a review from a team as a code owner October 12, 2024 19:04
@TCROC
Copy link
Contributor Author

TCROC commented Nov 17, 2024

Hey @raulsntos. Can we add this PR to the 4.4 milestone? Otherwise mono + gradle builds will no longer be possible.

@TCROC
Copy link
Contributor Author

TCROC commented Nov 18, 2024

@clayjohn Just pinging you to make sure this doesn't get forgotten for 4.4 :). Its in my fork and its such a small change that I won't have a problem maintaining it. But it will prevent other devs using mono + dotnet from being able to use android gradle and aab exports. Which is commonly used for distribution on Google Play so that FAT apk's aren't needed

@clayjohn
Copy link
Member

CC @raulsntos to comment on the Csharp stuff and @m4gr3d to comment on the gradle stuff

@clayjohn clayjohn requested review from raulsntos and m4gr3d November 18, 2024 23:06
@TCROC
Copy link
Contributor Author

TCROC commented Nov 23, 2024

Hey @clayjohn :) Can we add this to the 4.4 milestone? This way it doesn't get forgotten before 4.4 release. Even if it does get rejected in favor of something else. It came up on my mind again today and I don't want to get caught up in my other work + lose track of this and then 4.4 ship with Android + mono exports being broken for ".aab" gradle users. Which is likely most Google Play Store devs.

Comment on lines 331 to 348
if (fileName.EndsWith(".jar"))
{
// Don't export the same dotnet jar twice. Otherwise we will have conflicts. This
// can happen when exporting for multiple architectures. The jars exported from
// godot for dotnet can be found in .godot/mono/temp/bin/Export[Debug|Release].
// If you compare the bytecode with AndroidStudio or IntelliJ, you will find that
// they are identical. Hence why we can only export one.
//
// Gradle does not distinguish jars between abi folders like it does with .so files. It will
// try to compile both into the application if they are both exported.
if (exportedJars.Contains(fileName))
{
return;
}

exportedJars.Add(fileName);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@raulsntos As mentioned in RC, I think we should exclude the jar file from being exported via add_shared_object.
Instead, we should create a monoLibs folder under platform/android/java/app/, move libSystem.Security.Cryptography.Native.Android.jar there, and update the build.gradle dependency to match.

This will slightly increase the size of the standard gradle build template, but I think it's a tradeoff worth having, as it'll allow us to use the same gradle build template for both standard and mono builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m4gr3d or @raulsntos Do either of you know how to implement this? I personally do not yet see the vision in order to implement it. Better is better and I like better. I'm just missing some context on how to change this PR to match your suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

What @m4gr3d is saying is that, instead of taking the jar file from the .NET build, we can use the one included in the Godot repo here: modules/mono/thirdparty/libSystem.Security.Cryptography.Native.Android.jar.

You would move that to the suggested directory: platform/android/java/app/monoLibs.

Then, update the build.gradle to point to that path. This is the part that needs to be changed:

// .NET dependencies
String jar = '../../../../modules/mono/thirdparty/libSystem.Security.Cryptography.Native.Android.jar'
if (file(jar).exists()) {
monoImplementation files(jar)
}

You'd change the path to be relative to the platform/android/java/app directory, so it works in user Gradle builds, so it'd be: monoLibs/libSystem.Security.Cryptography.Native.Android.jar. And we don't need to check if the jar exists anymore because now it's guaranteed to always be there1.

Lastly, change ExportPlugin.cs to exclude every jar files, since the required jar will already be included in the Gradle template instead.

Personally, I'm a bit worried that this could break if a future .NET version changes the jar since, with this change, we'll always be including the jar from .NET 8 and may result in broken builds if the user project targets a different version that requires a different jar. But at least it should fix Gradle builds for 4.4, and we can always improve it later if needed.

Footnotes

  1. This check was added originally because we wanted Gradle builds to pull the jar from the user project build, instead of including it in the template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raulsntos brings up a good point regarding targeting different .net versions and future breakage. Is there any reason why we wouldn't want to do it the way it is currently set up in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raulsntos @m4gr3d just nudging again on what you guys think of @raulsntos concern? I think they bring up a good point that it may be more stable to use the .net version that microsoft distributes rather than maintaining our own copy. So that we maintain stability with differing .NET versions. From my understanding, Godot actually recommends targeting different versions of .NET depending on the platform. I personally target net8.0 universally. But I believe Godot reccomends different versions.

https://docs.godotengine.org/en/stable/contributing/development/compiling/compiling_with_dotnet.html

Found the docs. Looks like Godot reccomends 8.0 across the board now which is nice! :) But still, what version does the current jar target? And there is a very real possibility as @raulsntos pointed out, different major versions may break. Even patch versions could. Fortunately it does not seem to change per target architecture. Just per version.

I'm personally in favor of keeping it the way this PR does it. But I'll change it (or at least add a boolean for users to decide) if anyone of you prefers I do that instead.

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.godotengine.org/en/stable/contributing/development/compiling/compiling_with_dotnet.html

Note that this is the documentation for building the engine. For users we recommend to always use the latest SDK version, but you may still target a different version in your csproj. For example, you can use the .NET 9 SDK to build a C# project that targets .NET 8.

We'll also bump the GodotSharp target to net8.0 in 4.4 which means user projects must target at least .NET 8, but they can always target a newer version (e.g.: .NET 9).

But still, what version does the current jar target?

It was retrieved from a .NET 8 build, if I remember correctly.

I'm personally in favor of keeping it the way this PR does it. But I'll change it (or at least add a boolean for users to decide) if anyone of you prefers I do that instead.

Since we're getting close to the 4.4 release, I think it'd be better to change this PR to bundle the .jar as described. I think that'd be more likely to get merged quickly, so we can make sure Gradle builds are not broken when 4.4 is released. Then, we can improve upon it in a separate PR, without the pressure of the 4.4 release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Here is my suggested plan of action:

  1. I will open a new issue regarding retrieving updates to Microsoft distributed jars for .NET. This PR will solve that issue so I will leave this PR here and open. Even if we come up with a different approach, this PR can serve as an option for 4.5 until we come up with something better.

  2. We add Android mono export with 2 or more cpu architectures fails #98064 to the 4.4 milestone as this issue is a regression and will break android devs. I do not think we should ship 4.4 with this issue still open.

  3. We create a new PR with @m4gr3d 's suggestion above. I personally cannot commit to doing that PR at this very moment. My immediate efforts are directed at getting our game ready to start early testing with our users this late fall / early winter.

What do you think? And is this a PR @m4gr3d or someone else would be able to do in the mean time? If my time does free up before someone else puts in the PR, I'll gladly do it myself. But I don't foresee that happening before the 4.4 release.

Copy link
Member

@raulsntos raulsntos Dec 13, 2024

Choose a reason for hiding this comment

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

Sounds good. I opened a PR then #100351. Good luck with your game release!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m4gr3d @raulsntos should I mark this as resolved?

@m4gr3d
Copy link
Contributor

m4gr3d commented Jan 21, 2025

@raulsntos Can we close this PR since #100351 was merged?

@TCROC
Copy link
Contributor Author

TCROC commented Jan 21, 2025

@raulsntos Can we close this PR since #100351 was merged?

@raulsntos

Its up to you and the other godot devs. There is technically still an issue that this PR resolves that the other PR does not.

The issue being keeping an up to date jar for android. And especially jars for each .NET version. The current approach requires using the one manually stored in this GitHub repo. I haven't tested to confirm, but my guess is this likely will not be compatible between dotnet versions and will require regular updating. Or keeping one stored for each dotnet version godot wants to support. I still recommend leaving this PR open until either this PR goes through or a different PR goes through similar to this PR that fixes the remaining issue regarding .NET version compatibility.

@TCROC
Copy link
Contributor Author

TCROC commented Jan 21, 2025

This PR uses the jar microsoft provides in the nuget package. So its automatically pulled from official Microsoft feeds. Granted this PR can likely be improved by making sure it grabs the jar associated with the android target. I don't suspect that would be too hard to add support for. I just personally don't have time to make changes to this PR as we are getting our game ready for release. But this PR accomplishes everything except ensuring which jar is chosen based on target abi.

BUT

That may even be a non issue based on my research here: #98066 (comment)

^ And if that research holds true, this PR is good to go and should go through and override the other PR.

At the end of the day, our fork is working so we are happy. Whatever you guys decide :). If maintaining our fork becomes too challenging I'll bring more attention to this.

@raulsntos
Copy link
Member

@m4gr3d Looks like the .NET 8 jar we're vendoring of System.Security.Cryptography.Native.Android is incompatible with .NET 9 (see #101948 (comment)). This PR would fix that issue for Gradle builds (non-Gradle builds would still be affected though).

@m4gr3d
Copy link
Contributor

m4gr3d commented Jan 27, 2025

@m4gr3d Looks like the .NET 8 jar we're vendoring of System.Security.Cryptography.Native.Android is incompatible with .NET 9 (see #101948 (comment)). This PR would fix that issue for Gradle builds (non-Gradle builds would still be affected though).

Thanks for the context!
We should go ahead with this approach then to solve the issue for Gradle builds. This would also require that we omit the jar file included in the monoLibs directory when we generate the gradle templates to avoid conflict with the retrieved jar file.

@raulsntos For the non-gradle builds, can we detect the version of .NET being used and show a warning?

@TCROC
Copy link
Contributor Author

TCROC commented Jan 30, 2025

@raulsntos @m4gr3d I'm glad to hear you guys are leaning towards this approach! :) We recently finished out load tests so I do have a little bit of capacity now to make some changes to this PR if you want anything changed in order to push it through.

@TCROC
Copy link
Contributor Author

TCROC commented Feb 4, 2025

hey @m4gr3d @raulsntos Just checking in again to see if there's any adjustments you want me to make on this pr so we can close the issue it will fix :)

@raulsntos
Copy link
Member

For the non-gradle builds, can we detect the version of .NET being used and show a warning?

Yes, since we now require at least .NET 8 we can use CLI-based project evaluation.

dotnet build MyProject.csproj --getProperty:TargetFramework

@TCROC TCROC force-pushed the fix-android-mono-export branch from 6cce403 to 4151bf8 Compare February 5, 2025 01:04
@TCROC
Copy link
Contributor Author

TCROC commented Feb 5, 2025

For the non-gradle builds, can we detect the version of .NET being used and show a warning?

Yes, since we now require at least .NET 8 we can use CLI-based project evaluation.

dotnet build MyProject.csproj --getProperty:TargetFramework

Is this something you would like me to do in this PR? If so, how do I go about doing this?

And another question while I'm at it, do we need to make any changes now that the other PR that handles jars is in? Will this PR's logic conflict with that PR's logic during the export process? PR in question here: #100351

@raulsntos
Copy link
Member

do we need to make any changes now that the other PR that handles jars is in?

We'll want to revert the changes to build.gradle, so the jar is only include in the export templates but not in gradle builds.

dotnet build MyProject.csproj --getProperty:TargetFramework

Is this something you would like me to do in this PR? If so, how do I go about doing this?

I think it can be done in a follow-up PR, but feel free to do it here if you want. Here's how you could execute the command:

String pipe;
List<String> args;
args.push_back("build");
args.push_back(project_path);
args.push_back("--getProperty:TargetFramework");

int exitcode;
Error err = OS::get_singleton()->execute("dotnet", args, &pipe, &exitcode, true);

if (err != OK) {
    WARN_PRINT("Failed to execute dotnet command. Error " + error_names[err]);
} else if(exitcode != 0) {
    print_line(pipe);
    WARN_PRINT("dotnet command exited with code " + itos(exitcode) + ". See output above for more details.");
} else {
    String tfm = pipe.strip_edges();
    if (tfm != "net8.0") {
        err += TTR("Project targets '" + tfm + "' but the export template only supports 'net8.0'. Consider using gradle builds instead.");
    }
}

And you'd want to execute this somewhere around here:

#ifdef MODULE_MONO_ENABLED
// Android export is still a work in progress, keep a message as a warning.
err += TTR("Exporting to Android when using C#/.NET is experimental.") + "\n";
#endif

@TCROC
Copy link
Contributor Author

TCROC commented Feb 6, 2025

do we need to make any changes now that the other PR that handles jars is in?

We'll want to revert the changes to build.gradle, so the jar is only include in the export templates but not in gradle builds.

Do you simply want me to revert this build.gradle back to what it was before?

https://github.com/godotengine/godot/pull/100351/files#diff-81aadb220697f9a3dac1980c60648954b1982b9c19cd3629a57c0508dd5b3653

@raulsntos
Copy link
Member

Yes, and move the .jar file back to /modules/mono/thirdparty.

@TCROC TCROC force-pushed the fix-android-mono-export branch from 4151bf8 to 62f4238 Compare February 6, 2025 21:30
@TCROC TCROC requested a review from a team as a code owner February 6, 2025 21:30
@TCROC
Copy link
Contributor Author

TCROC commented Feb 7, 2025

Yes, and move the .jar file back to /modules/mono/thirdparty.

Done 😎. I'll leave your other suggestion about a warning for a follow up PR from someone who's more familiar on how to implement it.

Would you like me to make any other changes to this PR?

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me for the C# side.

I also went ahead and implemented the TFM validation in #102627.

@akien-mga akien-mga requested a review from m4gr3d February 9, 2025 19:20
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Feb 9, 2025
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.

One small cleanup remaining and the PR is good to go!

@TCROC
Copy link
Contributor Author

TCROC commented Feb 10, 2025

One small cleanup remaining and the PR is good to go!

Perfect! I'll clean this up tomorrow! :)

@TCROC TCROC force-pushed the fix-android-mono-export branch from 62f4238 to 5e2fd7b Compare February 10, 2025 16:40
@Repiteo Repiteo merged commit c90fd7f into godotengine:master Feb 10, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 10, 2025

Thanks!

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