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

Abstract apple framework creation behind a tool. #1679

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Jan 2, 2025

The new tool will take library binaries, and merge them with lipo to create a valid .framework bundle.

The main reason to do this is to simplify .framework / .xcframework creation for godot-cpp users.
This may help alleviate adoption problems when releasing framework bundles for macOS. I personally have found this to be a bit confusing at first.
Also, currently, copies of Info.plist have to be copied around needlessly across build combinations, as each framework needs their own in appropriate spots. This PR removes this boilerplate by generating appropriate Info.plist.

Edit: Whoops, wrong branch. But no matter, the other PR had already been merged.

@Ivorforce Ivorforce requested review from a team as code owners January 2, 2025 02:29
@Ivorforce Ivorforce force-pushed the gh-action-setup-godot-cpp branch 4 times, most recently from 39ab389 to 7986baa Compare January 2, 2025 02:40
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

  • iOS portion is wrong.
  • It's not using lipo on macOS.
  • It's not necessary for macOS, only iOS is restricted to not use dylibs.

iOS xcframework should have structure like this:

ios-arm64			# folder with either .framework or static lib (.a) inside
ios-arm64_x86_64-simulator	# folder with either .framework or static lib (.a) inside
Info.plist			# list of target platforms

Info.plist is like:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
	<key>AvailableLibraries</key>
	<array>
		<dict>
			<key>BinaryPath</key>
			<string>__name__.framework/__name__</string>
			<key>LibraryIdentifier</key>
			<string>ios-arm64_x86_64-simulator</string>
			<key>LibraryPath</key>
			<string>__name__.framework</string>
			<key>SupportedArchitectures</key>
			<array>
				<string>arm64</string>
				<string>x86_64</string>
			</array>
			<key>SupportedPlatform</key>
			<string>ios</string>
			<key>SupportedPlatformVariant</key>
			<string>simulator</string>
		</dict>
		<dict>
			<key>BinaryPath</key>
			<string>__name__.framework/__name__</string>
			<key>LibraryIdentifier</key>
			<string>ios-arm64</string>
			<key>LibraryPath</key>
			<string>__name__.framework</string>
			<key>SupportedArchitectures</key>
			<array>
				<string>arm64</string>
			</array>
			<key>SupportedPlatform</key>
			<string>ios</string>
		</dict>
	</array>
	<key>CFBundlePackageType</key>
	<string>XFWK</string>
	<key>XCFrameworkFormatVersion</key>
	<string>1.0</string>
</dict>
</plist>

I would suggest doing framework generation as an extra step to allow combining all required versions with lipo and xcframework (see how generate_bundle is used in the Godot build).

tools/apple_framework.py Outdated Show resolved Hide resolved
tools/apple_framework.py Outdated Show resolved Hide resolved
tools/apple_framework.py Outdated Show resolved Hide resolved
test/SConstruct Outdated
env["platform"], env["target"], env["platform"], env["target"]
),
if env["platform"] == "macos" or env["platform"] == "ios":
# Static libraries (.dylib) are not supported on the iOS app store.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Static libraries (.dylib) are not supported on the iOS app store.
# Dynamic libraries (.dylib) are not supported on the iOS app store.

Static libraries (.a) are supported on iOS, as well as dynamic .frameworks and .xcframeworks with both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be good to document somewhere better than a single comment.

test/SConstruct Outdated
Comment on lines 27 to 28
if env["ios_simulator"]:
framework_name += ".simulator"
Copy link
Member

Choose a reason for hiding this comment

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

Why? The only purpose of .xcframework is to contain libs for multiple targets, so there should not be separate simulator xcframework.

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, the previous code confused me too, but I merely adapted what I found.

test/SConstruct Outdated
framework_name += ".simulator"

library_targets = framework_tool.generate(
f"project/bin/{framework_name}.xcframework",
Copy link
Member

Choose a reason for hiding this comment

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

This should be .framework for macOS, and .xcframework for iOS (it's a different format with different plist).

Copy link
Contributor Author

@Ivorforce Ivorforce Jan 4, 2025

Choose a reason for hiding this comment

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

I could be wrong, but a superficial search told me .framework is for single-platform libraries while .xcframework is for multi-platform libraries. Perhaps we can get away with .framework on both?
Edit: I suppose .xcframework was perhaps preferred to indulge the .simulator builds being part of the same framework?

@Ivorforce
Copy link
Contributor Author

It was definitely too late when I was working on this 😅
I've done a bit more research and can hopefully address some of your comments.

  • It's not using lipo on macOS.

This is indeed a problem I'd like to see addressed too.
I have previously opened #1633 to track this, but the short version is: godot-cpp currently builds with multi-target gcc options, and a lipo workflow is not yet supported.

  • It's not necessary for macOS, only iOS is restricted to not use dylibs.

That's true; there is a comment in the code that addresses this. Anyway it's not a change of behavior, building frameworks is what the demo does right now anyway (and other users can always decide to just not call the framework creation tool for macOS).

iOS xcframework should have structure like this:
[...]

Right, I have totally misunderstood .xcframework's purpose.
As far as I'm understanding, iOS should support a normal .framework structure too, while .xcframework was introduced as a newer format primarily for multi-platform libraries. I think it should be sufficient to stay with .framework for our purposes.

I would suggest doing framework generation as an extra step to allow combining all required versions with lipo and xcframework (see how generate_bundle is used in the Godot build).

I think that's a good idea! I'm happy to indulge once we have some sort of plan for integrating lipo.
This would be a good topic for the next GDExtension meeting, perhaps we'll see you there?

@bruvzg
Copy link
Member

bruvzg commented Jan 4, 2025

As far as I'm understanding, iOS should support a normal .framework structure too

Yes, but with normal .framework you can't have both device and simulator files at the same time, with .xcframework you can, Xcode will extract the correct framework when building the final app.

@Ivorforce Ivorforce force-pushed the gh-action-setup-godot-cpp branch 4 times, most recently from 1e9bd08 to d9693ec Compare January 8, 2025 12:26
@Ivorforce
Copy link
Contributor Author

Ivorforce commented Jan 8, 2025

I have refactored the PR to be a lot more versatile.

Info.plist is now handled by a shell script.
The tool itself takes static libraries as the source parameter, which is a lot more agnostic to how it should be called. This supports both the 'universal' target build structure as well as two separate build envs (although this approach may require a few refactors of godot-cpp to make it work).

The way test/SConscript is set up now actually generates both a .dylib and a .framework. Actual projects should probably be changed to generate the .dylib in a temporary folder, to avoid accidentally adding it to a release gdextension, but I can figure that out when I modify godot-cpp-template after this.

In either case, I'm a lot more happy with the structure now, and i'd be happy to receive some more feedback!

@Ivorforce Ivorforce force-pushed the gh-action-setup-godot-cpp branch 2 times, most recently from c0f8efc to 7137666 Compare January 8, 2025 13:35
…e input binaries together and create an appropriate `Info.plist`.
@Ivorforce Ivorforce force-pushed the gh-action-setup-godot-cpp branch from 7137666 to 3bfe5fd Compare January 8, 2025 14:29
@migueldeicaza
Copy link

The limitation on dynamic libraries on iOS was removed some time ago, it is now possible to use dynamic libraries in iOS.

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

Successfully merging this pull request may close these issues.

3 participants