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.

Sign up for GitHub

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

[iOS] Fix dotnet export. #84945

Merged
merged 1 commit into from
Nov 16, 2023
+21 −12
Merged

[iOS] Fix dotnet export. #84945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions modules/mono/editor/Godot.NET.Sdk/Godot.NET.Sdk/Sdk/iOSNativeAOT.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@
<Message Importance="normal" Text="Found XCode at $(XcodeSelect)" Condition=" '$(FindXCode)' == 'true' "/>

<ItemGroup>
<LinkerArg Include="-isysroot %22$(XCodePath)Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk%22"
<LinkerArg Include="-mios-simulator-version-min=12.0 -isysroot %22$(XCodePath)Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk%22"
Condition=" $(RuntimeIdentifier.Contains('simulator')) "/>
<LinkerArg Include="-isysroot %22$(XCodePath)Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk%22"
<LinkerArg Include="-miphoneos-version-min=12.0 -isysroot %22$(XCodePath)Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk%22"
Condition=" !$(RuntimeIdentifier.Contains('simulator')) "/>
</ItemGroup>

Expand Down
2 changes: 1 addition & 1 deletion modules/mono/editor/GodotTools/GodotTools/Export/AotBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

string iOSSdkPath = Path.Combine(XcodeHelper.XcodePath,
$"Contents/Developer/Platforms/{iOSPlatformName}.platform/Developer/SDKs/{iOSPlatformName}.sdk");

Expand Down
4 changes: 2 additions & 2 deletions modules/mono/editor/GodotTools/GodotTools/Export/ExportPlugin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@bruvzg bruvzg Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

@shana shana Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

UseTempDir = false,
});
}

Expand Down Expand Up @@ -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");
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (!BuildManager.GenerateXCFrameworkBlocking(outputPaths,
Path.Combine(GodotSharpDirs.ProjectBaseOutputPath, publishConfig.BuildConfig, xcFrameworkPath)))
{
Expand Down
2 changes: 2 additions & 0 deletions modules/mono/mono_gd/gd_mono.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ class GDMono {

String project_assembly_path;
uint64_t project_assembly_modified_time = 0;
#ifdef GD_MONO_HOT_RELOAD
int project_load_failure_count = 0;
#endif

#ifdef TOOLS_ENABLED
bool _load_project_assembly();
Expand Down
21 changes: 14 additions & 7 deletions platform/ios/export/export_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,14 +1077,15 @@ Error EditorExportPlatformIOS::_copy_asset(const String &p_out_dir, const String
return ERR_FILE_NOT_FOUND;
}

String base_dir = p_asset.get_base_dir().replace("res://", "");
String base_dir = p_asset.get_base_dir().replace("res://", "").replace(".godot/mono/temp/bin/", "");
String asset = p_asset.ends_with("/") ? p_asset.left(p_asset.length() - 1) : p_asset;
String destination_dir;
String destination;
String asset_path;

bool create_framework = false;

if (p_is_framework && p_asset.ends_with(".dylib")) {
if (p_is_framework && asset.ends_with(".dylib")) {
// For iOS we need to turn .dylib into .framework
// to be able to send application to AppStore
asset_path = String("dylibs").path_join(base_dir);
Expand All @@ -1103,7 +1104,7 @@ Error EditorExportPlatformIOS::_copy_asset(const String &p_out_dir, const String
destination_dir = p_out_dir.path_join(asset_path);
destination = destination_dir.path_join(file_name);
create_framework = true;
} else if (p_is_framework && (p_asset.ends_with(".framework") || p_asset.ends_with(".xcframework"))) {
} else if (p_is_framework && (asset.ends_with(".framework") || asset.ends_with(".xcframework"))) {
asset_path = String("dylibs").path_join(base_dir);

String file_name;
Expand Down Expand Up @@ -1147,6 +1148,9 @@ Error EditorExportPlatformIOS::_copy_asset(const String &p_out_dir, const String
if (err) {
return err;
}
if (asset_path.ends_with("/")) {
asset_path = asset_path.left(asset_path.length() - 1);
}
IOSExportAsset exported_asset = { binary_name.path_join(asset_path), p_is_framework, p_should_embed };
r_exported_assets.push_back(exported_asset);

Expand Down Expand Up @@ -1213,13 +1217,16 @@ Error EditorExportPlatformIOS::_copy_asset(const String &p_out_dir, const String
Error EditorExportPlatformIOS::_export_additional_assets(const String &p_out_dir, const Vector<String> &p_assets, bool p_is_framework, bool p_should_embed, Vector<IOSExportAsset> &r_exported_assets) {
for (int f_idx = 0; f_idx < p_assets.size(); ++f_idx) {
String asset = p_assets[f_idx];
if (!asset.begins_with("res://")) {
if (asset.begins_with("res://")) {
Error err = _copy_asset(p_out_dir, asset, nullptr, p_is_framework, p_should_embed, r_exported_assets);
ERR_FAIL_COND_V(err, err);
} else if (ProjectSettings::get_singleton()->localize_path(asset).begins_with("res://")) {
Error err = _copy_asset(p_out_dir, ProjectSettings::get_singleton()->localize_path(asset), nullptr, p_is_framework, p_should_embed, r_exported_assets);
ERR_FAIL_COND_V(err, err);
} else {
// either SDK-builtin or already a part of the export template
IOSExportAsset exported_asset = { asset, p_is_framework, p_should_embed };
r_exported_assets.push_back(exported_asset);
} else {
Error err = _copy_asset(p_out_dir, asset, nullptr, p_is_framework, p_should_embed, r_exported_assets);
ERR_FAIL_COND_V(err, err);
}
}

Expand Down