SCons: Unify tools/target build type configuration #66242

akien-mga · 2022-09-22T07:03:01Z

Implements godotengine/godot-proposals#3371.

TL;DR:

To build the Godot editor for engine development, use scons target=editor dev_build=yes
- You might also want to add dev_mode=yes which is an alias for verbose=yes warnings=extra werror=yes tests=yes
To build the Godot editor to use while developing your game, use scons target=editor
- If you get crashes and want to get a useful stacktrace to report a bug, rebuild with scons target=editor debug_symbols=yes
Both the intermediate object files (.o, .a, .obj, .lib) and the Godot binary in bin are changing names, so you might want to clean your Git checkout to remove the old ones to save disk space, and adjust any scripts/config you might have that runs the Godot binary to use the new name.

New `target` presets

The tools option is removed and target changes to use three new presets,
which match the builds users are familiar with. These targets control the
default optimization level and enable editor-specific and debugging code:

editor: Replaces tools=yes target=release_debug.
- Defines: TOOLS_ENABLED, DEBUG_ENABLED, -O2//O2
template_debug: Replaces tools=no target=release_debug.
- Defines: DEBUG_ENABLED, -O2//O2
template_release: Replaces tools=no target=release.
- Defines: -O3//O2

New `dev_build` option

The previous target=debug is now replaced by a separate dev_build=yes
option, which can be used in combination with either of the three targets,
and changes the following:

