fix: stop compat.freetype and compat.zlib leaking GCC-only cflags to MSVC - #208
Merged
Conversation
…MSVC Both declared compiler flags that only GCC and Clang understand in their COMMON cflags, so MSVC received them verbatim. Found while building XRGUI (Sunrisepeak/xrgui#3) with mcpp on windows-latest with MSVC 14.5x: cl : Command line error D8021 : invalid numeric argument '/Wno-implicit-function-declaration' That is compat.freetype, and it is fatal -- the package cannot build on Windows at all. Every consumer goes down with it: harfbuzz's FreeType bridge, msdfgen's ext/import-font, and anything drawing text. compat.zlib's is the quieter and worse-behaved half: cl : Command line warning D9002 : ignoring unknown option '-include' cl : Command line warning D9024 : unrecognized source file type 'mcpp_zlib_config.h', object file assumed cl does not reject `-include`; it warns, drops the flag, and builds. So mcpp_zlib_config.h was never included and the package compiled with a silently different configuration than the recipe describes. On Windows that header is empty by construction (everything in it is behind `#if !defined(_WIN32)`), so nothing was actually miscompiled this time -- but the mechanism would not have told us if it had been. THE FIX Both descriptors already had per-OS sections; the platform-specific flags now live in them. compat.freetype common cflags keep only the FT_* defines. cl accepts -D, so those reach MSVC unchanged. -Wno-implicit-function-declaration -> linux + macosx -D_DARWIN_C_SOURCE -> macosx. It was on every platform, which was wrong on Linux too, just harmlessly so. compat.zlib -D_GNU_SOURCE -> linux; -include mcpp_zlib_config.h -> linux + macosx. Windows needs neither. Also switched to the two-element {"-include", "file"} form the rest of the index uses, rather than one string with an embedded space. VERIFIED Linux: tests/examples/freetype, msdfgen and harfbuzz all still pass (1 passed, 0 failed each) -- msdfgen and harfbuzz because they are freetype's consumers and a fix that only satisfies Windows would be no fix at all. Windows: the point of the change; this repo's windows workspace leg builds tests/examples/freetype, and it only rebuilds members a PR touches -- which is why the defect survived until a project outside this repo pulled freetype on MSVC. THE SAME DEFECT, NOT TOUCHED HERE Sweeping every descriptor for platform-specific flags in common cflags turns up four more, all with a windows xpm section, so all reachable on MSVC today: compat.lua -include mcpp_lua_platform_config.h compat.godot-cpp -include cstdlib compat.redis-plus-plus -include cstdint compat.eui-neo -include mcpp_eui_backends.h, -fno-char8_t Left alone deliberately: each needs its own judgement about what the MSVC equivalent should be (/FI, /Zc:char8_t-) or whether the flag is needed there at all, and I have no evidence about those packages on Windows the way I do for these two. Flagging rather than blind-editing. (The X11 packages also carry -D_GNU_SOURCE in common cflags. cl accepts -D and those packages are Linux-only, so it is untidy rather than broken.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both declared compiler flags that only GCC and Clang understand in their COMMON
cflags, so MSVC received them verbatim. Found while building XRGUI
(Sunrisepeak/xrgui#3) with mcpp on windows-latest with MSVC 14.5x:
That is compat.freetype, and it is fatal -- the package cannot build on Windows
at all. Every consumer goes down with it: harfbuzz's FreeType bridge, msdfgen's
ext/import-font, and anything drawing text.
compat.zlib's is the quieter and worse-behaved half:
cl does not reject
-include; it warns, drops the flag, and builds. Somcpp_zlib_config.h was never included and the package compiled with a silently
different configuration than the recipe describes. On Windows that header is
empty by construction (everything in it is behind
#if !defined(_WIN32)), sonothing was actually miscompiled this time -- but the mechanism would not have
told us if it had been.
THE FIX
Both descriptors already had per-OS sections; the platform-specific flags now
live in them.
compat.freetype common cflags keep only the FT_* defines. cl accepts -D, so
those reach MSVC unchanged.
-Wno-implicit-function-declaration -> linux + macosx
-D_DARWIN_C_SOURCE -> macosx. It was on every platform,
which was wrong on Linux too, just harmlessly so.
compat.zlib -D_GNU_SOURCE -> linux; -include mcpp_zlib_config.h ->
linux + macosx. Windows needs neither. Also switched to the
two-element {"-include", "file"} form the rest of the index
uses, rather than one string with an embedded space.
VERIFIED
Linux: tests/examples/freetype, msdfgen and harfbuzz all still pass (1 passed,
0 failed each) -- msdfgen and harfbuzz because they are freetype's consumers and
a fix that only satisfies Windows would be no fix at all.
Windows: the point of the change; this repo's windows workspace leg builds
tests/examples/freetype, and it only rebuilds members a PR touches -- which is
why the defect survived until a project outside this repo pulled freetype on
MSVC.
THE SAME DEFECT, NOT TOUCHED HERE
Sweeping every descriptor for platform-specific flags in common cflags turns up
four more, all with a windows xpm section, so all reachable on MSVC today:
compat.lua -include mcpp_lua_platform_config.h
compat.godot-cpp -include cstdlib
compat.redis-plus-plus -include cstdint
compat.eui-neo -include mcpp_eui_backends.h, -fno-char8_t
Left alone deliberately: each needs its own judgement about what the MSVC
equivalent should be (/FI, /Zc:char8_t-) or whether the flag is needed there at
all, and I have no evidence about those packages on Windows the way I do for
these two. Flagging rather than blind-editing.
(The X11 packages also carry -D_GNU_SOURCE in common cflags. cl accepts -D and
those packages are Linux-only, so it is untidy rather than broken.)