godot icon indicating copy to clipboard operation
godot copied to clipboard

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

Open deralmas opened this issue 1 year ago • 15 comments

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:

  1. 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).
  2. 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.

deralmas avatar Mar 13 '24 17:03 deralmas

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

deralmas avatar Mar 13 '24 17:03 deralmas

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.

deralmas avatar Mar 13 '24 18:03 deralmas

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.

Calinou avatar Mar 13 '24 19:03 Calinou

@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

deralmas avatar Mar 13 '24 19:03 deralmas

@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[^1]. 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 )

[^1]: 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."

deralmas avatar Mar 13 '24 23:03 deralmas

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 :)

deralmas avatar Mar 14 '24 16:03 deralmas

Can you do a small rebase against master? Thanks!

fire avatar Mar 15 '24 13:03 fire

@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[^1].

[^1]: 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.

deralmas avatar Mar 15 '24 14:03 deralmas

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 avatar Mar 15 '24 15:03 fire

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

fire avatar Mar 15 '24 15:03 fire

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

akien-mga avatar Mar 15 '24 15:03 akien-mga

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

Seems to error in different places.

[Initial build] Linking Program bin\godot.windows.editor.x86_64.llvm.exe ...
=====
ld.lld: error: undefined symbol: OS::get_exit_code() const
>>> referenced by platform\windows\godot_windows.windows.editor.x86_64.llvm.o:(widechar_main(int, wchar_t**))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(vtable for OS_Windows)

ld.lld: error: undefined symbol: ScriptDebugger::set_depth(int)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(HandlerRoutine(unsigned long))

ld.lld: error: undefined symbol: ScriptDebugger::set_lines_left(int)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(HandlerRoutine(unsigned long))

ld.lld: error: undefined symbol: String::utf16() const
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::alert(String const&, String const&))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::alert(String const&, String const&))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(_error_handler(void*, char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType))
>>> referenced 111 more times

ld.lld: error: undefined symbol: Char16String::get_data() const
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::alert(String const&, String const&))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::alert(String const&, String const&))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(debug_dynamic_library_check_dependencies(String const&, String const&, HashSet<String, HashMapHasherDefault, HashMapComparatorDefault<String>>&, HashSet<String, HashMapHasherDefault, HashMapComparatorDefault<String>>&))
>>> referenced 122 more times

ld.lld: error: undefined symbol: Memory::free_static(void*, bool)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::alert(String const&, String const&))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::alert(String const&, String const&))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::initialize())
>>> referenced 58637 more times

ld.lld: error: undefined symbol: NetSocketPosix::make_default()
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::initialize())

ld.lld: error: undefined symbol: operator new(unsigned long long, char const*)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::initialize())
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::OS_Windows(HINSTANCE__*))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::OS_Windows(HINSTANCE__*))
>>> referenced 3684 more times

ld.lld: error: undefined symbol: IPUnix::make_default()
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::initialize())

ld.lld: error: undefined symbol: is_print_verbose_enabled()
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::initialize())
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::initialize())
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::OS_Windows(HINSTANCE__*))
>>> referenced 117 more times

ld.lld: error: undefined symbol: Variant::Variant(char const*)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::initialize())
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::get_memory_info() const)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::get_memory_info() const)
>>> referenced 1421 more times

ld.lld: error: undefined symbol: stringify_variants(Variant const&)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::initialize())
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::OS_Windows(HINSTANCE__*))
>>> referenced by platform\windows\display_server_windows.windows.editor.x86_64.llvm.o:(DisplayServerWindows::_update_tablet_ctx(String const&, String const&))
>>> referenced 137 more times

ld.lld: error: undefined symbol: __print_line(String const&)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::initialize())
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::OS_Windows(HINSTANCE__*))
>>> referenced by platform\windows\display_server_windows.windows.editor.x86_64.llvm.o:(DisplayServerWindows::_update_tablet_ctx(String const&, String const&))
>>> referenced 135 more times