dev_build: Defines DEV_ENABLED, disables optimization (-O0//0d),
enables generating debug symbols, does not define NDEBUG so assert()
works in thirdparty libraries, adds a .dev suffix to the binary name.

Note: Unlike previously, dev_build defaults to off so that users who
compile Godot from source get an optimized and small build by default.
Engine contributors should now set dev_build=yes in their build scripts or
IDE configuration manually.

Changed binary names

The name of generated binaries and object files are changed too, to follow
this format:

godot.<platform>.<target>[.dev][.double].<arch>[.<extra_suffix>][.<ext>]

For example:

godot.linuxbsd.editor.dev.arm64
godot.windows.template_release.double.x86_64.mono.exe

Be sure to update your links/scripts/IDE config accordingly.

More flexible `optimize` and `debug_symbols` options

The optimization level and whether to generate debug symbols can be further
specified with the optimize and debug_symbols options. So the default
values listed above for the various target and dev_build combinations
are indicative and can be replaced when compiling, e.g.:

scons p=linuxbsd target=template_debug dev_build=yes optimize=debug
will make a "debug" export template with dev-only code enabled, -Og
optimization level for GCC/Clang, and debug symbols. Perfect for debugging
complex crashes at runtime in an exported project.

The above description was updated to match the latest state of this PR.

Original description and comments

There's a lot to discuss there, I'll try to make a list of the main point.

As described in the proposal, this PR unifies the current tools and target combinations in a single target enum, with the intention of making the valid build targets clearer, and to clarify the confusion around the meaning of the word "debug".

The targets implemented here are therefore:

target:
    editor:
        editor: yes (TOOLS_ENABLED)
        debug-features: yes (DEBUG_ENABLED)
        dev-features: no
        optimize: speed-debug (-O2)
        debug_symbols: yes (-g2)
    editor-dev:
        editor: yes (TOOLS_ENABLED)
        debug-features: yes (DEBUG_ENABLED)
        dev-features: yes (DEV_ENABLED)
        optimize: debug (-Og)
        debug_symbols: yes (-g3)
    template-release:
        editor: no
        debug-features: no
        dev-features: no
        optimize: speed (-O3) / size (-Os) on Web
        debug_symbols: no
    template-debug:
        editor: no
        debug-features: yes (DEBUG_ENABLED)
        optimize: speed-debug (-O2) / size (-Os) on Web
        debug_symbols: yes (-g2)
    template-dev:
        editor: no
        debug-features: yes (DEBUG_ENABLED)
        dev-features: yes (DEV_ENABLED)
        optimize: debug (-Og)
        debug_symbols: yes (-g3)

Notes

The new target mostly governs three things:
- Editor or export template
- Debugging features (always true for editor, true for "debug" export template)
- Dev build with full debugging symbols (-g3 -Og)
  - This one could possibly be split into a separate dev_build=yes option, which would bring down the target enum to "editor", "template-release", "template-debug", which are effectively what we ship. Thoughts? This idea is growing on me as I type it.
optimize and debug_symbols can be overridden for any target.
- So if you want, you can build target=editor-dev optimize=size debug_symbols=no or something. It doesn't make much sense per se but the flexibility can be useful in other situations like target=template-release optimize=speed-debug debug_symbols=yes. This is not exactly the same as template-debug as the debug features are still off - so it can be pertinent to debug an issue that only happens in release export templates and may be linked to not have DEBUG_ENABLED code.
The object suffix was changed from the previous mix of opt, tools and debug to just use target, so the build objects and final binary names are changing. E.g. scons p=linuxbsd target=editor-dev arch=x86_64 will produce bin/godot.linuxbsd.editor-dev.x86_64.
The default option for scons without target is now target=editor, which will imply optimize=speed-debug debug_symbols=yes
- This is different to the current default which is tools=yes target=debug debug_symbols=yes which means a debug-optimized (-Og) (or actually one some platforms it would be unoptimized -O0) build with DEV_ENABLED.
- Basically this makes the default more end-user friendly (they get a usable editor build for using Godot), and contributors will have to specify target=editor-dev to get back the previous behavior.
- The default values for optimize and debug_symbols in the SCons help are not really accurate as they eventually depend on the target. I could add auto as a default to clarify this, though this can be a bit tricky to handle properly.
Compiler and linker flags for the different optimize and debug_symbols levels are now unified in SConstruct, right after configure (needs to be after for env.msvc to be valid).
- This should be refactored eventually to reduce the size of the SConstruct and make the code more readable. Could be in a dedicated function / SCons Tool.
- All platforms used to do their own handling of flags for all targets, I did my best to unify what can be but there are some platform-specific tweaks that remain in the various detect.py, which should be assessed. There are also some default values changed which I'll point out in review comments.
godot-cpp would need to be ported to support something similar probably. Whether or not it can/should might also inform us on whether the set of target presets proposed here is actually the best design or not.
- I didn't port the GDExtension compatibility build script for TextServers, it does some reimplementation of the SHLIBSUFFIX
We have a mess of platform-specific defines of DEBUG=1, DEBUG:FULL _DEBUG, NDEBUG... this needs a thorough review and unification to make sure we do the right thing in the right situation. Notably, it used to be tied to either using debug_symbols=yes or target=debug but I'm not sure it's a good idea for all of these.
- See SCons: Cleanup DEBUG, _DEBUG and NDEBUG defines #66303.
All these configuration options could possibly be handled a bit more elegantly with e.g. a Python class that keeps track of this data and can be accessed from SCsubs via the env, e.g. env.target.editor_build / env.target.template_build / env.target.debug_features, etc.
Some platforms (Web and iOS) used to set their optimization flags both for the compiler and the linker (CCFLAGS and LINKFLAGS). I don't know why, but I guess the LINKFLAGS lines can be safely dropped?
Clang based platforms (Android, iOS and macOS) all defined -ftree-vectorize for both speed and size optimizations, and -fomit-frame-pointer for speed optimizations. This is apparently supported by GCC too, and possibly by Emscripten (LLVM based too), so should we define them consistently for all platforms? On MSVC they're somewhat part of /O1//O2 already by way of /Oy.
- See Review use of -fomit-frame-pointer and -ftree-vectorize flags for optimized builds #66296 and SCons: Remove redundant -fomit-frame-pointer and -ftree-vectorize #66297.

bruvzg · 2022-09-22T07:20:24Z

platform/macos/detect.py

+    # TODO: Re-evaluate the need for this / streamline with common config.
+    if env["target"] == "template-release":
+        if env["optimize"].startswith("speed"):
+            env.Prepend(CCFLAGS=["-fomit-frame-pointer", "-ftree-vectorize"])


These options are not macOS or clang specific, so probably should be set everywhere (/Oy for MSVC).

With exception to -no_deduplicate (LD64 flag).

Indeed, currently we seem to set -fomit-frame-pointer -ftree-vectorize only for Clang builds: Android, iOS, macOS.
I don't know why / if there's a problem using those with GCC. I guess we could try them out in a separate PR so that we don't have to deal with changing this optimization in this massive refactor.

/Oy for MSVC seems implied by both /O1 and /O2 so I guess it's not needed: https://learn.microsoft.com/en-us/cpp/build/reference/oy-frame-pointer-omission?view=msvc-170

-fomit-frame-pointer has been a thing in GCC for a very long time (12+ years). I've seen it used by default in a lot of C++ projects, so I'd say it's battle-tested at this point 🙂

I researched this a bit in #66296, and found that we don't need these flags at all, so #66297 removes them.

bruvzg · 2022-09-22T07:26:01Z

platform/linuxbsd/detect.py

-    elif env["target"] == "debug":
-        env.Prepend(CCFLAGS=["-g3"])
+    # TODO: Re-evaluate the need for this / streamline with common config.
+    if env["target"].endswith("-dev"):


Not sure why it's here. Isn't it to make executable symbols visible to the libraries?

Yeah we noticed this discrepancy a while ago in March on Rocket.Chat, discussing it with @raulsntos and @neikeq: https://chat.godotengine.org/channel/devel?msg=2Dkxu3KsP2TZEwRNn

It seems it was added by @marcelofg55 in #11061, presumably for the Linux crash handler. I don't know if it's actually needed though. I can try crashing Godot without it and see what happens.

It also used to be used by the Mono module (on all targets, which would actually prevent removing unused symbols, explaining a part of the size difference between Mono and non-Mono builds in 3.x) but it's not needed anymore with .NET.

So I gave it a try, and indeed without -rdynamic the crash handler doesn't retrieve the symbols properly.

With -rdynamic:

================================================================ handle_crash: Program crashed with signal 4 Engine version: Godot Engine v4.0.beta.custom_build (8e14f9ba21725a9445f3979742f2fc87c8a7b463) Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues [1] /lib64/libc.so.6(+0x36970) [0x7f4f2fa7d970] (??:0) [2] core_bind::OS::crash(String const&) (/home/akien/Projects/godot/godot.git/core/core_bind.cpp:232) [3] void call_with_ptr_args_helper<__UnexistingClass, String const&, 0ul>(__UnexistingClass*, void (__UnexistingClass::*)(String const&), void const**, IndexSequence<0ul>) (/home/akien/Projects/godot/godot.git/./core/variant/binder_common.h:283 (discriminator 8)) [4] void call_with_ptr_args<__UnexistingClass, String const&>(__UnexistingClass*, void (__UnexistingClass::*)(String const&), void const**) (/home/akien/Projects/godot/godot.git/./core/variant/binder_common.h:532) [5] MethodBindT<String const&>::ptrcall(Object*, void const**, void*) (/home/akien/Projects/godot/godot.git/./core/object/method_bind.h:331) [6] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (/home/akien/Projects/godot/godot.git/modules/gdscript/gdscript_vm.cpp:1965) [7] GDScript::_create_instance(Variant const**, int, Object*, bool, Callable::CallError&) (/home/akien/Projects/godot/godot.git/modules/gdscript/gdscript.cpp:175) [8] GDScript::instance_create(Object*) (/home/akien/Projects/godot/godot.git/modules/gdscript/gdscript.cpp:406) [9] Object::set_script(Variant const&) (/home/akien/Projects/godot/godot.git/core/object/object.cpp:843) [10] MainLoop::initialize() (/home/akien/Projects/godot/godot.git/core/os/main_loop.cpp:61 (discriminator 4)) [11] OS_LinuxBSD::run() (/home/akien/Projects/godot/godot.git/platform/linuxbsd/os_linuxbsd.cpp:571) [12] ./bin/godot.linuxbsd.tools.x86_64.rdynamic(main+0x146) [0x2460eec] (/home/akien/Projects/godot/godot.git/platform/linuxbsd/godot_linuxbsd.cpp:74) [13] /lib64/libc.so.6(+0x23677) [0x7f4f2fa6a677] (??:0) [14] /lib64/libc.so.6(__libc_start_main+0x85) [0x7f4f2fa6a735] (??:0) [15] ./bin/godot.linuxbsd.tools.x86_64.rdynamic(_start+0x21) [0x2460ce1] (??:?) -- END OF BACKTRACE -- ================================================================

Without -rdynamic:

================================================================ handle_crash: Program crashed with signal 4 Engine version: Godot Engine v4.0.beta.custom_build (8e14f9ba21725a9445f3979742f2fc87c8a7b463) Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues [1] /lib64/libc.so.6(+0x36970) [0x7fb97e2c1970] (??:0) [2] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0x43cc3d7] (/home/akien/Projects/godot/godot.git/core/core_bind.cpp:232) [3] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0x756b14] (/home/akien/Projects/godot/godot.git/./core/variant/binder_common.h:283 (discriminator 8)) [4] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0x756737] (/home/akien/Projects/godot/godot.git/./core/variant/binder_common.h:532) [5] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0x7563d0] (/home/akien/Projects/godot/godot.git/./core/object/method_bind.h:331) [6] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0xc647a4] (/home/akien/Projects/godot/godot.git/modules/gdscript/gdscript_vm.cpp:1965) [7] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0xb0ad5f] (/home/akien/Projects/godot/godot.git/modules/gdscript/gdscript.cpp:175) [8] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0xb0c41e] (/home/akien/Projects/godot/godot.git/modules/gdscript/gdscript.cpp:406) [9] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0x49b029e] (/home/akien/Projects/godot/godot.git/core/object/object.cpp:843) [10] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0x4479a86] (/home/akien/Projects/godot/godot.git/core/os/main_loop.cpp:61 (discriminator 4)) [11] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0x20fd63] (/home/akien/Projects/godot/godot.git/platform/linuxbsd/os_linuxbsd.cpp:571) [12] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0x209eec] (/home/akien/Projects/godot/godot.git/platform/linuxbsd/godot_linuxbsd.cpp:74) [13] /lib64/libc.so.6(+0x23677) [0x7fb97e2ae677] (??:0) [14] /lib64/libc.so.6(__libc_start_main+0x85) [0x7fb97e2ae735] (??:0) [15] ./bin/godot.linuxbsd.tools.x86_64.no_rdynamic() [0x209ce1] (??:?) -- END OF BACKTRACE -- ================================================================

Size difference:

-rwxr-xr-x 1 akien akien 697M Sep 22 13:41 godot.linuxbsd.tools.x86_64.no_rdynamic* -rwxr-xr-x 1 akien akien 731M Sep 22 13:41 godot.linuxbsd.tools.x86_64.rdynamic*

However gdb has no problem getting the stacktrace from the build without -rdynamic so this might be a problem with our crash handler. It does imply that even with debug symbols, our release_debug builds currently wouldn't give a meaningful backtrace.

(gdb) bt #0 0x00000000043cc3d7 in core_bind::OS::crash (this=0x8eb3ee0, p_message=...) at core/core_bind.cpp:232 #1 0x0000000000756b14 in call_with_ptr_args_helper<__UnexistingClass, String const&, 0ul> (p_instance=0x8eb3ee0, p_method=(void (__UnexistingClass::*)(__UnexistingClass * const, const String &)) 0x43cc392 <core_bind::OS::crash(String const&)>, p_args=0x7fffffff9a20) at ./core/variant/binder_common.h:283 #2 0x0000000000756737 in call_with_ptr_args<__UnexistingClass, String const&> (p_instance=0x8eb3ee0, p_method=(void (__UnexistingClass::*)(__UnexistingClass * const, const String &)) 0x43cc392 <core_bind::OS::crash(String const&)>, p_args=0x7fffffff9a20) at ./core/variant/binder_common.h:531 #3 0x00000000007563d0 in MethodBindT<String const&>::ptrcall (this=0x8eb4c30, p_object=0x8eb3ee0, p_args=0x7fffffff9a20, r_ret=0x0) at ./core/object/method_bind.h:329 #4 0x0000000000c647a4 in GDScriptFunction::call (this=0xaabc110, p_instance=0xb7736a0, p_args=0x0, p_argcount=0, r_err=..., p_state=0x0) at modules/gdscript/gdscript_vm.cpp:1962 #5 0x0000000000b0ad5f in GDScript::_create_instance (this=0xb773e20, p_args=0x0, p_argcount=0, p_owner=0x7fffac002d70, p_is_ref_counted=false, r_error=...) at modules/gdscript/gdscript.cpp:175 #6 0x0000000000b0c41e in GDScript::instance_create (this=0xb773e20, p_this=0x7fffac002d70) at modules/gdscript/gdscript.cpp:406 #7 0x00000000049b029e in Object::set_script (this=0x7fffac002d70, p_script=...) at core/object/object.cpp:843 #8 0x0000000004479a86 in MainLoop::initialize (this=0x7fffac002d70) at core/os/main_loop.cpp:61 #9 0x000000000020fd63 in OS_LinuxBSD::run (this=0x7fffffffcff0) at platform/linuxbsd/os_linuxbsd.cpp:563 #10 0x0000000000209eec in main (argc=3, argv=0x7fffffffd548) at platform/linuxbsd/godot_linuxbsd.cpp:72

For now I'm adding a comment to clarify that, but we might be able to get rid of the flag by reworking the crash handler.

akien-mga · 2022-09-22T07:46:59Z

SConstruct

-        if env["tools"]:
-            env["tests"] = methods.get_cmdline_bool("tests", True)
+        env["tests"] = methods.get_cmdline_bool("tests", True)


This changes behavior, tests will now be compiled also for template builds with the dev=yes convenience shortcut.
We already have tests working on non-editor builds so there's no reason to exclude them when full-blown dev mode is requested.

akien-mga · 2022-09-22T07:54:28Z

SConstruct

+    # Set optimize and debug_symbols flags.
+    # Needs to happen after configure to have `env.msvc` defined.


@Faless I started from https://github.com/godotengine/godot-cpp/blob/master/tools/targets.py but as I started syncing with the platform-specific detect.py I had to change some things which should likely be unified further in godot-cpp:

Removed 0 to 3 flags as 3 isn't valid for MSVC, and IMO the use for these is limited. I think users can specify what they need with CCFLAGS if they need to override what we have (we could add optimize=custom for this if need be, to make sure we don't end up adding -O0 from optimize=none after user-specified CCFLAGS).

Removed auto as the logic is more complex than this, instead we handle it in SConstruct when handling the targets.

Removed forcing debug symbols on -dev (ex debug) builds. There's a sane default now, but I think we should still give precedence to the debug_symbols option if specified.

MSVC:

Removed /Z7 for builds without debug symbols. Copy paste mistake I believe?

Added /FS for debug_symbols=yes as this was used in platform/windows/detect.py. Didn't assess what it is/does.

Removed _DEBUG as we weren't defining it before. I think it's meant to be set by MSVC when using MTd or MDd and IMO shouldn't be tied to debug_symbols, which is something we can strip (a debug_symbols=yes build stripped should be similar to a debug_symbols=no build IMO).

size optimization: the proper flag for this is /O1: https://learn.microsoft.com/en-us/cpp/build/reference/o1-o2-minimize-size-maximize-speed?view=msvc-170. /Os is a subset of /O1 but the equivalent to GCC -Os is /O1 as far as I know (this also makes a case for not exposing 0 - 3 flags as -O1 and /O1 are quite different).

Added env.Append(LINKFLAGS=["/OPT:REF"]) which was in the Windows detect.py.

GCC/Clang:

Added speed-debug for -O2 on what used to be release_debug builds, as that's what we used there (apparently to keep potential backtraces somewhat usable).

I think all the changes are good, and I agree having a optmize=custom that requires you to set CCFLAGS seems reasonable.

/FS seems to be a partial workaround to windows having issues with multiprocessing.
They say it helps though it doesn't actually fix all the problems: https://learn.microsoft.com/en-us/cpp/build/reference/fs-force-synchronous-pdb-writes?view=msvc-170
Seems safe to use.

I added optimize=custom in the latest iteration.

akien-mga · 2022-09-22T07:59:11Z

platform/android/detect.py

-        elif env["optimize"] == "size":  # optimize for size
-            env.Append(CCFLAGS=["-Oz"])


A couple differences for Android:

optimize=size used to be -Oz, now it's -Os like for all other platforms. -Oz is apparently a more aggressive size optimization flag, but for the Web build we have this info:

# -Os reduces file size by around 5 MiB over -O3. -Oz only saves about # 100 KiB over -Os, which does not justify the negative impact on # run-time performance.

target=debug used to force no optimization (-O0), now it uses debug optimization (-Og) like other platforms, which is a subset of -O1 which does not hinder debugging.

akien-mga · 2022-09-22T08:08:31Z

platform/android/detect.py

+        env.Append(CCFLAGS=["-fno-limit-debug-info"])
        env.Append(CPPDEFINES=["_DEBUG"])
        env.Append(CPPFLAGS=["-UNDEBUG"])


Some of these defines seem to be overkill for Android:

-fno-limit-debug-info isn't defined on any other platform so not sure why it's needed here. Seems like it was added a long time ago by @RandomShaper in Improve Android build (Clang + tidyness) #6958, do you remember why?

Undefining NDEBUG shouldn't be needed since we don't define it. But most importantly we want to avoid exceptions and AFAIK this controls exception behavior in assert so I'm not sure why we specifically want to re-enable it. Was also added in Improve Android build (Clang + tidyness) #6958.

_DEBUG AFAIK is a MSVC specific define used for CRT debugging, so it makes no sense for Android. It's been there since Godot was open sourced, can likely be safely removed as it was likely an oversight.

In Android I tried to match the flags set by the NDK as close as possible. However, I understand that some may not be technically needed and the NDK is adding them to support Android tooling (i.e., Android Studio). It's hard to tell. Maybe @m4gr3d has some insight on how convenient it'd be to remove it.

Cleaning this up in #66303.

akien-mga · 2022-09-22T08:11:11Z

platform/ios/detect.py

+        env.Append(CCFLAGS=["-gdwarf-2"])
        env.Append(CPPDEFINES=["_DEBUG", ("DEBUG", 1)])


@bruvzg

Does it make a difference for iOS if we use -g or -gdwarf-2? If not we can remove that first line.

Worth noting that the change will also make iOS template-dev build (optimize=debug) use -Og like other platforms instead of -O0 like before.

_DEBUG is likely unnecessary as this is a MSVC specific macro.

Not sure what DEBUG=1 does and why this is only defined on iOS.

Macros are probably just copy-pasted for other platform, I do not think it will do anything.

gdwarf-2 is likely also unnecessary, probably for using debuggers on old macOS versions (IIRC, default for current clang is drawf-4, and Xcode on macOS older than 10.10 do not support it).

Changed it in #66303.

akien-mga · 2022-09-22T08:11:50Z

platform/ios/detect.py

-    # Disable exceptions on non-tools (template) builds
-    if not env["tools"]:


I removed the check since we don't support editor build on iOS for now. Could be readded once it's needed.

akien-mga · 2022-09-22T08:28:17Z

platform/web/detect.py

-        elif env["optimize"] == "speed":
-            env.Append(CCFLAGS=["-O3"])
-            env.Append(LINKFLAGS=["-O3"])
-
-        if env["target"] == "release_debug":
-            # Retain function names for backtraces at the cost of file size.
-            env.Append(LINKFLAGS=["--profiling-funcs"])
-    else:  # "debug"
-        env.Append(CCFLAGS=["-O1", "-g"])
-        env.Append(LINKFLAGS=["-O1", "-g"])


@Faless By unifying the code with other platforms a couple changes were made here:

optimize=speed now uses -O3 for template-release but -O2 for template-debug/editor, like on other platforms. We optimize for size by default anyway so I figured this isn't a big deal?

-dev builds (ex target=debug) default to debug optimization level i.e. -Og, instead of -O1 as was set here. It seemed to me the change is fine as debug is not something we ship, and -Og is supposed to be fairly close to -O1, with the exception of those optimizations that hinder debugging.

platform/web/detect.py

akien-mga · 2022-09-22T08:36:18Z

platform/macos/detect.py

        if env["arch"] != "arm64":
            env.Prepend(CCFLAGS=["-msse2"])


Our handling of SSE2 is a bit arbitrary. We seem to forcefully define it for Windows MinGW and macOS x86_64 with target=release specifically, and not on any other platform:

$ rg msse2 platform/windows/detect.py 496: env.Append(CCFLAGS=["-msse2"]) platform/macos/detect.py 95: env.Prepend(CCFLAGS=["-msse2"]) modules/raycast/SCsub 70: env_raycast.Append(CPPFLAGS=["-msse2", "-mxsave"])

akien-mga · 2022-09-22T08:38:01Z

platform/web/emscripten_helpers.py

@@ -38,7 +38,7 @@ def create_engine_file(env, target, source, externs):


 def create_template_zip(env, js, wasm, worker, side):
-    binary_name = "godot.tools" if env["tools"] else "godot"
+    binary_name = "godot.editor" if env.editor_build else "godot"


@Faless Worth noting that I changed this too while at it. I've changed all cases of "\.tools" but I'm not sure that covers everything for the Web platform.

akien-mga · 2022-09-22T11:43:40Z

I noticed this size difference between a tools=yes target=debug build of the current master and target=editor-dev from this PR (which should be equivalent). I'll check what explains it.

-rwxr-xr-x 1 akien akien 935M Sep 22 09:25 godot.linuxbsd.editor-dev.x86_64*
-rwxr-xr-x 1 akien akien 697M Sep 22 13:41 godot.linuxbsd.tools.x86_64*

Edit: The difference is that for LinuxBSD we didn't set -Og for debug builds. Apparently -Og makes bigger binaries than no optimization / -O0? That's weird.

neikeq · 2022-09-22T12:15:05Z

Edit: The difference is that for LinuxBSD we didn't set -Og for debug builds. Apparently -Og makes bigger binaries than no optimization / -O0? That's weird.

The size difference doesn't sound weird to me. What confuses me is why we weren't using -Og before.

SConstruct

neikeq · 2022-09-22T12:35:07Z

The size difference doesn't sound weird to me.

I'm referring to this in particular:

-Og 

... It is a better choice than -O0 for producing
debuggable code because some compiler passes
that collect debug information are disabled at -O0.

I wouldn't trust that it's better for debugging, though. There have been problems in the past, like -Og optimizing out variables: https://stackoverflow.com/questions/31435771/variables-optimized-out-with-g-and-the-og-option#comment80796148_37830258. Perhaps that's why we are using -O0 instead.

aaronfranke

Looks really good overall.

SConstruct

Faless

Really good work.
The only remark I would like to add is about:

This one could possibly be split into a separate dev_build=yes option, which would bring down the target enum to "editor", "template-release", "template-debug", which are effectively what we ship. Thoughts? This idea is growing on me as I type it.

I wholeheartedly support doing this to avoid further confusion among the users.
And I think dev_build=yes should simply add the DEV_ENABLED flag and set default values for debug symbols and optimization levels so they can still be overridden.
By splitting dev_build=yes engine developers could simply set it the custom.py and have those defaults set for all targets (maybe ignore it if target is "template-release"?).

Faless · 2022-09-23T13:34:26Z

SConstruct

+    # Set optimize and debug_symbols flags.
+    # Needs to happen after configure to have `env.msvc` defined.


I think all the changes are good, and I agree having a optmize=custom that requires you to set CCFLAGS seems reasonable.

/FS seems to be a partial workaround to windows having issues with multiprocessing.
They say it helps though it doesn't actually fix all the problems: https://learn.microsoft.com/en-us/cpp/build/reference/fs-force-synchronous-pdb-writes?view=msvc-170
Seems safe to use.

Zireael07 · 2022-09-30T10:44:48Z

How about putting that in documentation? IIRC there's a whole section on compiling with Scons...

akien-mga · 2022-09-30T10:52:10Z

How about putting that in documentation? IIRC there's a whole section on compiling with Scons...

Sure, but one thing at a time. See #66242 (comment)

EricEzaM · 2022-09-30T10:57:26Z

methods.py

                    for plat_id in ModuleConfigs.PLATFORM_IDS
+                    for dev in ModuleConfig.DEV_SUFFIX


Typo? Should be ModuleConfigs. I get an error immediately running scons platform=windows dev_build=yes vsproj=yes -j10. NameError: name 'ModuleConfig' is not defined

Yes, feel free to make a PR if you can also validate that it actually works fine to build in VS.

Fixed in 166df08.

EricEzaM · 2022-09-30T11:15:50Z

Also cannot build vsproj:

[Initial build] scons: *** [godot.vcxproj] InternalError : Sizes of 'buildtarget' and 'variant' lists must be the same.
Traceback (most recent call last):
  File "C:\Users\Eric\AppData\Local\Programs\Python\Python38\lib\site-packages\SCons\Action.py", line 1318, in execute
    result = self.execfunction(target=target, source=rsources, env=env)
  File "C:\Users\Eric\AppData\Local\Programs\Python\Python38\lib\site-packages\SCons\Tool\msvs.py", line 1845, in GenerateProject
    GenerateDSP(dspfile, source, env)
  File "C:\Users\Eric\AppData\Local\Programs\Python\Python38\lib\site-packages\SCons\Tool\msvs.py", line 1795, in GenerateDSP
    g = _GenerateV10DSP(dspfile, source, env)
  File "C:\Users\Eric\AppData\Local\Programs\Python\Python38\lib\site-packages\SCons\Tool\msvs.py", line 1196, in __init__
    _DSPGenerator.__init__(self, dspfile, source, env)
  File "C:\Users\Eric\AppData\Local\Programs\Python\Python38\lib\site-packages\SCons\Tool\msvs.py", line 448, in __init__
    raise SCons.Errors.InternalError("Sizes of 'buildtarget' and 'variant' lists must be the same.")
SCons.Errors.InternalError: Sizes of 'buildtarget' and 'variant' lists must be the same.
scons: building terminated because of errors.

akien-mga · 2022-09-30T13:25:43Z

@EricEzaM Opened an issue to keep track of it: #66664.

See godotengine/godot#66242

…odotengine#66242

Introduced in godotengine/godot#66242 the `tools=yes/no` option was removed and merged into the `target` preset. Replace the `tools=yes/no` calls in the documentation to the new template target presets `editor`, `template_release` and `template_debug`.

) Document the Unification of tools/target build type configuration Introduced in godotengine/godot#66242 the `tools=yes/no` option was removed and merged into the `target` preset. Includes miscellaneous fixes identified during code review as well Co-authored-by: Raul Santos <raulsntos@gmail.com> Co-authored-by: Marius Hanl <66004280+Maran23@users.noreply.github.com> Co-authored-by: Clay John <claynjohn@gmail.com>

akien-mga requested review from bruvzg, Calinou, Faless, aaronfranke and a team September 22, 2022 07:09

akien-mga added enhancement topic:buildsystem breaks compat labels Sep 22, 2022

akien-mga added this to the 4.0 milestone Sep 22, 2022

bruvzg reviewed Sep 22, 2022

View reviewed changes

akien-mga force-pushed the scons-unify-tools-target branch from 49fff8a to a313013 Compare September 22, 2022 07:32

akien-mga commented Sep 22, 2022

View reviewed changes

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

akien-mga commented Sep 22, 2022

View reviewed changes

akien-mga force-pushed the scons-unify-tools-target branch from a313013 to 7aa7959 Compare September 22, 2022 08:46

neikeq reviewed Sep 22, 2022

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

aaronfranke approved these changes Sep 22, 2022

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

SConstruct Outdated Show resolved Hide resolved

This was referenced Sep 22, 2022

Fix various -Wmaybe-uninitialized warnings from GCC 12.2.1 #66248

Merged

Review use of -fomit-frame-pointer and -ftree-vectorize flags for optimized builds #66296

Closed

Faless approved these changes Sep 23, 2022

View reviewed changes

EricEzaM reviewed Sep 30, 2022

View reviewed changes

akien-mga mentioned this pull request Sep 30, 2022

Visual Studio vsproj generation broken after #66242 #66664

Closed

akien-mga mentioned this pull request Sep 30, 2022

fix: typo in ModuleConfigs name #66659

Closed

Bromeon added a commit to Bromeon/godot4-nightly that referenced this pull request Oct 1, 2022

Update to new scons config

683a509

See godotengine/godot#66242

nonunknown mentioned this pull request Oct 1, 2022

Crash when opening project after unify build PR #66703

Closed

m4gr3d mentioned this pull request Oct 5, 2022

Fix the gradle build configuration for the Android platform #66935

Merged

m4gr3d added a commit to m4gr3d/godot that referenced this pull request Oct 5, 2022

Fix the gradle build configuration for the Android platform following g…

3178b04

…odotengine#66242

Anutrix mentioned this pull request Oct 6, 2022

New 4.0 scons build options godotengine/godot-docs#6274

Closed

raulsntos mentioned this pull request Oct 7, 2022

Unable to build mono-glue on Godot 4 beta #65936

Closed

nyabinary mentioned this pull request Oct 10, 2022

[Outdated] godot-git chaotic-aur/packages#2015

Closed

1 task

akien-mga mentioned this pull request Oct 13, 2022

Unify target/tools arguments after upstream change godotengine/godot-build-scripts#72

Merged

NeroBurner mentioned this pull request Oct 15, 2022

Document the Unification of tools/target build type configuration godotengine/godot-docs#6316

Merged

Chaosus mentioned this pull request Oct 18, 2022

Documentation pages to add or update for Godot 4.0 godotengine/godot-docs#5121

Open

41 tasks

Calinou mentioned this pull request Oct 20, 2022

Allow compiling editor with target=release for aggressive optimization #60355

Closed

emmyleaf mentioned this pull request Nov 6, 2022

Update to Godot 4.0-beta4 codecat/godot-tbloader#35

Merged

aaronfranke mentioned this pull request Nov 21, 2022

Unify tools/target build type configuration to disambiguate "debug" (used for both game debugging and engine dev tools) godotengine/godot-proposals#3371

Closed

raulsntos mentioned this pull request Nov 21, 2022

ERROR: Condition "p_elem->_root != this" is true. #62970

Closed

Maran23 mentioned this pull request Nov 22, 2022

Change build flags in Godot 4 docs to current variants godotengine/godot-docs#6375

Closed

catwasd mentioned this pull request Jul 1, 2023

Fix dev=yes in unit testing guide godotengine/godot-docs#7596

Merged

		# Set optimize and debug_symbols flags.
		# Needs to happen after configure to have `env.msvc` defined.

		elif env["optimize"] == "size": # optimize for size
		env.Append(CCFLAGS=["-Oz"])

		env.Append(CCFLAGS=["-gdwarf-2"])
		env.Append(CPPDEFINES=["_DEBUG", ("DEBUG", 1)])

		# Disable exceptions on non-tools (template) builds
		if not env["tools"]:

		for plat_id in ModuleConfigs.PLATFORM_IDS
		for dev in ModuleConfig.DEV_SUFFIX

SCons: Unify tools/target build type configuration #66242

SCons: Unify tools/target build type configuration #66242

Conversation

akien-mga commented Sep 22, 2022 • edited Loading

New target presets

New dev_build option

Changed binary names

More flexible optimize and debug_symbols options

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Calinou Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Sep 22, 2022 • edited Loading

neikeq commented Sep 22, 2022

neikeq commented Sep 22, 2022 • edited Loading

aaronfranke left a comment

Choose a reason for hiding this comment

Faless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zireael07 commented Sep 30, 2022

akien-mga commented Sep 30, 2022

EricEzaM Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricEzaM commented Sep 30, 2022

akien-mga commented Sep 30, 2022

akien-mga commented Sep 22, 2022 •

edited

Loading

New `target` presets

New `dev_build` option

More flexible `optimize` and `debug_symbols` options

Calinou Sep 22, 2022 •

edited

Loading

akien-mga Sep 22, 2022 •

edited

Loading

akien-mga commented Sep 22, 2022 •

edited

Loading

neikeq commented Sep 22, 2022 •

edited

Loading

EricEzaM Sep 30, 2022 •

edited

Loading