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

Unify bits, android_arch, macos_arch ios_arch into arch, support non-x86 Linux #759

Merged
merged 1 commit into from
Jun 5, 2022

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented May 31, 2022

This PR is the master / 4.x version of #714, and is similar to 55778 in the engine repo.

In the current master branch of godot-cpp, we have several arguments for specifying the architecture: bits, ios_arch, macos_arch, and android_arch. This is a bit of a mess, so this PR replaces all of these with a single unified arch.

The list of valid architectures is ["", "universal", "x86_32", "x86_64", "arm32", "arm64", "rv64", "ppc32", "ppc64", "wasm32"]. The names were chosen to be explicit such that each architecture always includes the bitness in the name (except universal, which contains multiple architectures).

If "", then it will automatically determine the architecture. It will use universal for macOS and iOS, arm64 for Android, wasm32 for JavaScript, and otherwise it will use the host architecture, so arm64 on modern ARM devices, x86_64 on a typical modern desktop, and x86_32 on an old 32-bit desktop. Most users won't have to specify arch manually because it will be automatically selected.

For macOS, iOS, JavaScript, and Android, it will throw an error if trying to build for an invalid architecture. The same should be done for Windows and Linux but I'm not sure how to get cross-compiling working in all cases where it should exist or how to show an error when the necessary libs etc are missing.

This PR does NOT require engine changes to work. The engine can already recognize the names x86_64 etc because it checks these names via the list of supported features in the OS class. There are a few cases where the names are still not quite the same, like x86_32 isn't recognized, but these inconsistencies can be fixed later in the engine code independently of this PR.

I have tested this on Windows x86_64 (works fine before/after this PR), macOS x86_64 (builds with errors before/after this PR), macOS arm64 (builds fine but crashes at runtime before/after this PR), Linux x86_64 (works fine before/after this PR), Linux arm64 and Linux rv64 (doesn't work without this PR, but does work with this PR). PowerPC is untested, and I was not able to get the web / javascript platform working with or without this PR.

This PR also updates the test project to the latest Godot master.

@aaronfranke aaronfranke added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup labels May 31, 2022
@aaronfranke aaronfranke added this to the 4.0 milestone May 31, 2022
@aaronfranke aaronfranke requested a review from akien-mga May 31, 2022 05:55
SConstruct Show resolved Hide resolved
@Faless
Copy link
Contributor

Faless commented May 31, 2022

The required modification to the test SConstruct will break projects that import the library SConstruct as a base.
I think we need to fix that so it's either added in the library SConstruct (requiring no change in the test project), or update the test project .gdextension to work without it (which still breaks other project, but does not require SConstruct change, only the extension file change).

@aaronfranke
Copy link
Member Author

@Faless Breaking other projects is unavoidable with this change, since the point is to change this. But Godot 4 isn't stable yet, so this is acceptable. We could update the test project .gdextension to have 64 point to x86_64 but then this wouldn't work on ARM systems, so I think this is the simplest interim workaround before we update GDExtension on the engine side.

@bruvzg
Copy link
Member

bruvzg commented May 31, 2022

We could update the test project .gdextension to have 64 point to x86_64 but then this wouldn't work on ARM systems, so I think this is the simplest interim workaround before we update GDExtension on the engine side.

It should already work with os.x86_64=... and os.arm64=..., but arch names aren't exactly the same - https://github.com/godotengine/godot/blob/d8b0a8cd29c1f67fe850d0df0ca09ffb4d73fe10/core/os/os.cpp#L416-L460

@aaronfranke aaronfranke force-pushed the scons-cpu-arch branch 2 times, most recently from 03050db to ffdd2d6 Compare May 31, 2022 19:47
@aaronfranke
Copy link
Member Author

@Faless @bruvzg Thanks for the review, it's great to see that GDExtension already supports the x86_64 and arm64 names via the OS features. I updated this PR, including updating the README, and it's much better now.

Unify arguments and add support for ARM64 and RV64 Linux
@Faless Faless merged commit 851ec2f into godotengine:master Jun 5, 2022
@aaronfranke aaronfranke deleted the scons-cpu-arch branch June 5, 2022 17:43
@aaronfranke
Copy link
Member Author

This PR has discovered a pre-existing engine bug where the x86_64 OS feature doesn't work on Windows. :(

@Faless
Copy link
Contributor

Faless commented Jun 5, 2022

Weird, I thought I had tested it, well, we should fix it upstream, I can have a look if you wish :)

@aaronfranke
Copy link
Member Author

aaronfranke/godot@5ec340a

I haven't tested it yet but I made this.

@bruvzg
Copy link
Member

bruvzg commented Jun 5, 2022

This PR has discovered a pre-existing engine bug where the x86_64 OS feature doesn't work on Windows. :(

Is it not loading the library, or not exporting it? Export plugin probably have only 64 feature hard-coded.

@aaronfranke
Copy link
Member Author

aaronfranke commented Jun 5, 2022

@bruvzg The editor crashes because it can't load the library. There's a discussion in the #gdextension channel in RocketChat.

@Faless
Copy link
Contributor

Faless commented Jun 5, 2022

aaronfranke/godot@5ec340a

I haven't tested it yet but I made this.

Oh, that's why it worked for me, it's MSVC not having defined(__x86_64) || defined(__x86_64__) || defined(__amd64__), but since I use mingw like the official builds I didn't get it maybe?

@Faless
Copy link
Contributor

Faless commented Jun 5, 2022

The editor crashes because it can't load the library

Well, it shouldn't crash the engine, I mean, missing gdextension libraries are not supposed to.

@aaronfranke
Copy link
Member Author

Opened this PR, needs testing: godotengine/godot#61732

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

Successfully merging this pull request may close these issues.

3 participants