ld.lld: error: undefined symbol: Variant::_clear_internal()
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::initialize())
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::get_memory_info() const)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::get_memory_info() const)
>>> referenced 8522 more times

ld.lld: error: undefined symbol: String::utf8(char const*, int)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(_error_handler(void*, char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(_error_handler(void*, char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(_error_handler(void*, char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType))
>>> referenced 212 more times

ld.lld: error: undefined symbol: String::String(char const*)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(_error_handler(void*, char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(_error_handler(void*, char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(debug_dynamic_library_check_dependencies(String const&, String const&, HashSet<String, HashMapHasherDefault, HashMapComparatorDefault<String>>&, HashSet<String, HashMapHasherDefault, HashMapComparatorDefault<String>>&))
>>> referenced 21628 more times

ld.lld: error: undefined symbol: String::operator+(String const&) const
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(_error_handler(void*, char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(_error_handler(void*, char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(_error_handler(void*, char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType))
>>> referenced 2493 more times

ld.lld: error: undefined symbol: itos(long long)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(_error_handler(void*, char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(format_error_message(unsigned long))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::move_to_trash(String const&))
>>> referenced 344 more times

ld.lld: error: undefined symbol: String::operator+=(char const*)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(_error_handler(void*, char const*, char const*, int, char const*, char const*, bool, ErrorHandlerType))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(OS_Windows::open_dynamic_library(String const&, void*&, bool, String*))
>>> referenced by libmain.windows.editor.x86_64.llvm.a(main.windows.editor.x86_64.llvm.o):(Main::setup(char const*, int, char**, bool))
>>> referenced 58 more times

ld.lld: error: undefined symbol: Memory::alloc_static(unsigned long long, bool)
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(HashMap<long long, OS_Windows::ProcessInfo, HashMapHasherDefault, HashMapComparatorDefault<long long>, DefaultTypedAllocator<HashMapElement<long long, OS_Windows::ProcessInfo>>>::insert(long long const&, OS_Windows::ProcessInfo const&, bool))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(HashMap<long long, OS_Windows::ProcessInfo, HashMapHasherDefault, HashMapComparatorDefault<long long>, DefaultTypedAllocator<HashMapElement<long long, OS_Windows::ProcessInfo>>>::insert(long long const&, OS_Windows::ProcessInfo const&, bool))
>>> referenced by platform\windows\os_windows.windows.editor.x86_64.llvm.o:(HashSet<String, HashMapHasherDefault, HashMapComparatorDefault<String>>::insert(String const&))
>>> referenced 1307 more times

ld.lld: error: too many errors emitted, stopping now (use --error-limit=0 to see all errors)
clang-18: error: linker command failed with exit code 1 (use -v to see invocation)

=====
scons: *** [bin\godot.windows.editor.x86_64.llvm.exe] Error 1
scons: building terminated because of errors.
[Time elapsed: 00:02:35.360]
PS D:\20240307_windows\other\VSK_godot> scons use_mingw=yes use_llvm=yes ninja=no scu_build=yes

fire avatar Mar 15 '24 15:03 fire

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 avatar Mar 15 '24 16:03 fire

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

fire avatar Mar 15 '24 16:03 fire

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: https://github.com/godotengine/godot/blob/99ff024f78f65ba0bc54fb409cfeca43ba2008fe/thirdparty/mbedtls/include/mbedtls/aes.h#L34

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?

Calinou avatar Mar 24 '24 17:03 Calinou

@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.

deralmas avatar Mar 25 '24 07:03 deralmas

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

  1. 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^1, but it's a manageable workaround while ninja currently lacks the functionality; though run_ninja_env.bat will need to be added to .gitignore.
  2. 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()
     

Repiteo avatar Mar 30 '24 17:03 Repiteo

Thanks!

akien-mga avatar Apr 04 '24 12:04 akien-mga