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

[3.x] Unify bits, macos_arch into arch, support non-x86 on Linux #714

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Feb 27, 2022

This PR unifies arguments for bits and macos_arch, replacing them with one "arch" argument. This simplifies the code and adds support for ARM64 and RV64 Linux (PPC64 may or may not work, completely untested).

I did not change the behavior for Android and iOS to avoid unintentional breakage, but for other platforms this does intentionally break compatibility with the files produced by the build system, so users will need to adjust their .gdnlib files (or just not upgrade to the godot-cpp commit that includes this PR).

On Windows and Linux, either bits or arch can be used to specify the architecture. bits=32 is treated the same as arch=x86_32 and bits=64 is treated the same as arch=x86_64. On macOS, bits does nothing, and either arch or macos_arch can be used to specify the architecture (defaults to universal, can also be set to x86_64 or arm64).

I tested compiling x86_64 Windows, universal/x86_64/arm64 macOS, x86_32/x86_64/arm64/rv64 Ubuntu Linux, and the web platform (the suffix changes from wasm to wasm32).

Old PR description (previously it modified Android and iOS too and removed the old build options):

This PR unifies arguments for bits, android_arch, macos_arch, and ios_arch, replacing them all with one "arch" argument. This simplifies the code and adds support for ARM64 and RV64 Linux (PPC64 may or may not work, completely untested).

This breaks build system compatibility, since now those arguments do nothing, and you need to write "arch=" instead to specify the architecture. However, most users will not need to do this anyway since it will default to a good choice (universal for macOS, arm64 for Android and iOS, wasm32 on web, otherwise host arch). Also, this breaks compatibility with the final library name, so users will need to minorly adjust their projects.

To test it, here is a version built on the 3.4 branch: https://github.com/aaronfranke/godot-cpp/tree/3.4-scons-cpu-arch

And here is a branch of the demos using that 3.4 branch: https://github.com/aaronfranke/gdnative-demos/tree/scons-cpu-arch

I tested the demos using the 3.4 version on x86_64 Windows, universal/x86_64/arm64 macOS, x86_32/x86_64/arm64/rv64 Ubuntu Linux, and I tested compiling without running with arm64/x86_64 Android and arm32/arm64 iOS. I wasn't able to get the JavaScript platform working either with or without these changes. Overall, this PR tries to not overhaul the SConstruct file too much and just improves the architecture handling, so hopefully what was working will keep working.

This PR also updates the headers to the 3.x branch of the headers repo, updates the CI to use 3.5 beta1, and updates the test project SConstruct to be a copy of the SConstruct files in the gdnative-demos repository.

@aaronfranke aaronfranke added enhancement This is an enhancement on the current functionality breaks compat labels Feb 27, 2022
@aaronfranke aaronfranke added this to the 3.5 milestone Feb 27, 2022
Copy link

@Karmavil Karmavil left a comment

Choose a reason for hiding this comment

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

Just learning from your work. Thanks nice job!

SConstruct Outdated Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
@aaronfranke
Copy link
Member Author

aaronfranke commented Jun 24, 2022

Rebased and updated following #691. Note that this PR is for 3.x, the master version #759 is already merged.

@aaronfranke aaronfranke changed the title [3.x] Unify bits, android_arch, macos_arch, ios_arch into arch, support non-x86 on Linux [3.x] Unify bits, macos_arch into arch, support non-x86 on Linux Sep 8, 2022
SConstruct Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
SConstruct Show resolved Hide resolved
SConstruct Show resolved Hide resolved
SConstruct Show resolved Hide resolved
SConstruct Show resolved Hide resolved
SConstruct Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

Oh sorry, I completely missed that I was reviewing a 3.x PR and not master.

So my points about removing support for the old arguments were maybe not so good, it's probably not a good idea to break compat in 3.6 when it's the 7th and last minor release of the 3.x series. So we should maybe bring them back... that being said I'm not too convinced of the value of doing this change at all for 3.6 only.

And so my above review comments actually apply to #759 which should be further updated to sync with what we ended up merging in godotengine/godot.

@aaronfranke
Copy link
Member Author

Un-updated this PR to be similar to what it was before except the Linux architecture changes are still gone and the x86 for x86_32 alias is still present. The env["arch"] = env_arch stuff is unavoidable if we want to have multiple arguments contribute to the architecture selection.

@aaronfranke aaronfranke force-pushed the 3.x-scons-cpu-arch branch from c8aeca4 to 900a4d0 Compare July 6, 2023 17:04
@aaronfranke
Copy link
Member Author

Rebased the PR to resolve conflicts. If we want to support non-x86 architectures on Windows and Linux in Godot 3.6, this PR is useful. Users would have to adjust the .gdnlib file but that's a very quick fix. Old arguments for the build system will continue to work since it supports using bits and macos_arch flags which it then maps to arch.

@aaronfranke aaronfranke force-pushed the 3.x-scons-cpu-arch branch from 900a4d0 to 2638dd5 Compare July 8, 2023 15:29
@aaronfranke aaronfranke requested a review from akien-mga July 8, 2023 15:35
@aaronfranke aaronfranke modified the milestones: 3.5, 3.x Jul 8, 2023
@dsnopek dsnopek added the topic:buildsystem Related to the buildsystem or CI setup label Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks compat 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.

5 participants