Fix the builds on macOS (universal). #26

Ivorforce · 2024-10-24T18:09:00Z

A few comments:

`std::quick_exit`

std::quick_exit is not supported on macOS. For the replacement call I referenced this change:
wesnoth/wesnoth@6ce218f#diff-fc3beea58bc7268f70c24a27ee7f556f6440c92ebb72d7db4ff321b82cec4df4R182

(Oh, i didn't even realize it was from wesnoth. Nice!)

libs

The line (v.removeprefix('-l') for v in value.split()) failed for the following error:

❯ scons
scons: Reading SConscript files ...
Auto-detected 16 CPU cores available for build parallelism. Using 15 cores by default. You can override it with the -j argument.
Building for architecture universal on platform macos
scons: done reading SConscript files.
scons: Building targets ...
clang++ -o bin/macos-universal/libgodot-python.macos.universal.dylib -arch x86_64 -arch arm64 -framework Cocoa -Wl,-undefined,dynamic_lookup -fvisibility=hidden -Wl,-S -Wl,-x -Wl,-dead_strip -Wl,-S -Wl,-x -Wl,-dead_strip -fvisibility=hidden -ldl -framework CoreFoundation -shared src/extension/.extension.os src/module/.class_creation_info.os src/module/.class_method_info.os src/module/.module.os src/module/.property_info.os src/module/.property_list.os src/module/.script_instance_info.os src/util/.exceptions.os src/util/.python_utils.os src/util/.system.os src/variant/.callable.os src/variant/.object.os src/variant/.string.os src/variant/.string_name.os src/variant/.variant.os src/.generated/.godot_module_archive.os -Lsrc/.generated/python/macos-universal/python/lib -ldl -l-framework -lCoreFoundation -lpython3.12
ld: warning: ignoring duplicate libraries: '-ldl'
ld: library '-framework' not found
clang: error: linker command failed with exit code 1 (use -v to see invocation)
scons: *** [bin/macos-universal/libgodot-python.macos.universal.dylib] Error 1
scons: building terminated because of errors.

I printed print(config_vars.link_libs) to get the following result:

['dl', '-framework', 'CoreFoundation', 'python3.12']

CoreFoundation is technically a library, but it doesn't compile with it:

clang++ -o bin/macos-universal/libgodot-python.macos.universal.dylib -arch x86_64 -arch arm64 -framework Cocoa -Wl,-undefined,dynamic_lookup -fvisibility=hidden -Wl,-S -Wl,-x -Wl,-dead_strip -Wl,-S -Wl,-x -Wl,-dead_strip -fvisibility=hidden -ldl -framework CoreFoundation -shared src/extension/.extension.os src/module/.class_creation_info.os src/module/.class_method_info.os src/module/.module.os src/module/.property_info.os src/module/.property_list.os src/module/.script_instance_info.os src/util/.exceptions.os src/util/.python_utils.os src/util/.system.os src/variant/.callable.os src/variant/.object.os src/variant/.string.os src/variant/.string_name.os src/variant/.variant.os src/.generated/.godot_module_archive.os -Lsrc/.generated/python/macos-universal/python/lib -lCoreFoundation
ld: library 'CoreFoundation' not found

Since it compiles fine without it, I conclude none of my items are libraries. I'm sure there's a reason the line is there, so I opted to just include the libraries if they actually do start with -l (even if there aren't any that do on my end).

strip -s

strip -s is not supported as that in macOS. But to strip dylibs, -x is needed. I opted to just separate the 2 lines, it's likely more differences may emerge in the future.

dylib vs framework

On macOS, both dylibs and frameworks are supported. On iOS, only .framework is supported. .framework and .dylib have the same binary but slightly different directory structures. The reason godot-cpp defaults to .framework is for consistency to iOS, as well as for signing the binary later. This is not important for now since the python extension supports neither iOS nor signing currently. Besides, godot's loading of .framework remains spotty, and I don't want to fiddle with SConstruct too much for now. We can switch over later .framework later when it becomes relevant.

More work

The current build still has 3 warnings:

ld: warning: ignoring duplicate libraries: '-ldl'
ld: warning: ignoring duplicate libraries: '-ldl'
ld: warning: ignoring file '/Users/lukas/dev/godot/godot-python-extension/src/.generated/python/macos-universal/python/lib/libpython3.12.dylib': found architecture 'x86_64', required architecture 'arm64'

At least the final warning is probably relevant for arm macs, but we can address it later. For now, libgodot-python.macos.universal.dylib is built, and that's better than before.

I have not yet tested the library in action.

Edit: I have now, it works.

Ivorforce · 2024-10-24T23:22:36Z

Looks like the macOS build is passing, though Windows is failing. Curious, as I didn't touch the Windows builds. Maybe it's affected by the if v.startswith('-l') change? Or somehow something else broke with a windows-latest update.

src/util/system.cpp

maiself · 2024-10-24T23:22:31Z

test/python.gdextension

@@ -5,7 +5,7 @@ entry_symbol = "python_extension_init"

 [libraries]

-macos = "res://bin/macos/libgodot-python.macos.framework"
+macos = "res://bin/macos/libgodot-python.macos.universal.dylib"


I don't know the difference here, but maybe there's a tag to be added after macos so that both can be listed?

The difference between the dylib is explained in my PR text. TL;DR dylib and framework use the same binary but with a slightly different folder structure. Both cannot be supported since the gdextension must choose one path for the tags at hand.

tools/build/python_config.py

maiself · 2024-10-24T23:46:00Z

Looks like the macOS build is passing, though Windows is failing. Curious, as I didn't touch the Windows builds. Maybe it's affected by the if v.startswith('-l') change? Or somehow something else broke with a windows-latest update.

I don't think anything you did broke anything, my guess is windows-latest changed somehow. The compiler output doesn't really help...

Ivorforce · 2024-10-24T23:55:53Z

I do have one tip for you.

Windows builds were very stubborn in my own project, until I adopted include-what-you-use for my includes. Being verbose with includes stabilizes include order across systems and was the final thing I needed to compile on all systems reliably. Since Windows is failing around a reference to GDExtensionTypePtr, included from another file, I can imagine it may help here too.

Ivorforce · 2024-10-25T11:52:59Z

This PR includes #29 now so that the runners can pass.

Ivorforce · 2024-10-25T15:19:33Z

I managed to run Godot and execute some python code! 🥳

The binary needs to be patched after build like so:

scons
install_name_tool -add_rpath @loader_path bin/macos-universal/libgodot-python.macos.universal.dylib
install_name_tool -change /install/lib/libpython3.12.dylib @rpath/libpython3.12.dylib bin/macos-universal/libgodot-python.macos.universal.dylib

I added that to SConstruct to be performed automatically. It's probably not the cleanest solution to do it as a post-action (the expected path may be changeable elsehow), but it works.

I get the icons and can move them with the arrow keys:

fire · 2024-10-25T15:50:49Z

@maiself Are you following the godot engine policy of 1 commit squashed pull request?

maiself · 2024-10-25T16:58:44Z

@fire Out of habit I've been following Blender's policy of a linear rebased history.

fire · 2024-10-25T17:05:59Z

My personal preference is to have a merge commit per pull request. I am not picky about single commit or sequence of commits, but it helps me a lot to have the merge commit.

maiself · 2024-10-25T17:17:57Z

I'm the opposite, I've always found it easier if merge commits are avoided so that the history is easier to navigate, bisection is easier, rebasing if needed is easier etc. Can you give insight into how having merge commits helps you?

Ivorforce · 2024-10-26T12:56:59Z

I just realized the 'universal' macOS build was a lie: The libpython is single-arch, so we cannot have a proper universal build. I updated everything to match. aarch64 builds should work fine though when we add a target for it.

Besides, Godot also uses two separate builds for universal, joining them after the fact. Godot-CPP is inconsistent with Godot upstream in this right now (see: godotengine/godot-cpp#1613).

Ivorforce · 2024-10-26T13:18:20Z

tools/build/prepare_python.py

+	if platform == 'macos':
+		# Rename the library id (which we depend on) to be in @rpath.
+		# (it defaults to /install/lib/)
+		subprocess.run(['install_name_tool', '-id', f'@rpath/{lib_filename}', src_lib_path], check=True)


This should maybe be performed on the copy instead, but that doesn't work because the copy isn't what is being linked, rendering the id change useless, and the final binary links to /install/lib/.

…64). Closes maiself#11.

maiself · 2024-10-28T01:19:00Z

Horray! Thanks a bunch for the help!

Ivorforce mentioned this pull request Oct 24, 2024

Add a github runner for macOS builds. #27

Merged

Ivorforce force-pushed the macos-build-config branch 3 times, most recently from 09bc08b to b4123c2 Compare October 24, 2024 18:23

maiself added bug Something isn't working build related to the build system labels Oct 24, 2024

maiself reviewed Oct 24, 2024

View reviewed changes

maiself mentioned this pull request Oct 25, 2024

Build failure on github actions #28

Closed

Ivorforce mentioned this pull request Oct 25, 2024

Simplify apply_type_list to not use a lambda to fix windows builds. Fixes #28. #29

Merged

Ivorforce mentioned this pull request Oct 25, 2024

Run problems on macOS #30

Closed

Ivorforce commented Oct 26, 2024

View reviewed changes

Ivorforce force-pushed the macos-build-config branch from 3daa317 to 93967c0 Compare October 28, 2024 00:59

Add build configurations and GitHub runners for macOS (x86_64 and arm…

990d8eb

…64). Closes maiself#11.

Ivorforce force-pushed the macos-build-config branch from 589655f to 990d8eb Compare October 28, 2024 01:16

maiself merged commit 4b2cc53 into maiself:master Oct 28, 2024
4 checks passed

Ivorforce deleted the macos-build-config branch October 28, 2024 01:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the builds on macOS (universal). #26

Fix the builds on macOS (universal). #26

Ivorforce commented Oct 24, 2024 •

edited

Loading

Ivorforce commented Oct 24, 2024

maiself Oct 24, 2024

Ivorforce Oct 24, 2024

maiself commented Oct 24, 2024

Ivorforce commented Oct 24, 2024

Ivorforce commented Oct 25, 2024 •

edited

Loading

Ivorforce commented Oct 25, 2024 •

edited

Loading

fire commented Oct 25, 2024 •

edited

Loading

maiself commented Oct 25, 2024

fire commented Oct 25, 2024

maiself commented Oct 25, 2024

Ivorforce commented Oct 26, 2024 •

edited

Loading

Ivorforce Oct 26, 2024

maiself commented Oct 28, 2024

Fix the builds on macOS (universal). #26

Fix the builds on macOS (universal). #26

Conversation

Ivorforce commented Oct 24, 2024 • edited Loading

std::quick_exit

libs

strip -s

dylib vs framework

More work

Ivorforce commented Oct 24, 2024

maiself Oct 24, 2024

Choose a reason for hiding this comment

Ivorforce Oct 24, 2024

Choose a reason for hiding this comment

maiself commented Oct 24, 2024

Ivorforce commented Oct 24, 2024

Ivorforce commented Oct 25, 2024 • edited Loading

Ivorforce commented Oct 25, 2024 • edited Loading

fire commented Oct 25, 2024 • edited Loading

maiself commented Oct 25, 2024

fire commented Oct 25, 2024

maiself commented Oct 25, 2024

Ivorforce commented Oct 26, 2024 • edited Loading

Ivorforce Oct 26, 2024

Choose a reason for hiding this comment

maiself commented Oct 28, 2024

Ivorforce commented Oct 24, 2024 •

edited

Loading

`std::quick_exit`

Ivorforce commented Oct 25, 2024 •

edited

Loading

Ivorforce commented Oct 25, 2024 •

edited

Loading

fire commented Oct 25, 2024 •

edited

Loading

Ivorforce commented Oct 26, 2024 •

edited

Loading