Skip to content

Comments

MacOs ci#4552

Draft
PieterVdc wants to merge 30 commits intodkfans:masterfrom
PieterVdc:mac_ci
Draft

MacOs ci#4552
PieterVdc wants to merge 30 commits intodkfans:masterfrom
PieterVdc:mac_ci

Conversation

@PieterVdc
Copy link
Member

completely untested, and atm could probably use extra cleanup, but it produces a mac build

@AI-Guru
Copy link

AI-Guru commented Feb 21, 2026

Merging this would be very appreciated!

@PieterVdc PieterVdc marked this pull request as ready for review February 21, 2026 11:02
Copilot AI review requested due to automatic review settings February 21, 2026 11:02
@PieterVdc PieterVdc marked this pull request as draft February 21, 2026 11:02
@PieterVdc
Copy link
Member Author

Merging this would be very appreciated!

to be clear, I have no idea if it even works, don't own any macOs devices to test it on,
I triggered it a new build, so if you're willing to check if it even starts once it's done, would be good to know
https://github.com/dkfans/keeperfx/actions/runs/22255657213

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a macOS (Apple Silicon) CI build path and app bundling, along with build-system and source tweaks intended to improve cross-platform compilation (notably on macOS).

Changes:

  • Add a macOS GitHub Actions job that builds an arm64 binary and uploads a zipped .app bundle artifact.
  • Extend linux.mk to support platform/arch detection and to build/download third-party deps in a more flexible way (including macOS).
  • Apply various C/C header adjustments for portability (switch-case scoping, signal defines, packing pragmas, string-compare compatibility, etc.).

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
tools/macos_bundle.sh New script to construct a .app bundle and copy/link Homebrew dylib dependencies.
src/thing_stats.c Adds braces in switch cases to allow declarations cleanly.
src/pre_inc.h Adds strings.h include and strnicmp mapping.
src/player_instances.h Removes #pragma pack directives.
src/player_computer.h Removes #pragma pack directives.
src/lvl_script_lib.c Adds a scoped block + break in a switch default path.
src/gui_topmsg.h Removes #pragma pack directives.
src/frontend.h Removes #pragma pack(1) at file start (affects struct packing).
src/front_lvlstats.h Removes #pragma pack directives.
src/config.h Removes #pragma pack directives.
src/config.c Minor whitespace change.
src/bflib_vidraw.h Replaces max() macro usage in scaling-step macros with explicit ternaries.
src/bflib_sprite.h Removes #pragma pack directives.
src/bflib_network.h Switches packing to #pragma pack(push, 1) / pop scoped around wire structs.
src/bflib_guibtns.h Removes #pragma pack directives.
src/bflib_filelst.h Removes #pragma pack directives.
src/bflib_crash.c Broadens POSIX signal handling beyond Linux and guards non-portable signals.
src/bflib_cpu.c Refactors CPUID helpers under an x86-only guard.
src/ariadne.c Removes an unused counter variable in a nav-tree helper.
linux.mk Adds PLATFORM/ARCH handling, mac support, pkg-config usage changes, and dependency build/download logic.
.github/workflows/build-prototype.yml Adds a macOS build+bundle+upload job.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +46 to +48
ifeq ($(PLATFORM),linux)
DEPS_SUFFIX ?= lin64
endif
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

DEPS_SUFFIX is hard-coded to lin64 for all Linux builds, but this makefile now supports ARCH=arm64. If someone builds on Linux arm64 with DEPS_DOWNLOAD=1, it will try to download *-lin64.tar.gz (x86_64) and fail or produce unusable binaries. Consider making DEPS_SUFFIX depend on both PLATFORM and ARCH (e.g., lin64 vs linarm64) or disabling downloads for non-x86_64 until an appropriate artifact exists.

Suggested change
ifeq ($(PLATFORM),linux)
DEPS_SUFFIX ?= lin64
endif
ifeq ($(PLATFORM),linux)
ifeq ($(ARCH),x86_64)
DEPS_SUFFIX ?= lin64
else
# No prebuilt dependency archive defined for this architecture; disable downloads.
DEPS_DOWNLOAD ?= 0
endif
endif

Copilot uses AI. Check for mistakes.
Comment on lines 11 to +16

#include <strings.h>

#ifndef strnicmp
#define strnicmp strncasecmp
#endif
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

pre_inc.h now unconditionally includes <strings.h>, which is not available on all toolchains (notably MSVC, and some Windows environments). This can break Windows builds. Guard the include (and any strncasecmp mapping) behind #if !defined(_WIN32)/#if defined(unix) or move the <strings.h> include to the same platform-conditional area where strncasecmp/strnicmp are defined.

Suggested change
#include <strings.h>
#ifndef strnicmp
#define strnicmp strncasecmp
#endif
#if !defined(_WIN32)
#include <strings.h>
#ifndef strnicmp
#define strnicmp strncasecmp
#endif
#endif

Copilot uses AI. Check for mistakes.
Comment on lines 39 to 46
#define PANEL_SPRITES_COUNT 514
#define FRONTEND_FONTS_COUNT 4
// After that much milliseconds in main menu, demo is started
#define MNU_DEMO_IDLE_TIME 30000
/******************************************************************************/
#pragma pack(1)

enum DemoItem_Kind {
DIK_PlaySmkVideo,
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

#pragma pack(1) was removed, but this header still defines struct NetMessage { ... } // sizeof = 0x41. Without packing, the compiler will typically insert padding after plyr_idx, making the struct larger than 0x41 and potentially breaking any code that relies on the documented size / binary layout. Reintroduce packing around struct NetMessage (preferably via #pragma pack(push, 1)/#pragma pack(pop) scoped to just the structs that need it), or otherwise adjust the struct so its size/layout is stable.

Copilot uses AI. Check for mistakes.
Comment on lines +349 to +357
KFX_INCLUDES = \
-Ideps/centijson/include \
-Ideps/centitoml \
-Ideps/astronomy/include \
-Ideps/enet6/include \
$(shell pkg-config --cflags-only-I sdl2) \
$(shell pkg-config --cflags-only-I SDL2_image) \
$(shell pkg-config --cflags-only-I SDL2_mixer) \
$(shell pkg-config --cflags-only-I SDL2_net) \
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

KFX_INCLUDES is defined with a recursive = assignment and contains many $(shell pkg-config ...) calls. Because KFX_INCLUDES is expanded for every compile/link command, this will re-run pkg-config repeatedly and can significantly slow builds/CI. Consider precomputing pkg-config results once using := (or separate PKG_CFLAGS := $(shell ...) / PKG_LIBS := $(shell ...) variables) so the shell commands execute only once per make invocation.

Copilot uses AI. Check for mistakes.
Comment on lines +472 to +473
git clone --depth "$(DEPS_CLONE_DEPTH)" "$(DEPS_ASTRONOMY_REPO)" "$(DEPS_BUILD_DIR)/astronomy"; \
if [ -n "$(DEPS_ASTRONOMY_REF)" ]; then git -C "$(DEPS_BUILD_DIR)/astronomy" checkout "$(DEPS_ASTRONOMY_REF)"; fi; \
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

The git clone step for $(DEPS_ASTRONOMY_REPO) fetches an external repository without pinning to an immutable commit or verifying integrity. If that upstream repo or the network path is compromised, an attacker could inject malicious code into the built binary simply by changing the remote default branch. Pin this dependency to a specific commit (and/or vendor it) and add checksum or signature verification for downloaded sources to prevent supply chain compromise.

Copilot uses AI. Check for mistakes.
Comment on lines +512 to +513
git clone --depth "$(DEPS_CLONE_DEPTH)" "$(DEPS_CENTIJSON_REPO)" "$(DEPS_BUILD_DIR)/centijson"; \
if [ -n "$(DEPS_CENTIJSON_REF)" ]; then git -C "$(DEPS_BUILD_DIR)/centijson" checkout "$(DEPS_CENTIJSON_REF)"; fi; \
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

The git clone step for $(DEPS_CENTIJSON_REPO) pulls code from a third-party GitHub repository without locking to a specific commit or validating integrity. A compromise or malicious update on that remote could silently change what is built here, allowing injected code to run whenever the resulting binary is executed. Pin this dependency to an immutable revision and/or verify checksums or signatures of the fetched sources to harden the build against supply chain attacks.

Copilot uses AI. Check for mistakes.
Comment on lines +542 to +543
git clone --depth "$(DEPS_CLONE_DEPTH)" "$(DEPS_ENET6_REPO)" "$(DEPS_BUILD_DIR)/enet6"; \
if [ -n "$(DEPS_ENET6_REF)" ]; then git -C "$(DEPS_BUILD_DIR)/enet6" checkout "$(DEPS_ENET6_REF)"; fi; \
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

The git clone for $(DEPS_ENET6_REPO) similarly fetches code from an external repository without commit pinning or integrity checks. This allows any change on the remote default branch (including by an attacker who compromises that repo) to alter the code linked into your binary without changes in this repo. Pin to a specific commit or release and add checksum/signature verification for the downloaded sources to mitigate this supply chain risk.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants