Draft
Conversation
Introduces build/ directory organization: - CMake toolchain files for MinGW cross-compilation (mingw32.cmake, mingw64.cmake) - Version header template (ver_defs.h.in) for auto-generated version info - Windows manifest file (keeperfx.exe.manifest) - Make-based package build scripts for packaging workflow Foundation for modular build system with cross-platform support.
Introduces full MSVC/CMake build support alongside existing MinGW/Make: - CMakeLists.txt restructured with modular cmake modules - CMakePresets.json with MSVC (x86/x64), MinGW, and cross-compile presets - vcpkg manifest with custom registry for centijson, enet6, astronomy, libnatpmp - BuildTargets, Dependencies, Helpers, Platforms cmake modules - MSVC toolchain config (static runtime, C11/C++20, /J unsigned char) - compiler_compat.h with POSIX shims (strsep, access, mkdir, etc.) - tools/CMakeLists.txt with find_program fallback + download extraction - deps/CMakeLists.txt with MSVC find_package vs MinGW tarball paths
Fixes C99/C11 and POSIX constructs that MSVC does not support: - Replace designated initializers with positional/memset patterns - Fix mixed declarations and code (C89 requirement for MSVC /TC) - Add extern C guards for C headers included from C++ - Fix vector subscript out-of-bounds in spritesheet.cpp - Guard platform-specific includes (unistd.h, sys/stat.h) with ifdefs - Clean up bflib_basics.h macro definitions for MSVC compatibility
Adds ASan (+ UBSan for GCC/Clang) configuration via KEEPERFX_SANITIZERS option: - MSVC: /fsanitize=address with debug info and STL annotation workarounds - GCC/Clang: -fsanitize=address,undefined -fno-omit-frame-pointer - New CMakePresets for x86/x64 Debug and RelDebug with ASan enabled - Linux x64 ASan preset for native builds
Adds a GitHub Actions workflow that validates the MSVC/CMake build path on every push to master and PR. This catches regressions from developers who use the MinGW/Make build and may not test CMake compatibility. - Runs on windows-latest with MSVC x86 toolchain - Uses existing x86-windows-static-release preset - Leverages vcpkg binary caching for fast CI turnaround - Compile-only (no packaging or artifact upload)
There was a problem hiding this comment.
Pull request overview
Adds MSVC-focused build support via vcpkg + CMake (including ASan presets) and introduces portability shims to reduce reliance on GCC-only extensions.
Changes:
- Add/update vcpkg manifests + registries to pull in new deps (FFmpeg, OpenAL, enet6, etc.)
- Refactor CMake build into modular
build/cmake/modules/*and extendCMakePresets.jsonwith MSVC + ASan presets - Introduce MSVC compatibility fixes in C/C++ sources (attributes shims, include ordering, struct-field offset macros)
Reviewed changes
Copilot reviewed 58 out of 59 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vcpkg.json | Adds more third-party ports (FFmpeg, OpenAL, enet6, etc.) for vcpkg-based builds |
| vcpkg-configuration.json | Updates baseline and adds a custom git registry for missing ports |
| external/vcpkg | Adds vcpkg as a submodule at a pinned commit |
| CMakeLists.txt | Refactors root build to use modular CMake modules and generated version header |
| CMakePresets.json | Adds presets for MSVC x86/x64 static builds + ASan and Linux/cross presets |
| build/cmake/modules/Helpers.cmake | Adds centralized warning/link/system-lib helper functions |
| build/cmake/modules/Platforms.cmake | Adds platform options and sanitizer toggles (MSVC ASan, GCC/Clang ASan+UBSan) |
| build/cmake/modules/Dependencies.cmake | Centralizes find_package() calls and dependency status output |
| build/cmake/modules/BuildTargets.cmake | Defines targets (keeperfx, hvlog, tests) and source filtering logic |
| build/cmake/toolchains/msvc.cmake | Adds an MSVC toolchain file (flags/runtime/linker setup) |
| build/cmake/mingw32.cmake | Adds MinGW32 cross toolchain file |
| build/cmake/mingw64.cmake | Adds MinGW64 cross toolchain file |
| build/ver_defs.h.in | Adds a CMake template for generated version definitions |
| build/keeperfx.exe.manifest | Adds a Windows manifest (OS compatibility + DPI awareness) |
| deps/CMakeLists.txt | Adds vcpkg-vs-tarball resolution logic and MSVC find_package linking |
| tools/CMakeLists.txt | Adds PATH-first tool resolution + download/extract fallback for build tools |
| src/compiler_compat.h | Adds compiler/attribute shims and MSVC POSIX compatibility macros |
| src/bflib_basics.h | Switches GCC __attribute__ uses to portable compatibility macros |
| src/bflib_cpu.c | Adds MSVC __cpuid path, refactors cpuid helpers |
| src/bflib_fileio.c | Switches Windows directory creation to _mkdir |
| src/bflib_math.c | Removes DWORD usage for _BitScanReverse index var |
| src/bflib_fmvids.cpp | Guards GCC diagnostic pragma for non-GCC compilers |
| src/bflib_crash.c | Removes imagehlp.h include (keeps dbghelp/psapi) |
| src/net_portforward.cpp | Adds MSVC-friendly Windows header include ordering |
| src/cdrom.cpp | Adds MSVC-friendly Windows header include ordering |
| src/main.cpp | Exposes globals with extern "C" linkage for C translation units |
| src/lua_api_things.c | Changes method-table forward reference approach for MSVC friendliness |
| src/custom_sprites.c | Guards clang-specific pragmas with #ifdef __clang__ |
| src/spritesheet.cpp | Reworks sprite data extraction and adds trim_spritesheet() C API |
| src/config.h | Replaces GCC-only field() macro with MSVC-safe field_t/field_a, changes NamedFieldSet accessors |
| src/config.c | Updates config parsing/defaulting to use new NamedFieldSet accessors |
| src/config_terrain.c | Converts to field_t/field_a, adds macro push/pop around game, adds accessors |
| src/config_objects.c | Converts to field_t/field_a, adds macro push/pop around game, adds accessors |
| src/config_magic.c | Converts to field_t/field_a, adds macro push/pop around game, adds accessors |
| src/config_lenses.c | Converts to field_t, adds bounds checks, updates palette loading to use base+offset |
| src/config_effects.c | Converts to field_t, adds accessors, removes sentinel/gap-fill logic |
| src/config_cubes.c | Converts to field_t/field_a, adds macro push/pop around game, adds accessors |
| src/config_crtrstates.c | Converts to field_t, adds macro push/pop around game, adds accessors |
| src/config_compp.h | Moves comp_player_conf extern into extern "C" region for C++ consumers |
| src/config_compp.c | Converts to field_t/field_a, introduces NamedFieldSet base/count accessors |
| src/console_cmd.c | Enables strsep shim for MSVC builds too |
| .github/workflows/build-msvc-cmake.yml | Adds CI workflow to build MSVC via CMake presets |
| .gitmodules | Adds submodules (gfx, sfx, external/vcpkg) |
| .gitignore | Updates ignores for vcpkg installed dir, deploy output, Python caches, etc. |
| build/make/* | Adds/updates make packaging/tool rules aligned with submodule assets approach |
Comments suppressed due to low confidence (7)
tools/CMakeLists.txt:1
download_tool()always extracts using-E tar xzf, but the Windows downloads are.zipfiles (e.g.,png2ico-win-*.zip). Usingxzfis for gzip-compressed tarballs and will fail (or behave incorrectly) for zip archives. Use an extraction command that matches the archive type (e.g.,cmake -E tar xf ...for zip,xzffor.tar.gz), or have separate branches based on${TOOL_FILE}extension.
src/lua_api_things.c:1thing_methods_ptrisNULLuntilThing_register()runs, butthing_get_field()can now pass a null pointer intotry_get_c_method(), which is a potential crash or incorrect behavior iftry_get_c_method()doesn’t explicitly allowNULL. Prefer a compile-time solution: either (1) restore the original forward declarationstatic const struct luaL_Reg thing_methods[];(incomplete array + later definition is valid C), or (2) move thething_methods_arrdefinition abovething_get_field()so the function can reference a real array symbol instead of a runtime-initialized pointer.
src/lua_api_things.c:1thing_methods_ptrisNULLuntilThing_register()runs, butthing_get_field()can now pass a null pointer intotry_get_c_method(), which is a potential crash or incorrect behavior iftry_get_c_method()doesn’t explicitly allowNULL. Prefer a compile-time solution: either (1) restore the original forward declarationstatic const struct luaL_Reg thing_methods[];(incomplete array + later definition is valid C), or (2) move thething_methods_arrdefinition abovething_get_field()so the function can reference a real array symbol instead of a runtime-initialized pointer.
src/bflib_cpu.c:1- The previous
__i386__/__x86_64__guards around the inlineasm("cpuid")were removed, so non-x86 builds will now fail to compile (or worse, compile incorrectly). Reintroduce architecture guards around the GCC/Clang asm implementation (and ensurecpu_detect()doesn’t call CPUID on non-x86), with a safe fallback that leaves fields zeroed when CPUID isn’t available.
src/config.h:1 field_tencodes an integer offset by casting it tovoid*, which relies on implementation-defined integer↔pointer conversions and makes later arithmetic require casts like(ptrdiff_t)named_field->field. A more portable approach is to store offsets as an integer type (ptrdiff_torsize_t) in theNamedFieldstruct (e.g.,ptrdiff_t field_offset;) and keep the API consistently “offset-based” rather than “pointer-based”.
tools/CMakeLists.txt:1- These build-time downloads don’t pin integrity (no
EXPECTED_HASH) and will silently accept changed artifacts from the remote URL. Since this PR expands the set of downloaded binaries, consider addingEXPECTED_HASH SHA256=<...>for each tool archive (or using a reproducible, pinned artifact mechanism) to reduce supply-chain risk.
src/config_effects.c:1 - The previous logic that ensured
effect_desc(and similar *_desc arrays) had a contiguous, non-NULL “name” range up to the highest configured id was removed. If any downstream lookup (e.g.,get_id()-style functions) stops iterating at the firstname == NULL, gaps in configured IDs can now truncate lookups and cause IDs above the gap to become unreachable. Either restore the gap-filling/sentinel maintenance or ensure*_desctables are pre-initialized for all indices so there are no NULL “early terminators” in the usable range.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PieterVdc
reviewed
Mar 23, 2026
Add persistent binary caching via GitHub Packages NuGet alongside the existing x-gha ephemeral cache. vcpkg's ABI hash keys on compiler identity, triplet, and port SHA so matching is automatic. - Add permissions (contents: read, packages: write) - Register GitHub Packages NuGet source with GITHUB_TOKEN auth - Push to master/develop: readwrite; PRs: read-only - Trigger on develop branch alongside master
Re-add sentinel NULL entries and gap placeholder logic for load_effects, load_effectsgenerators, and load_effectelements that was inadvertently removed in the MSVC compatibility commit.
Expand build-msvc-cmake.yml into a full CI Build workflow that runs all three toolchains (MinGW x86, Linux x86_64, MSVC x86) on every PR push and push to master/develop. Prototype workflow remains unchanged — triggered only on ready_for_review/review_requested with artifact uploads.
MinGW is GCC but targets Windows where mkdir takes 1 argument. The guard must check the platform (_WIN32), not the compiler (KFX_COMPILER_GCC), to select the correct mkdir signature.
PieterVdc
approved these changes
Mar 24, 2026
Member
|
I put it to draft because after build it crashes to desktop. Peter Edit : He means it crashes when built via MingW |
PieterVdc
reviewed
Mar 30, 2026
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.
Adds support for CMAKE MSVC.
Introduces presets for Address Sanitizer.
Provides compatibility shims for GCC specific hacks