Unify bits, arch, and android_arch into env["arch"] #55778

aaronfranke · 2021-12-10T07:48:37Z

This PR addresses part of godotengine/godot-proposals#1416

In the master branch before this PR, we have several arguments for specifying the architecture: bits, arch, and android_arch. This is a bit of a mess, so this PR replaces all three of these with a single unified arch. Note that env["bits"] still exists internally for now (or it could be kept forever).

The list of valid architectures is ["auto", "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 for "auto").

If "auto", then it will automatically determine the architecture. It will use arm64 for Android and iOS, 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.

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. There's also several places in both Windows and Linux that are hard-coded for x86. I left some TODOs in the Windows and Linux code for future PRs.

I've tested this PR compiling on an arm64 macOS host for x86_64 and arm64 macOS and for arm64 iOS, on an arm64 Ubuntu VM for arm64 Linux, on a rv64 Ubuntu VM for rv64 Linux, on an x86_64 Ubuntu host for x86_64 Linux, on an x86_32 Ubuntu VM for x86_32 Linux (fails linking, but happens on master too), on an x86_64 Windows host for x86_32 and x86_64 Windows using MSVC and x86_64 using MinGW, on both an x86_64 Windows and x86_64 Ubuntu host for x86_32, x86_64, arm32, and arm64 Android (x86 Android fails linking on Ubuntu but this happens on master too, and I only tested if the arm64 Android build runs), and on an x86_64 Windows host for wasm32 JavaScript. Still, it needs further testing. Most of the platforms I've tested on have revealed bugs (some of which exist on master too).

This PR also simplifies a lot of architecture checks (such as the ones in the denoise module).

@q66 Can you test if this works as expected on PowerPC?

platform/iphone/detect.py

modules/mono/build_scripts/mono_configure.py

neikeq · 2021-12-10T10:19:34Z

I think the drop of 32bit support on iOS should go into its own commit.
It's much better to have Drop iOS 32-bit support commit than for these changes to be hidden in an unrelated commit.

akien-mga · 2021-12-10T10:26:46Z

I think the drop of 32bit support on iOS should go into its own commit. It's much better to have Drop iOS 32-bit support commit than for these changes to be hidden in an unrelated commit.

Yes, and similarly I would put the actual build fixes in a dedicated PR too. This should only focus on buildsystem changes.
(In the same vein, I didn't comment as I didn't want to delay further, but for #55572 I would also have preferred separate PRs for actually fixing the double build and adding it to the CI.)

aaronfranke · 2021-12-10T16:24:58Z

Opened #55792 and #55793 which take pieces out of this PR.

SConstruct

platform/android/java/lib/build.gradle

bruvzg · 2021-12-15T10:23:26Z

platform/osx/export/export_plugin.cpp

-			if (file.find("/data.mono.osx.64.release_debug/") != -1) {
+			if (file.find("/data.mono.osx.x86_64.release_debug/") != -1) {
 				if (!p_debug) {
 					ret = unzGoToNextFile(src_pkg_zip);
 					continue; // skip
 				}
-				file = file.replace("/data.mono.osx.64.release_debug/", "/GodotSharp/");
+				file = file.replace("/data.mono.osx.x86_64.release_debug/", "/GodotSharp/");
 			}
-			if (file.find("/data.mono.osx.64.release/") != -1) {
+			if (file.find("/data.mono.osx.x86_64.release/") != -1) {
 				if (p_debug) {
 					ret = unzGoToNextFile(src_pkg_zip);
 					continue; // skip
 				}
-				file = file.replace("/data.mono.osx.64.release/", "/GodotSharp/");
+				file = file.replace("/data.mono.osx.x86_64.release/", "/GodotSharp/");


This should stay 64 or skip architecture at all, usually this path point to the Universal (x86_64 + arm64) binaries.

Since this is supposed to point to universal, I reverted this part for this PR, but I think it would make sense to remove the architecture in the future for universal things, or use the word "universal".

Yeah I think it can be left out for this PR, but that's something that needs more work later.

We can indeed remove the bits/arch part which is currently irrelevant. But we have to think about whether we want to actually make it possibility to support different architectures with this like we do on other platforms, and thus have:

data.mono.osx.x86_64.release data.mono.osx.arm64.release data.mono.osx.universal.release

Official builds would use the latter, but users who want to ship separate x86_64 and arm64 builds for some reason would still have the option to do it with pre-defined names. Currently you'd have to use data.mono.osx.64.release regardless of what the actual native binaries inside are compiled for.

For the record the names were changed here: https://github.com/godotengine/godot/pull/63096/files#diff-72b5d6bdf8391a9477aca91fe06263335d88623f353c71cc56ffc8bda6102221

bruvzg · 2022-08-16T06:22:50Z

platform/windows/detect.py

+    if env["arch"] == "x86_32":
        cmdbase = env["mingw_prefix_32"]
    else:
        cmdbase = env["mingw_prefix_64"]


I guess it should be simply a single mingw_prefix that can be set alongside arch.

platform/windows/detect.py

bruvzg · 2022-08-16T06:45:23Z

platform/windows/detect.py

-            "Failed to manually detect MSVC compiler architecture version... Defaulting to 32bit executable settings"
-            " (forcing bits=32). Compilation attempt will continue, but SCons can not detect for what architecture this"
+            "Failed to manually detect MSVC compiler architecture version... Defaulting to x86 32-bit executable settings"
+            " (forcing arch=x86_32). Compilation attempt will continue, but SCons can not detect for what architecture this"
            " build is compiled for. You should check your settings/compilation setup, or avoid setting VCINSTALLDIR."
        )


This should just fail, there's no point to continue if compiler is not present.

The print is no longer accurate then, since it says explicitly that it's falling back to x86 32-bit settings.

platform/windows/detect.py

bruvzg · 2022-08-16T06:51:12Z

platform/windows/detect.py

+    # TODO: This doesn't seem to be working, or maybe I just have
+    # MinGW set up incorrectly. It always gives me x86_64 builds,
+    # even though arch == "x86_32" and the file name has x86_32 in it.


Depending on setup, MinGW can have either prefixed binaries (e.g. x86_64-w64-mingw32-gcc) or everything in the subfolder (gcc) in this case which one used is determined by path. I'm not sure how to detect arch in this case, we can do something like detect_visual_c_compiler_version, but MinGW path is not standard (might work for MSYS2 - https://www.msys2.org/docs/environments/).

bruvzg

Using -march= seems to work fine with MinGW, but x86-64 for example only available if running from 64-bit MSYS2 environment.

So I guess the best way is to, try to set prefix based on arch if it's not set (ignore it otherwise since it can be something line /mingw/bin/ without arch name). Always set -march to make sure it's building for the correct one. Plus check MSYS2 env variables and warn if trying to use 64-bit arch in 32-bit env.

For MSVC, I think we can always force compilers path based on arch, even if it's started from wrong environment (maybe with a warning as well).

In any case, this should be good to merge as is (after rebase), and I'll add both checks / -march for Windows cross compile when implementing ARM support for Windows.

modules/mono/build_scripts/mono_configure.py

akien-mga · 2022-08-25T08:30:23Z

I'm pretty happy with the state of this PR now, I'll do a final review and squash the commits so we can merge.

My only remaining concern is the same as expressed by @m4gr3d, there's potential confusion between the values we use for arch and the values with have to use for ABI names/folders in Android.

Fully removes the `bits` option and adapts the code that relied on it. Co-authored-by: Rémi Verschelde <[email protected]>

akien-mga · 2022-08-25T09:22:36Z

I reviewed the changes again and amended a couple nitpicks.

I tested an Android build with:

scons p=android arch=arm64 tools=no target=release_debug
cd platform/android/java
./gradlew generateGodotTemplates

And it seems to work fine. I could use the resulting APK and source templates to do both APK and Custom Build/AAB exports to my arm64 Poco F1, which ran fine.

I still have some concerns about the Android+Mono changes, and I think some renaming might be in order to make a clear separation between the arch as used by Godot SCons and the abi as expected by Gradle/Android. We should avoid assigning Godot's env["arch"] names to variables named abi IMO. But I think this can be cleaned up in future PRs together with testing and if needed fixing the Android+Mono export code.

Original concerns seem to have been addressed. Needs re-review/testing.

akien-mga

Amazing work!

akien-mga · 2022-08-25T11:03:55Z

Thanks!

Needed after godotengine/godot#55778. Not sure how alpha15 built properly without this :O

aaronfranke added platform:web platform:android topic:buildsystem labels Dec 10, 2021

aaronfranke added this to the 4.0 milestone Dec 10, 2021

aaronfranke requested review from akien-mga and a team December 10, 2021 07:48

aaronfranke requested review from a team as code owners December 10, 2021 07:48

aaronfranke added the needs testing label Dec 10, 2021

bruvzg reviewed Dec 10, 2021

View reviewed changes

platform/iphone/detect.py Outdated Show resolved Hide resolved

aaronfranke force-pushed the use-arch-btw branch from 2f98468 to 46d18ca Compare December 10, 2021 08:21

neikeq suggested changes Dec 10, 2021

View reviewed changes

modules/mono/build_scripts/mono_configure.py Outdated Show resolved Hide resolved

aaronfranke force-pushed the use-arch-btw branch 2 times, most recently from 5428d42 to 9735747 Compare December 11, 2021 03:50

m4gr3d reviewed Dec 13, 2021

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

m4gr3d reviewed Dec 13, 2021

View reviewed changes

platform/android/java/lib/build.gradle Outdated Show resolved Hide resolved

neikeq approved these changes Dec 14, 2021

View reviewed changes

bruvzg reviewed Dec 15, 2021

View reviewed changes

akien-mga added the enhancement label Dec 15, 2021

aaronfranke force-pushed the use-arch-btw branch from 9735747 to dd009bb Compare December 16, 2021 02:02

aaronfranke marked this pull request as ready for review August 13, 2022 19:31

bruvzg reviewed Aug 16, 2022

View reviewed changes

bruvzg approved these changes Aug 24, 2022

View reviewed changes

aaronfranke force-pushed the use-arch-btw branch 2 times, most recently from a556b4e to 1ae98cc Compare August 24, 2022 20:11

neikeq reviewed Aug 24, 2022

View reviewed changes

modules/mono/build_scripts/mono_configure.py Outdated Show resolved Hide resolved

aaronfranke force-pushed the use-arch-btw branch from 1ae98cc to b2b30da Compare August 25, 2022 01:06

bruvzg mentioned this pull request Aug 25, 2022

[macOS] Check .NET binary architecture, and search for the cross compile SDK in the subfolders. #64869

Merged

akien-mga force-pushed the use-arch-btw branch from b2b30da to 10bcb1e Compare August 25, 2022 09:17

Unify bits, arch, and android_arch into env["arch"]

27b0f18

Fully removes the `bits` option and adapts the code that relied on it. Co-authored-by: Rémi Verschelde <[email protected]>

akien-mga force-pushed the use-arch-btw branch from 10bcb1e to 27b0f18 Compare August 25, 2022 09:19

akien-mga approved these changes Aug 25, 2022

View reviewed changes

akien-mga merged commit c7eb423 into godotengine:master Aug 25, 2022

SaracenOne mentioned this pull request Aug 25, 2022

MSVC builds are broken #64904

Closed

m4gr3d mentioned this pull request Aug 26, 2022

Revert the architecture values update made to the Android export logic #64912

Merged

Zylann mentioned this pull request Aug 26, 2022

Broken Scons file refers to an undefined dictionary key 'bits' Zylann/godot_voxel#434

Closed

Zylann added a commit to Zylann/godot_voxel that referenced this pull request Aug 26, 2022

bits was removed from Godot's build system (godotengine/godot#55778)

bf10c13

raulsntos mentioned this pull request Aug 28, 2022

Fix .NET exporting in Linux #64987

Merged

aaronfranke deleted the use-arch-btw branch September 6, 2022 00:28

akien-mga added a commit to godotengine/godot-build-scripts that referenced this pull request Sep 7, 2022

Android: Fix still using android_arch over arch

4677343

Needed after godotengine/godot#55778. Not sure how alpha15 built properly without this :O

akien-mga mentioned this pull request Sep 7, 2022

Android: Fix still using android_arch over arch godotengine/godot-build-scripts#68

Merged

aaronfranke mentioned this pull request Dec 30, 2022

[macOS] Add support for one step universal binary build. #70740

Closed

aaronfranke mentioned this pull request Feb 20, 2023

Cross compilation ignores arch and bits options #24008

Closed

raulsntos mentioned this pull request Mar 4, 2023

C#: Support exporting for Android #73257

Merged

3 tasks

Faless mentioned this pull request Feb 15, 2024

[3.x] Fix X11/Server cross-bits compilation non-x86 #88369

Open

akien-mga mentioned this pull request May 11, 2024

libtheora compiling errors for ARM (Hard Float) #23506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify bits, arch, and android_arch into env["arch"] #55778

Unify bits, arch, and android_arch into env["arch"] #55778

aaronfranke commented Dec 10, 2021 •

edited

Loading

neikeq commented Dec 10, 2021

akien-mga commented Dec 10, 2021

aaronfranke commented Dec 10, 2021 •

edited

Loading

bruvzg Dec 15, 2021

aaronfranke Dec 16, 2021 •

edited

Loading

akien-mga Dec 16, 2021

aaronfranke Apr 5, 2023

bruvzg Aug 16, 2022

bruvzg Aug 16, 2022

akien-mga Aug 25, 2022

bruvzg Aug 16, 2022 •

edited

Loading

bruvzg left a comment

akien-mga commented Aug 25, 2022 •

edited

Loading

akien-mga commented Aug 25, 2022

akien-mga left a comment

akien-mga commented Aug 25, 2022

Unify bits, arch, and android_arch into env["arch"] #55778

Unify bits, arch, and android_arch into env["arch"] #55778

Conversation

aaronfranke commented Dec 10, 2021 • edited Loading

neikeq commented Dec 10, 2021

akien-mga commented Dec 10, 2021

aaronfranke commented Dec 10, 2021 • edited Loading

bruvzg Dec 15, 2021

Choose a reason for hiding this comment

aaronfranke Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

akien-mga Dec 16, 2021

Choose a reason for hiding this comment

aaronfranke Apr 5, 2023

Choose a reason for hiding this comment

bruvzg Aug 16, 2022

Choose a reason for hiding this comment

bruvzg Aug 16, 2022

Choose a reason for hiding this comment

akien-mga Aug 25, 2022

Choose a reason for hiding this comment

bruvzg Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

bruvzg left a comment

Choose a reason for hiding this comment

akien-mga commented Aug 25, 2022 • edited Loading

akien-mga commented Aug 25, 2022

akien-mga left a comment

Choose a reason for hiding this comment

akien-mga commented Aug 25, 2022

aaronfranke commented Dec 10, 2021 •

edited

Loading

aaronfranke commented Dec 10, 2021 •

edited

Loading

aaronfranke Dec 16, 2021 •

edited

Loading

bruvzg Aug 16, 2022 •

edited

Loading

akien-mga commented Aug 25, 2022 •

edited

Loading