Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCons: Fix Ninja compilation with MSVC #90208

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Apr 4, 2024

Incorporates a handful of fixes brought up in the initial Ninja PR1, allowing MSVC builds to successfully compile.

Footnotes

  1. https://github.com/godotengine/godot/pull/89452#issuecomment-2028338110

@akien-mga
Copy link
Member

akien-mga commented Apr 4, 2024

This breaks GCC/Clang:

gcc -o thirdparty/mbedtls/library/aes.linuxbsd.editor.x86_64.o -c -std=gnu11 -pipe -O2 -Wall -Wextra -Wwrite-strings -Wno-unused-parameter -Wshadow -Wno-misleading-indentation -Wno-type-limits -Walloc-zero -Wduplicated-branches -Wduplicated-cond -Wstringop-overflow=4 -Wattribute-alias=2 -Werror -w -isystem thirdparty/glad -DTOOLS_ENABLED -DDEBUG_ENABLED -DNDEBUG -DNO_EDITOR_SPLASH -DSOWRAP_ENABLED -DTOUCH_ENABLED -DFONTCONFIG_ENABLED -DALSA_ENABLED -DALSAMIDI_ENABLED -DPULSEAUDIO_ENABLED -D_REENTRANT -DDBUS_ENABLED -DSPEECHD_ENABLED -DXKB_ENABLED -DJOYDEV_ENABLED -DUDEV_ENABLED -DLINUXBSD_ENABLED -DUNIX_ENABLED -D_FILE_OFFSET_BITS=64 -DX11_ENABLED -DLIBDECOR_ENABLED -DWAYLAND_ENABLED -DVULKAN_ENABLED -DRD_ENABLED -DGLES3_ENABLED -DCRASH_HANDLER_ENABLED -DMINIZIP_ENABLED -DBROTLI_ENABLED -DTHREADS_ENABLED -DCLIPPER2_ENABLED -DZSTD_STATIC_LINKING_ONLY -DUSE_VOLK -DVK_USE_PLATFORM_XLIB_KHR -DVK_USE_PLATFORM_WAYLAND_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 -Iplatform/linuxbsd -Ithirdparty/linuxbsd_headers/wayland -Ithirdparty/linuxbsd_headers -Iplatform/linuxbsd -I. thirdparty/mbedtls/library/aes.c
sh: 1: cannot create -Ithirdparty/mbedtls/include: Directory nonexistent

The angle brackets < and > are interpreted by the shell to read from/write to streams.

Comment on lines +24 to +26
config_path = "thirdparty/mbedtls/include/godot_core_mbedtls_config.h"
config_path = f"<{config_path}>" if env_crypto["ninja"] and env_crypto.msvc else f'\\"{config_path}\\"'
env_crypto.Append(CPPDEFINES=[("MBEDTLS_CONFIG_FILE", config_path)])
Copy link
Member

Choose a reason for hiding this comment

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

That's starting to be a bit convoluted...
I guess it's fine for now, but it would be worth figuring out why it's not being escaped properly by Ninja+MSVC, and how upstream mbedtls even expect this define to be passed in a way that won't break on some platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's not the prettiest solution & is very much a workaround. It should be reverted once the issue is ironed out in SCons, but that'll require some digging.

@akien-mga akien-mga merged commit d100888 into godotengine:master Apr 5, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Repiteo Repiteo deleted the scons/ninja-msvc branch May 8, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants