[iOS] Fix dotnet export. #84945

bruvzg · 2023-11-15T17:30:03Z

Fixes xcframework build.
Fixes location of AOT files in the Xcode project.
Fixes embedding settings for frameworks.
Fixes exporting .xarchive and .ipa (with "Export Project Only" unchecked).

raulsntos

I can't test this since I don't have a macOS device, but the code looks good to me. Maybe @shana can also review this.

raulsntos · 2023-11-15T23:57:53Z

modules/mono/editor/GodotTools/GodotTools/Export/AotBuilder.cs

@@ -195,7 +195,7 @@ public static void CompileAssembliesForiOS(ExportPlugin exporter, bool isDebug,
                    bool isSim = arch == "i386" || arch == "x86_64"; // Shouldn't really happen as we don't do AOT for the simulator
                    string versionMinName = isSim ? "iphonesimulator" : "iphoneos";
                    string iOSPlatformName = isSim ? "iPhoneSimulator" : "iPhoneOS";
-                    const string versionMin = "10.0"; // TODO: Turn this hard-coded version into an exporter setting
+                    const string versionMin = "12.0"; // TODO: Turn this hard-coded version into an exporter setting


We're not using the AotBuilder class, it's a remnant from Godot 3.

We should probably remove unused classes (not in this PR), they can always be brought back from the Git history if needed in the future.

shana · 2023-11-16T12:20:47Z

modules/mono/editor/GodotTools/GodotTools/Export/ExportPlugin.cs

@@ -194,7 +194,7 @@ private void _ExportBeginImpl(string[] features, bool isDebug, string path, long
                    BundleOutputs = false,
                    IncludeDebugSymbols = publishConfig.IncludeDebugSymbols,
                    RidOS = OS.DotNetOS.iOSSimulator,
-                    UseTempDir = true,


I'm testing this right now, and the problem is not the temp dir output, the problem is that the xcframework command should not be including the library argument that's pointing to the temp dir. The simulator dylib should be already lipo'd into the non-simulator dylib, so there should be no references to the simulator dylib anymore. This might look like it fixes things, but it's not the correct fix here.

The simulator dylib should be already lipo'd into the non-simulator dylib

This is impossible, simulator lib can't be lipoed with non simulator lib since both contain arm64 code, that's the only season xcframeworks exist in a first place (merging libs for the different platform but the same architecture).

Sorry, I mispoke, the x64 and arm64 simulator dylibs are lipo'd together, and the lipo result is what we want, not the separate dylibs. This is not where the fix should be, the fix is where the lipo happens for iOS, where the output paths are adjusted for creating the framework.

This is the correct fix for the problem where the xcframework fails to be created properly: https://github.com/godotengine/godot/compare/master...shana:godot:fix-ios-dotnet-export?diff=unified

Could you add a comment on this line that this is a temporary fix with a link to this PR, so we don't forget to fix it up properly?

akien-mga · 2023-11-16T12:53:41Z

I made a test build from this PR to help testing, especially if we can find someone who still has Xcode < 15 to verify that makes things work for Xcode 15 didn't break assumptions for older versions:

https://downloads.tuxfamily.org/godotengine/testing/4.2-beta6-pr84945/

It's a build from latest master + this PR, it poses at 4.2-beta6 so that it reuses the nuget packages for it, those should be compatible I think.

bruvzg · 2023-11-16T12:58:56Z

I think I have also found the reason for another issue (#82729 (review)) which is preventing full archive/IPA export from the editor, and only allowing exporting Xcode project. The AOT xcframework have the same name as the main Godot library ({project_name}.xcframework) and it's causing some sort of conflict. Renaming it should fix it (and renaming just the lib will not affect .dylib name, so there's no need to change library loading code).

bruvzg · 2023-11-16T13:07:50Z

modules/mono/editor/GodotTools/GodotTools/Export/ExportPlugin.cs

@@ -361,7 +361,7 @@ private void _ExportBeginImpl(string[] features, bool isDebug, string path, long
                }

                var xcFrameworkPath = Path.Combine(GodotSharpDirs.ProjectBaseOutputPath, publishConfig.BuildConfig,
-                    $"{GodotSharpDirs.ProjectAssemblyName}.xcframework");
+                    $"{GodotSharpDirs.ProjectAssemblyName}_aot.xcframework");


For the reference, this is the only changed line in the last push. Added to fix name conflict (#82729 (review)) and should not have any effect on the rest of code.

I just ran into the same export problem with the naming conflict, it's an Xcode 15 thing, apparently. This fixes it 👍

shana · 2023-11-16T13:25:25Z

@bruvzg @akien-mga @raulsntos I pushed a more long term fix for the temp paths breaking the xcframework generation (https://github.com/godotengine/godot/compare/master...shana:godot:fix-ios-dotnet-export?diff=unified), but not using temp dirs for any of the iOS exports is a safe fix right now, even if it does generate more garbage, so 👍 for my side.

bruvzg · 2023-11-16T13:35:05Z

I pushed a more long term fix for the temp paths breaking the xcframework generation (https://github.com/godotengine/godot/compare/master...shana:godot:fix-ios-dotnet-export?diff=unified), but not using temp dirs for any of the iOS exports is a safe fix right now, even if it does generate more garbage, so 👍 for my side.

I can confirm this also works fine (in place of UseTempDir change, the rest of the changes are still relevant).

akien-mga · 2023-11-16T13:55:49Z

Thanks everyone for the quick turnaround!

bruvzg added bug platform:ios topic:editor topic:dotnet topic:export labels Nov 15, 2023

bruvzg added this to the 4.2 milestone Nov 15, 2023

bruvzg force-pushed the ios_dotnet_export_fix branch from baaa08f to b99b5c4 Compare November 15, 2023 21:39

bruvzg marked this pull request as ready for review November 15, 2023 21:40

bruvzg requested review from a team as code owners November 15, 2023 21:40

raulsntos approved these changes Nov 16, 2023

View reviewed changes

shana reviewed Nov 16, 2023

View reviewed changes

[iOS] Fix dotnet export.

a92511f

bruvzg force-pushed the ios_dotnet_export_fix branch from b99b5c4 to a92511f Compare November 16, 2023 13:06

bruvzg commented Nov 16, 2023

View reviewed changes

akien-mga merged commit 7e679ea into godotengine:master Nov 16, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[iOS] Fix dotnet export. #84945

[iOS] Fix dotnet export. #84945

bruvzg commented Nov 15, 2023 •

edited

Loading

raulsntos left a comment

raulsntos Nov 15, 2023

akien-mga Nov 16, 2023

shana Nov 16, 2023

bruvzg Nov 16, 2023 •

edited

Loading

shana Nov 16, 2023 •

edited

Loading

akien-mga commented Nov 16, 2023

bruvzg commented Nov 16, 2023

bruvzg Nov 16, 2023

shana Nov 16, 2023

shana commented Nov 16, 2023

bruvzg commented Nov 16, 2023

akien-mga commented Nov 16, 2023

[iOS] Fix dotnet export. #84945

[iOS] Fix dotnet export. #84945

Conversation

bruvzg commented Nov 15, 2023 • edited Loading

raulsntos left a comment

Choose a reason for hiding this comment

raulsntos Nov 15, 2023

Choose a reason for hiding this comment

akien-mga Nov 16, 2023

Choose a reason for hiding this comment

shana Nov 16, 2023

Choose a reason for hiding this comment

bruvzg Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

shana Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

akien-mga commented Nov 16, 2023

bruvzg commented Nov 16, 2023

bruvzg Nov 16, 2023

Choose a reason for hiding this comment

shana Nov 16, 2023

Choose a reason for hiding this comment

shana commented Nov 16, 2023

bruvzg commented Nov 16, 2023

akien-mga commented Nov 16, 2023

bruvzg commented Nov 15, 2023 •

edited

Loading

bruvzg Nov 16, 2023 •

edited

Loading

shana Nov 16, 2023 •

edited

Loading