SCons: Enable the experimental Ninja backend and minimize timestamp changes to generated code #89452

Riteo · 2024-03-13T17:40:37Z

This PR implements everything needed to have initial support for the experimental ninja build backend for SCons documented here: https://scons.org/doc/production/HTML/scons-user/ch27s02.html

Unfortunately, just enabling the backend (which was trivial) was not enough, as ninja is timestamp-based and the code generators kept rewriting (and thus retouching) everything for no good reason, tripping it up.

This PR is thus a patchset of 2 related but atomic commits:

A commit that makes all generators I could find extremely conservative with timestamp changes, through a new method in methods.py called write_file_if_needed, which compares the file's contents to the target string before writing to it. The SCU generators had to get a special treatment, where they keep track of every file considered and use that as a reference for what to not delete when globbing everything (thus only deleting any "stale" file).
The actual ninja-enabling commit, which is quite small as it adds just the new option and the few lines needed to turn the backend on.

To use it, you'll need to have ninja installed and a python module named ninja, which offers nothing more than the ninja_syntax.py logic bundled by ninja.

To get it, either install the ninja python package like the docs recommend:

# In a virtualenv, or "python" is the native executable:
python -m pip install ninja

# Windows using Python launcher:
py -m pip install ninja

# Anaconda:
conda install -c conda-forge ninja

or just put the two files needed in your local site-packages directory.

Here's a zip which makes SCons use the system version of ninja by bundling the upstream ninja_syntax.py and a stub __init__.py that does absolutely nothing (it just needs to be there to be recognized by SCons): ninja.zip

(yeah, I know, this is a bit unnecessary, but that's how it works. We can improve things upstream if this approach becomes more popular)

After that, just add the ninja=yes option when invoking scons. SCons will generate a build.ninja and quit. After that, just call ninja with whatever flags you might need and enjoy :D

Also, ideally you won't have to call scons never again, unless you want to change some build flag, as ninja will detect any (timestamp-based) change to any SCons build script and invoke everything needed to regenerate itself.

While experimenting sometimes it would trip itself in an infinite loop, probably due to some backwards-and-forwards that changed SConstruct but not its actual contents. In that case, start a clean build by deleting build.ninja and the .ninja folder and try again.

Be aware that not everything is transferred properly for some reason, such as ~~whether to build compile_commands.json (it does so by default), the linker choice (it uses ld.bfd on my machine for some reason) and~~ the number of jobs (you'll have to manually specify -j while invoking ninja or it will use whatever's the default, which on my machine is 10).

Edit: more precisely, custom.py is not working completely for some reason. Passing manually the linker or other things will work, with the exception of compile_db and the jobs as it looks like they might be treated specially by SCons. I think that this workflow does not really allow it in the first place by design, so I would not rely on it.

Edit2: I had an old version of scons (4.3.0). The latest at the time of writing, 4.6.0, imports custom.py fine. It also skips building compile_commands.json correctly now.

That said, since this is still marked as experimental, I would not be surprised if some of those were some upstream limitations, so I would consider this good enough for now and open to testing.

Riteo · 2024-03-13T17:55:11Z

Ooops sorry I did some mistakes while cleaning up the generation method and messed some things up, one sec...

Riteo · 2024-03-13T18:02:13Z

There, should be fixed :D

Edit: nope, not yet :/

Edit 2: ding ding ding! I had to add an handler for FileNotFoundError as I could not find a more clever way of doing it. I hoped that r+ would open a file for reading and creation but in the end this turned out to be the simplest solution I could find.

Calinou · 2024-03-13T19:18:54Z

and the number of jobs (you'll have to manually specify -j while invoking ninja).

The good news though is that Ninja uses all CPU threads by default (unlike make), so builds won't be very slow if you forget to specify it.

Riteo · 2024-03-13T19:26:45Z

@Calinou yeah on my machine the default is 10, which is a bit more of my actual threads but still greater than 1, so that's something :D

Calinou

Tested locally, it works as expected.

I much prefer Ninja's single-line output as well (with subsequent compilation lines replacing previous lines), so this is also an upgrade from an usability perspective.

I agree it makes sense to push users to run Ninja directly to avoid the cost of spawning a Python interpreter and SCons every time you want to compile Godot. CMake has the luxury of having a fast cmake --build command that redirects to ninja in a few milliseconds since it's a C++ program, but we don't 🙂

A small issue I noticed is that if you run SCons and Ninja once, then run SCons with no changes and run ninja again, Ninja will check 100+ files instead of exiting early. I'm not sure if this can be fixed though.

scons
ninja  # Initial build.
ninja  # Null build (very fast).
scons
ninja  # This one takes longer than the null build,
       # though not as long as the initial build.

Benchmark

OS: Fedora 39 (GCC 13.2.1, ccache 4.8.2, mold 2.4.0)
CPU: Intel Core i9-13900K (performance governor)
SSD: Solidigm P44 Pro 2 TB

SCons command line:

scons -j32 platform=linuxbsd linker=mold optimize=none fast_unsafe=yes progress=no debug_symbols=yes builtin_embree=no builtin_enet=no builtin_freetype=no builtin_graphite=no builtin_harfbuzz=no builtin_libogg=no builtin_libpng=no builtin_libtheora=no builtin_libvorbis=no builtin_libwebp=no builtin_mbedtls=no builtin_miniupnpc=no builtin_pcre2=no builtin_zlib=no builtin_zstd=no scu_build=all

All tests done with a warm CCache to isolate buildsystem performance.

Null build

SCons:

  Time (mean ± σ):      1.841 s ±  0.016 s    [User: 1.662 s, System: 0.168 s]
  Range (min … max):    1.820 s …  1.871 s    10 runs

Ninja:

  Time (mean ± σ):     104.2 ms ±   3.8 ms    [User: 79.8 ms, System: 23.3 ms]
  Range (min … max):    98.4 ms … 110.3 ms    27 runs

main.cpp change (`sd '//' '// ' main/main.cpp`¹):

SCons:

  Time (mean ± σ):      4.864 s ±  0.035 s    [User: 4.834 s, System: 0.616 s]
  Range (min … max):    4.808 s …  4.924 s    10 runs

Ninja:

  Time (mean ± σ):      3.174 s ±  0.031 s    [User: 2.648 s, System: 0.237 s]
  Range (min … max):    3.090 s …  3.199 s    10 runs

editor_node.h change (`sd '//' '// ' editor/editor_node.h`¹)

SCons:

  Time (mean ± σ):      3.660 s ±  0.035 s    [User: 12.658 s, System: 2.558 s]
  Range (min … max):    3.618 s …  3.737 s    10 runs

Ninja:

  Time (mean ± σ):      2.547 s ±  0.044 s    [User: 14.285 s, System: 3.283 s]
  Range (min … max):    2.504 s …  2.651 s    10 runs

https://github.com/chmln/sd ↩ ↩²

Riteo · 2024-03-13T23:27:51Z

@Calinou thanks a lot for testing!

A small issue I noticed is that if you run SCons and Ninja once, then run SCons with no changes and run ninja again, Ninja will check 100+ files instead of exiting early. I'm not sure if this can be fixed though.

Mh, that's interesting. I've run ninja with the -dexplain flag which, as the name implies, explains why it decides to rebuild some file and the only meaningful message that I get on a fresh scons->ninja chain is:

ninja explain: output compile_commands.json older than most recent input build.ninja (1710369857680153344 vs 1710370007912264671)

It doesn't look like the compile_commands.json thing is really a dependency on anything, but as you can notice from the message calling scons regenerates the whole build.ninja. Perhaps ninja invalidates a bunch of stuff when it notices an inconsistency like this. I'm not sure.

I'm not sure if this can be fixed though.

Yeah I don't think we can fix this easily if at all, although one has to call scons relatively rarely, as it's just there for setting the compilation options, see below for some more findings.

BTW I finally found out why in the world I couldn't get it to build with mold: apparently custom.py gets (partially?) ignored, relying entirely on the arguments passed from the CLI.

I don't think that the SCons folks can do much either; TIWAGOS, but I think that this is by design as ninja has to invoke SCons to (re)generate any "dynamic" source file (e.g. anything *.gen.* like the Wayland protocol wrappers) and it has to know the settings in advance or bad things can happen.

Also, it's an explicit non-goal to have configuration handling in ninja¹. They look to be extremely minimalist and opinionated, which is what gives them their speed and determinism.

So, I think that we might have to live with that and communicate to the users that this workflow is a bit different from what we're used to. It might not be that bad though, as after all one has to call SCons only once per configuration change (and any SConstruct/SCsub change is taken in account by ninja too)

I think that we can get this to an usable state as-is if we become aware of its quirks and limitations. The current speed boost sounds already very appetizing IMO.

Eventually, we could discuss this further with the SCons folks; I suppose that they would appreciate the feedback and perhaps we could help get a bunch of improvements upstream too, such as with ninja detection ~~or perhaps a way to disable compile_commands.json for people who don't need it, as it looks hardcoded.~~ Edit: I had an old SCons version, it's not hardcoded anymore.

Hopefully this shines some further light on this backend, as there's unfortunately not much documentation, despite this being surprisingly rock solid for being experimental (we're a huge SCons project and it built first try, requiring only a few tweaks to have perfect incremental rebuilds, that's impressive :D )

From https://ninja-build.org/manual.html#_design_goals: "Some explicit non-goals: [...] * build-time customization of the build. Options belong in the program that generates the ninja files." ↩

SConstruct

Riteo · 2024-03-14T16:28:01Z

So folks I did some more testing and realized I had a 2 year old version of SCons...

The latest, 4.6.0 is faster, better, harder and stronger: custom.py works again (although it's a bit inconsistent as it exports the actual commands and the cli invocation but not the whole environment, so there's chance for some unreliability in the result) and compile_commands.json is not built anymore by default.

In other words, this is even more stable than I reported before :)

fire · 2024-03-15T13:21:29Z

Can you do a small rebase against master? Thanks!

Previously, all of the code generation routines would just needlessly write the same files over and over, even when not needed. This became a problem with the advent of the experimental ninja backend for SCons, which can be trivially enabled with a few lines of code and relies on timestamp changes, making it thus impractical.

Riteo · 2024-03-15T14:48:37Z

@fire rebased!

I also added a version check like with CompileDB, used the native setting for disabling the ninja autorun and renamed the new flaky file whitelist to paths_to_keep because that word is a bit... Controversial? And apparently native speakers feel way more uneasy with that word than non-native speakers like me¹.

Based from some previous informal discussions, I have no idea if that's really the case but in this case changing it was painless and, personally, a bit more consistent with how I like my variables named. ↩

akien-mga

Looks great!

platform_methods.py

With this option turned on, if properly set up, SCons generates a `build.ninja` file and quits. To actually build the engine, the user can then call `ninja` with whatever options they might prefer (not everything is yet transferred properly to this new generated file). Ideally, the scons file should never be called again, as ninja automatically detects any SCons build script change and invokes the required commands to regenerate itself. This approach speeds up incremental builds considerably, as it limits SCons to code generation and uses ninja's extremely fast timestamp-based file change detector.

fire · 2024-03-15T15:39:01Z

On windows 11, scons ninja=yes failed.

scons use_mingw=yes use_llvm=yes ninja=yes scu_build=yes

Found some warnings and a failure on Windows 11 with python 3.12.

In file included from platform/windows/rendering_context_driver_vulkan_windows.cpp:35:
platform/windows/rendering_context_driver_vulkan_windows.h:55:50: warning: class with destructor marked 'final' cannot be inherited from [-Wfinal-dtor-non-final-class]
   41 | class RenderingContextDriverVulkanWindows : public RenderingContextDriverVulkan {
      |                                           final
   42 | private:
   43 |         const char *_get_platform_surface_extension() const override final;
   44 |
   45 | protected:
   46 |         SurfaceID surface_create(const void *p_platform_data) override final;
   47 |
   48 | public:
   49 |         struct WindowPlatformData {
   50 |                 HWND window;
   51 |                 HINSTANCE instance;
   52 |         };
   53 |
   54 |         RenderingContextDriverVulkanWindows();
   55 |         ~RenderingContextDriverVulkanWindows() override final;
      |                                                         ^
platform/windows/rendering_context_driver_vulkan_windows.h:41:7: note: mark 'RenderingContextDriverVulkanWindows' as 'final' to silence this warning
   41 | class RenderingContextDriverVulkanWindows : public RenderingContextDriverVulkan {
      |       ^
1 warning generated.
[583/1820] Compiling platform\windows\display_server_windows.windows.editor.x86_64.llvm.o
In file included from platform/windows/display_server_windows.cpp:43:
platform/windows/rendering_context_driver_vulkan_windows.h:55:50: warning: class with destructor marked 'final' cannot be inherited from [-Wfinal-dtor-non-final-class]
   41 | class RenderingContextDriverVulkanWindows : public RenderingContextDriverVulkan {
      |                                           final
   42 | private:
   43 |         const char *_get_platform_surface_extension() const override final;
   44 |
   45 | protected:
   46 |         SurfaceID surface_create(const void *p_platform_data) override final;
   47 |
   48 | public:
   49 |         struct WindowPlatformData {
   50 |                 HWND window;
   51 |                 HINSTANCE instance;
   52 |         };
   53 |
   54 |         RenderingContextDriverVulkanWindows();
   55 |         ~RenderingContextDriverVulkanWindows() override final;
      |                                                         ^
platform/windows/rendering_context_driver_vulkan_windows.h:41:7: note: mark 'RenderingContextDriverVulkanWindows' as 'final' to silence this warning
   41 | class RenderingContextDriverVulkanWindows : public RenderingContextDriverVulkan {
      |       ^
1 warning generated.
FAILED: thirdparty/mbedtls/library/constant_time.windows.editor.x86_64.llvm.o
x86_64-w64-mingw32-clang "-o" "thirdparty/mbedtls/library/constant_time.windows.editor.x86_64.llvm.o" "-c" "-std=gnu11" "-O2" "-Wall" "-Wshadow-field-in-constructor" "-Wshadow-uncaptured-local" "-Wno-ordered-compare-function-pointers" "-MMD" "-MF" "thirdparty/mbedtls/library/constant_time.windows.editor.x86_64.llvm.o.d" "-w" "-isystem" "thirdparty/glad" "-DTOOLS_ENABLED" "-DDEBUG_ENABLED" "-DEIGEN_MPL2_ONLY" "-DNDEBUG" "-DNO_EDITOR_SPLASH" "-DWINDOWS_ENABLED" "-DWASAPI_ENABLED" "-DWINMIDI_ENABLED" "-DWINVER=0x0601" "-D_WIN32_WINNT=0x0601" "-DVULKAN_ENABLED" "-DRD_ENABLED" "-DGLES3_ENABLED" "-DMINGW_ENABLED" "-DMINGW_HAS_SECURE_API=1" "-DMINIZIP_ENABLED" "-DBROTLI_ENABLED" "-DTHREADS_ENABLED" "-DCLIPPER2_ENABLED" "-DZSTD_STATIC_LINKING_ONLY" "-DUSE_VOLK" "-DVK_USE_PLATFORM_WIN32_KHR" "-DGLAD_ENABLED" "-DEGL_ENABLED" "-DGODOT_MODULE" "-DMBEDTLS_CONFIG_FILE=/"thirdparty/mbedtls/include/godot_module_mbedtls_config.h/"" "-Ithirdparty/mbedtls/include" "-Ithirdparty/libpng" "-Ithirdparty/volk" "-Ithirdparty/vulkan" "-Ithirdparty/vulkan/include" "-Ithirdparty/zstd" "-Ithirdparty/zlib" "-Ithirdparty/clipper2/include" "-Ithirdparty/brotli/include" "-Ithirdparty/angle/include" "-Iplatform/windows" "-I." "thirdparty/mbedtls/library/constant_time.c"
In file included from thirdparty/mbedtls/library/constant_time.c:13:
thirdparty/mbedtls/library/common.h:15:10: error: expected "FILENAME" or <FILENAME>
   15 | #include MBEDTLS_CONFIG_FILE
      |          ^
<command line>:26:29: note: expanded from macro 'MBEDTLS_CONFIG_FILE'
   26 | #define MBEDTLS_CONFIG_FILE /thirdparty/mbedtls/include/godot_module_mbedtls_config.h/
      |                             ^
In file included from thirdparty/mbedtls/library/constant_time.c:16:
thirdparty/mbedtls/include/mbedtls/error.h:16:10: error: expected "FILENAME" or <FILENAME>
   16 | #include MBEDTLS_CONFIG_FILE
      |          ^
<command line>:26:29: note: expanded from macro 'MBEDTLS_CONFIG_FILE'
   26 | #define MBEDTLS_CONFIG_FILE /thirdparty/mbedtls/include/godot_module_mbedtls_config.h/
      |                             ^
In file included from thirdparty/mbedtls/library/constant_time.c:17:
thirdparty/mbedtls/include/mbedtls/platform_util.h:17:10: error: expected "FILENAME" or <FILENAME>
   17 | #include MBEDTLS_CONFIG_FILE
      |          ^
<command line>:26:29: note: expanded from macro 'MBEDTLS_CONFIG_FILE'
   26 | #define MBEDTLS_CONFIG_FILE /thirdparty/mbedtls/include/godot_module_mbedtls_config.h/
      |                             ^
3 errors generated.

fire · 2024-03-15T15:46:24Z

To be clear this isn't a blocker and we can work on it later as long as one platform works.

akien-mga · 2024-03-15T15:52:29Z

Can you check that there is no breakage if you don't use ninja=yes?

fire · 2024-03-15T16:00:13Z

Let me try harder to see if I can get a build on any platform.

Edited:

Build failure on windows using msvc.

https://gist.github.com/fire/69086e702fc02bca20c515a50b518cdc

I wasn't able to install python package ninja easily, but I'll try mac.

fire · 2024-03-15T16:14:28Z

I have to do some errands so I won't be able test currently.

Calinou · 2024-03-24T17:35:14Z

I've tested this PR on Windows 11 23H2 with MSVC 2022, but I get a build failure when calling ninja:

C:\Users\Hugo\Documents\Git\godotengine\godot>ninja
[2/2403] Compiling core\config\project_settings.windows.editor.x86_64.obj
FAILED: core/config/project_settings.windows.editor.x86_64.obj
cl "/Focore/config/project_settings.windows.editor.x86_64.obj" "/c" "core/config/project_settings.cpp" "/TP" "/std:c++17" "/nologo" "/MT" "/Gd" "/GR" "/utf-8" "/bigobj" "/Zi" "/FS" "/Od" "/W3" "/w34458" "/wd4100" "/wd4127" "/wd4201" "/wd4244" "/wd4245" "/wd4267" "/wd4305" "/wd4514" "/wd4714" "/wd4820" "/showIncludes" "/DTOOLS_ENABLED" "/DDEBUG_ENABLED" "/DNDEBUG" "/DNO_EDITOR_SPLASH" "/DWINDOWS_ENABLED" "/DWASAPI_ENABLED" "/DWINMIDI_ENABLED" "/DTYPED_METHOD_BIND" "/DWIN32" "/DWINVER=0x0601" "/D_WIN32_WINNT=0x0601" "/DNOMINMAX" "/D_WIN64" "/DVULKAN_ENABLED" "/DRD_ENABLED" "/DGLES3_ENABLED" "/D_HAS_EXCEPTIONS=0" "/DMINIZIP_ENABLED" "/DBROTLI_ENABLED" "/DTHREADS_ENABLED" "/DCLIPPER2_ENABLED" "/DZSTD_STATIC_LINKING_ONLY" "/Ithirdparty/zstd" "/Ithirdparty/zlib" "/Ithirdparty/clipper2/include" "/Ithirdparty/brotli/include" "/Ithirdparty/angle/include" "/Iplatform/windows" "/I."
CreateProcess failed: The system cannot find the file specified.
[3/2403] Compiling core\extension\extension_api_dump.windows.editor.x86_64.obj
FAILED: core/extension/extension_api_dump.windows.editor.x86_64.obj
cl "/Focore/extension/extension_api_dump.windows.editor.x86_64.obj" "/c" "core/extension/extension_api_dump.cpp" "/TP" "/std:c++17" "/nologo" "/MT" "/Gd" "/GR" "/utf-8" "/bigobj" "/Zi" "/FS" "/Od" "/W3" "/w34458" "/wd4100" "/wd4127" "/wd4201" "/wd4244" "/wd4245" "/wd4267" "/wd4305" "/wd4514" "/wd4714" "/wd4820" "/showIncludes" "/DTOOLS_ENABLED" "/DDEBUG_ENABLED" "/DNDEBUG" "/DNO_EDITOR_SPLASH" "/DWINDOWS_ENABLED" "/DWASAPI_ENABLED" "/DWINMIDI_ENABLED" "/DTYPED_METHOD_BIND" "/DWIN32" "/DWINVER=0x0601" "/D_WIN32_WINNT=0x0601" "/DNOMINMAX" "/D_WIN64" "/DVULKAN_ENABLED" "/DRD_ENABLED" "/DGLES3_ENABLED" "/D_HAS_EXCEPTIONS=0" "/DMINIZIP_ENABLED" "/DBROTLI_ENABLED" "/DTHREADS_ENABLED" "/DCLIPPER2_ENABLED" "/DZSTD_STATIC_LINKING_ONLY" "/Ithirdparty/zstd" "/Ithirdparty/zlib" "/Ithirdparty/clipper2/include" "/Ithirdparty/brotli/include" "/Ithirdparty/angle/include" "/Iplatform/windows" "/I."
CreateProcess failed: The system cannot find the file specified.
[4/2403] Compiling editor\animation_track_editor.windows.editor.x86_64.obj
FAILED: editor/animation_track_editor.windows.editor.x86_64.obj
cl "/Foeditor/animation_track_editor.windows.editor.x86_64.obj" "/c" "editor/animation_track_editor.cpp" "/TP" "/std:c++17" "/nologo" "/MT" "/Gd" "/GR" "/utf-8" "/bigobj" "/Zi" "/FS" "/Od" "/W3" "/w34458" "/wd4100" "/wd4127" "/wd4201" "/wd4244" "/wd4245" "/wd4267" "/wd4305" "/wd4514" "/wd4714" "/wd4820" "/showIncludes" "/DTOOLS_ENABLED" "/DDEBUG_ENABLED" "/DNDEBUG" "/DNO_EDITOR_SPLASH" "/DWINDOWS_ENABLED" "/DWASAPI_ENABLED" "/DWINMIDI_ENABLED" "/DTYPED_METHOD_BIND" "/DWIN32" "/DWINVER=0x0601" "/D_WIN32_WINNT=0x0601" "/DNOMINMAX" "/D_WIN64" "/DVULKAN_ENABLED" "/DRD_ENABLED" "/DGLES3_ENABLED" "/D_HAS_EXCEPTIONS=0" "/DMINIZIP_ENABLED" "/DBROTLI_ENABLED" "/DTHREADS_ENABLED" "/DCLIPPER2_ENABLED" "/DZSTD_STATIC_LINKING_ONLY" "/DUSE_VOLK" "/DVK_USE_PLATFORM_WIN32_KHR" "/DGLAD_ENABLED" "/DEGL_ENABLED" "/Ithirdparty/freetype/include" "/Ithirdparty/libpng" "/Ithirdparty/glad" "/Ithirdparty/volk" "/Ithirdparty/vulkan" "/Ithirdparty/vulkan/include" "/Ithirdparty/zstd" "/Ithirdparty/zlib" "/Ithirdparty/clipper2/include" "/Ithirdparty/brotli/include" "/Ithirdparty/angle/include" "/Iplatform/windows" "/I."
CreateProcess failed: The system cannot find the file specified.
...

It works when I start x64 Native Tools Command Prompt for VS 2022 in the start menu, so the automatic MSVC detection isn't working anymore. Is this something we could contribute to Ninja upstream? It helps a lot with MSVC usability, especially for people less familiar with compiling C++ code on Windows.

Afterwards, there's a build failure early on:

[5/1813] Compiling core\crypto\crypto_core.windows.editor.x86_64.obj
FAILED: core/crypto/crypto_core.windows.editor.x86_64.obj
cl "/Focore/crypto/crypto_core.windows.editor.x86_64.obj" "/c" "core/crypto/crypto_core.cpp" "/TP" "/std:c++17" "/nologo" "/MT" "/Gd" "/GR" "/utf-8" "/bigobj" "/Zi" "/FS" "/Od" "/W3" "/w34458" "/wd4100" "/wd4127" "/wd4201" "/wd4244" "/wd4245" "/wd4267" "/wd4305" "/wd4514" "/wd4714" "/wd4820" "/showIncludes" "/DTOOLS_ENABLED" "/DDEBUG_ENABLED" "/DNDEBUG" "/DNO_EDITOR_SPLASH" "/DWINDOWS_ENABLED" "/DWASAPI_ENABLED" "/DWINMIDI_ENABLED" "/DTYPED_METHOD_BIND" "/DWIN32" "/DWINVER=0x0601" "/D_WIN32_WINNT=0x0601" "/DNOMINMAX" "/D_WIN64" "/DVULKAN_ENABLED" "/DRD_ENABLED" "/DGLES3_ENABLED" "/D_HAS_EXCEPTIONS=0" "/DMINIZIP_ENABLED" "/DBROTLI_ENABLED" "/DTHREADS_ENABLED" "/DCLIPPER2_ENABLED" "/DZSTD_STATIC_LINKING_ONLY" "/DMBEDTLS_CONFIG_FILE=/"thirdparty/mbedtls/include/godot_module_mbedtls_config.h/"" "/Ithirdparty/mbedtls/include" "/Ithirdparty/zstd" "/Ithirdparty/zlib" "/Ithirdparty/clipper2/include" "/Ithirdparty/brotli/include" "/Ithirdparty/angle/include" "/Iplatform/windows" "/I."
C:\Users\Hugo\Documents\Git\godotengine\godot\thirdparty\mbedtls\include\mbedtls/aes.h(34): error C2006: '#include': expected "FILENAME" or <FILENAME>
C:\Users\Hugo\Documents\Git\godotengine\godot\thirdparty\mbedtls\include\mbedtls/aes.h(34): fatal error C1083: Cannot open include file: '': No such file or directory

This is the code in question:

godot/thirdparty/mbedtls/include/mbedtls/aes.h

Line 34 in 99ff024

#include MBEDTLS_CONFIG_FILE

The generated paths in build.ninja look like this:

      "/DMBEDTLS_CONFIG_FILE=/"thirdparty/mbedtls/include/godot_module_mbedtls_config.h/"" $

Maybe it's an issue with how quotes are escaped?

Riteo · 2024-03-25T07:17:49Z

@Calinou FTR, on my linux machine the same line is escaped properly:

      "-DMBEDTLS_CONFIG_FILE=\"thirdparty/mbedtls/include/godot_module_mbedtls_config.h\"" $

I suppose that some platform code is incorrectly swapping \ and / or something (Windows uses a different path separator after all, so that makes some sense). Not sure where it's doing that though.

I don't think that this is blocking, as @fire said. This looks like an upstream issue to me.
Edit: no idea where I took that from. They didn't say that, they only said it wasn't blocking. Oops, I think I took that from calinou, sorry.

Might be worth an issue ticket.

Repiteo · 2024-03-30T17:28:57Z

Been testing locally on my Windows machine, and two things stood out:

SCons automatically generates run_ninja_env.bat in the repo root, and running that runs ninja with the proper environment variables set. The SCons repo admits this isn't the prettiest solution¹, but it's a manageable workaround while ninja currently lacks the functionality; though run_ninja_env.bat will need to be added to .gitignore.
The DMBEDTLS_CONFIG_FILE quote-escape issue can be circumvented by substituting angle brackets instead. By changing the relevant SCsub defines from '\\"some_header.h\\"' to "<some_header.h>", everything compiles as expected. This obviously doesn't "fix" the core issue, but this at least allows for MSVC functionality out of the gate.

Diff

diff --git a/.gitignore b/.gitignore
index 46dcf84b43..3946cc96e5 100644
--- a/.gitignore
+++ b/.gitignore
@@ -38,6 +38,7 @@ platform/windows/godot_res.res
 # Ninja build files
 build.ninja
 .ninja
+run_ninja_env.bat
 
 # Generated by Godot binary
 .import/
diff --git a/core/crypto/SCsub b/core/crypto/SCsub
index ac79e10d19..093cb5a2f4 100644
--- a/core/crypto/SCsub
+++ b/core/crypto/SCsub
@@ -21,9 +21,7 @@ if is_builtin or not has_module:
 # to make a "light" build with only the necessary mbedtls files.
 if not has_module:
     # Minimal mbedTLS config file
-    env_crypto.Append(
-        CPPDEFINES=[("MBEDTLS_CONFIG_FILE", '\\"thirdparty/mbedtls/include/godot_core_mbedtls_config.h\\"')]
-    )
+    env_crypto.Append(CPPDEFINES=[("MBEDTLS_CONFIG_FILE", "<thirdparty/mbedtls/include/godot_core_mbedtls_config.h>")])
     # Build minimal mbedTLS library (MD5/SHA/Base64/AES).
     env_thirdparty = env_crypto.Clone()
     env_thirdparty.disable_warnings()
@@ -47,7 +45,7 @@ if not has_module:
 elif is_builtin:
     # Module mbedTLS config file
     env_crypto.Append(
-        CPPDEFINES=[("MBEDTLS_CONFIG_FILE", '\\"thirdparty/mbedtls/include/godot_module_mbedtls_config.h\\"')]
+        CPPDEFINES=[("MBEDTLS_CONFIG_FILE", "<thirdparty/mbedtls/include/godot_module_mbedtls_config.h>")]
     )
     # Needed to force rebuilding the core files when the configuration file is updated.
     thirdparty_obj = ["#thirdparty/mbedtls/include/godot_module_mbedtls_config.h"]
diff --git a/modules/mbedtls/SCsub b/modules/mbedtls/SCsub
index 4b8f65d8ff..dc6b214552 100644
--- a/modules/mbedtls/SCsub
+++ b/modules/mbedtls/SCsub
@@ -101,7 +101,7 @@ if env["builtin_mbedtls"]:
 
     env_mbed_tls.Prepend(CPPPATH=["#thirdparty/mbedtls/include/"])
     env_mbed_tls.Append(
-        CPPDEFINES=[("MBEDTLS_CONFIG_FILE", '\\"thirdparty/mbedtls/include/godot_module_mbedtls_config.h\\"')]
+        CPPDEFINES=[("MBEDTLS_CONFIG_FILE", "<thirdparty/mbedtls/include/godot_module_mbedtls_config.h>")]
     )
 
     env_thirdparty = env_mbed_tls.Clone()

https://github.com/SCons/scons/blob/7e6484275b04e17660b36ebbf38b2b67d9d1c3c7/SCons/Tool/ninja/__init__.py#L77-L89 ↩

Repiteo

Despite the above hiccups, this is still a rock-solid starting point for getting ninja officially integrated! That, and the added checks/conditionals on generated files is lovely QOL.

akien-mga · 2024-04-04T12:53:11Z

Thanks!

PR godotengine#89452 made assumptions on comparing paths as strings which doesn't work when composing them as POSIX paths (`/`) but processing them on NT (`\`, `\\`).

Riteo added enhancement topic:buildsystem needs testing labels Mar 13, 2024

Riteo added this to the 4.x milestone Mar 13, 2024

Riteo requested review from a team as code owners March 13, 2024 17:40

Riteo force-pushed the name-a-better-duo branch from 72db703 to c8bc666 Compare March 13, 2024 18:02

Riteo force-pushed the name-a-better-duo branch 2 times, most recently from 0dd31db to d4501a2 Compare March 13, 2024 19:03

Calinou added the performance label Mar 13, 2024

Calinou reviewed Mar 13, 2024

View reviewed changes

Repiteo reviewed Mar 14, 2024

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

Riteo force-pushed the name-a-better-duo branch 2 times, most recently from b5dea26 to 5c3e245 Compare March 14, 2024 08:17

This was referenced Mar 14, 2024

SCons: Disable verbose output for SCU build #89483

Merged

added ninja tool for scons #52793

Closed

Riteo force-pushed the name-a-better-duo branch from 5c3e245 to 76618e6 Compare March 15, 2024 14:46

akien-mga approved these changes Mar 15, 2024

View reviewed changes

platform_methods.py Outdated Show resolved Hide resolved

akien-mga modified the milestones: 4.x, 4.3 Mar 15, 2024

Riteo force-pushed the name-a-better-duo branch from 76618e6 to 55558fb Compare March 15, 2024 15:05

This comment was marked as outdated.

Sign in to view

Calinou mentioned this pull request Mar 28, 2024

Add target=gdscript #89962

Draft

Repiteo approved these changes Mar 30, 2024

View reviewed changes

akien-mga merged commit 7fa97f3 into godotengine:master Apr 4, 2024
16 checks passed

akien-mga mentioned this pull request Apr 9, 2024

SCons: Fix SCU build regression on Windows #90450

Merged

mihe mentioned this pull request Aug 13, 2024

Expose more Ninja-related build options #95504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCons: Enable the experimental Ninja backend and minimize timestamp changes to generated code #89452

SCons: Enable the experimental Ninja backend and minimize timestamp changes to generated code #89452

Riteo commented Mar 13, 2024 •

edited

Loading

Riteo commented Mar 13, 2024

Riteo commented Mar 13, 2024 •

edited

Loading

Calinou commented Mar 13, 2024 •

edited

Loading

Riteo commented Mar 13, 2024

Calinou left a comment •

edited

Loading

Riteo commented Mar 13, 2024 •

edited

Loading

Riteo commented Mar 14, 2024

fire commented Mar 15, 2024

Riteo commented Mar 15, 2024 •

edited

Loading

akien-mga left a comment

fire commented Mar 15, 2024 •

edited

Loading

fire commented Mar 15, 2024

akien-mga commented Mar 15, 2024

This comment was marked as outdated.

fire commented Mar 15, 2024 •

edited

Loading

fire commented Mar 15, 2024

Calinou commented Mar 24, 2024 •

edited

Loading

Riteo commented Mar 25, 2024 •

edited

Loading

Repiteo commented Mar 30, 2024

Repiteo left a comment

akien-mga commented Apr 4, 2024

SCons: Enable the experimental Ninja backend and minimize timestamp changes to generated code #89452

SCons: Enable the experimental Ninja backend and minimize timestamp changes to generated code #89452

Conversation

Riteo commented Mar 13, 2024 • edited Loading

Riteo commented Mar 13, 2024

Riteo commented Mar 13, 2024 • edited Loading

Calinou commented Mar 13, 2024 • edited Loading

Riteo commented Mar 13, 2024

Calinou left a comment • edited Loading

Choose a reason for hiding this comment

Benchmark

Null build

main.cpp change (sd '//' '// ' main/main.cpp1):

editor_node.h change (sd '//' '// ' editor/editor_node.h1)

Footnotes

Riteo commented Mar 13, 2024 • edited Loading

Footnotes

Riteo commented Mar 14, 2024

fire commented Mar 15, 2024

Riteo commented Mar 15, 2024 • edited Loading

Footnotes

akien-mga left a comment

Choose a reason for hiding this comment

fire commented Mar 15, 2024 • edited Loading

fire commented Mar 15, 2024

akien-mga commented Mar 15, 2024

This comment was marked as outdated.

fire commented Mar 15, 2024 • edited Loading

fire commented Mar 15, 2024

Calinou commented Mar 24, 2024 • edited Loading

Riteo commented Mar 25, 2024 • edited Loading

Repiteo commented Mar 30, 2024

Footnotes

Repiteo left a comment

Choose a reason for hiding this comment

akien-mga commented Apr 4, 2024

Riteo commented Mar 13, 2024 •

edited

Loading

Riteo commented Mar 13, 2024 •

edited

Loading

Calinou commented Mar 13, 2024 •

edited

Loading

Calinou left a comment •

edited

Loading

main.cpp change (`sd '//' '// ' main/main.cpp`¹):

editor_node.h change (`sd '//' '// ' editor/editor_node.h`¹)

Riteo commented Mar 13, 2024 •

edited

Loading

Riteo commented Mar 15, 2024 •

edited

Loading

fire commented Mar 15, 2024 •

edited

Loading

fire commented Mar 15, 2024 •

edited

Loading

Calinou commented Mar 24, 2024 •

edited

Loading

Riteo commented Mar 25, 2024 •

edited

Loading