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

macOS and iOS arch build behavior does not match Godot upstream #1633

Open
Ivorforce opened this issue Oct 29, 2024 · 9 comments
Open

macOS and iOS arch build behavior does not match Godot upstream #1633

Ivorforce opened this issue Oct 29, 2024 · 9 comments
Labels
enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Milestone

Comments

@Ivorforce
Copy link
Contributor

Ivorforce commented Oct 29, 2024

We recently found in a discussion that godot-cpp arch build behavior does not match Godot upstream.

In particular, godot-cpp defaults to building universal binaries (which passes the appropriate flags to the compiler to build both). The gdextension usually loads the same binary for both arm64 and x86_64 (as per godot-cpp-template).

Godot, on the other hand, builds arm64 and x86_64 separately and joins them afterwards with lipo. It does not support a 'universal' arch target.

One problem with the universal approach used by godot-cpp is that for universal builds, no architecture-specific build flags can be passed to the compilers (such as -mavx), because then the other architecture won't build (e.g. -mavx is not arm64 compatible). Notably, godot-cpp already supports compiling for the arches separately.

Changing the default behavior would affect all current macOS compatible godot-cpp extensions, because they will (likely) build with the default universal target right now (as per godot-cpp-template). We will have to figure out whether a change to separate the binaries will affect exports, especially for universal macOS apps.

We discussed this briefly at the last GDExtension meeting, and would like to have input on this before proceeding (especially from @Faless).

@Ivorforce Ivorforce changed the title macOS arch build behavior does not match Godot upstream macOS and iOS arch build behavior does not match Godot upstream Oct 29, 2024
@dsnopek dsnopek added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup labels Oct 29, 2024
@dsnopek dsnopek added this to the 4.x milestone Oct 29, 2024
@Ivorforce
Copy link
Contributor Author

Ivorforce commented Nov 3, 2024

I just tested exports on macOS.

When using feature tags, in-editor it tries to load the binary for the matching architecture (i.e. x86_64 or arm64, but not universal). When exporting, it tries to add the binary for the universal architecture, but not the arch-specific ones (x86_64 or arm64). (Edit: I opened a proposal for this issue)

This means that if we want to move to building the arches separately, we need to either patch godot-upstream to handle arch-separate binary exports, or lipo the binaries together to a universal as part of the build process.

@Faless
Copy link
Contributor

Faless commented Nov 4, 2024

I think universal builds will be completely deprecated soon as apple stop supporting intel Mac.

As far as I understand modern "frameworks" format should support using different libraries for different architectures, and Apple is trying really hard to force everyone to use frameworks for everything.

I don't think it's worth doing the lipo part in the godot-cpp build system because that won't include any third party dependency (which the user would likely have to lipo manually like we do for webrtc-native).

In the end, I think the solution is to start defaulting to arm64 (or auto-detect if building from macOS directly), and update the examples to use a multi-arch framework (we should confirm this is actually doable, I don't have a Mac, and I'm not an Apple fan as you've probably guessed, so I can't really verify any of this as their documentation is close to non-existent).

@Ivorforce
Copy link
Contributor Author

I think universal builds will be completely deprecated soon as apple stop supporting intel Mac.

Deprecated perhaps, but while Apple's support is a decent guideline for what Godot should support, there are still a lot of intel macs in circulation (and will be for a while). (speaking of which, this may be a good question to add for the next Godot user survey...)

I don't think it's worth doing the lipo part in the godot-cpp build system because that won't include any third party dependency (which the user would likely have to lipo manually like we do for webrtc-native).
In the end, I think the solution is to start defaulting to arm64 (or auto-detect if building from macOS directly), and update the examples to use a multi-arch framework (we should confirm this is actually doable, I don't have a Mac, and I'm not an Apple fan as you've probably guessed, so I can't really verify any of this as their documentation is close to non-existent).

If I understand correctly, you're hoping we can find a way to bundle the frameworks with separate binaries for separate arches? I'm not sure that's possible, but I'm also not deeply familiar with framework bundles in the first place. Apple seems to prefer creating universal bundles.

I don't see anything wrong with doing a lipo step in the GitHub runner to create a universal arch from both separate arches. But if you have any reservations against this approach please share them, as I'm not sure I wholly understood the point you were making. But either way, we seem to agree that building x86_64 and arm64 separately is preferable, correct?

@Faless
Copy link
Contributor

Faless commented Nov 5, 2024

I don't see anything wrong with doing a lipo step in the GitHub runner to create a universal arch from both separate arches.

I'm not sure how that would help, the GH artifacts are not supposed to be used by extension developers, but yeah, we can totally add the lipo step there.

But either way, we seem to agree that building x86_64 and arm64 separately is preferable, correct?

Yeah, I think that's fine.

We don't have to necessarily remove the "universal" option, we can just change the default (but I'm not against removal)

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Nov 5, 2024

I'm not sure how that would help, the GH artifacts are not supposed to be used by extension developers, but yeah, we can totally add the lipo step there.

Ah, i'm just proposing to do that for releases (ie. The GitHub runner). For local builds non-universal binaries are of course totally fine.

And yeah i agree there's no reason to remove the universal build option!

I'm happy to take on the next steps. Building arches on the GitHub runner separately can be done now (edit: actually godotengine/godot-cpp-template#55 should be addressed first). Changing the default to use the local arch should probably wait until godotengine/godot#98809 is merged, just to avoid somebody accidentally creating a non exportable build.

@Faless
Copy link
Contributor

Faless commented Nov 5, 2024

Ah, i'm just proposing to do that for releases (ie. The GitHub runner).

Again, not sure why you call them releases, the artifacts are not released in any way.

@Ivorforce
Copy link
Contributor Author

I guess that's correct, though I'm not sure what else the artifacts would be used for? I use the github runner from godot-cpp-template to create binaries for my GDExtension, which I can publish as a release. Does that clear it up?

@Faless
Copy link
Contributor

Faless commented Nov 5, 2024

I guess that's correct, though I'm not sure what else the artifacts would be used for?

Mostly for debugging CI issues themselves.

I use the github runner from godot-cpp-template to create binaries for my GDExtension, which I can publish as a release. Does that clear it up?

Yeah, I think it makes sense to have that for the godot-cpp-template since I assume people will rely on the CI file from that repository as a template for CI builds.

@enetheru
Copy link
Contributor

Let me know if you need help on the CMake side of things, I can help mirror whatever the process outcome is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

No branches or pull requests

4 participants