[OmarAglan]

• Takes over ci(build): Add MinGW-w64 i686 cross-compilation to CI workflow #2163 by @JohnsterID (agreed with @xezon in that PR)

This adds the MinGW-w64 i686 jobs from #2163 to CI, rebased onto main. It builds Generals and GeneralsMD on ubuntu-latest with the mingw-w64-i686 and mingw-w64-i686-debug presets. The profile preset is excluded because it uses MSVC inline assembly. JohnsterID's two commits are unchanged and keep him as author.

On current main, both executables compiled with MinGW but failed to link:

• OSDisplaySetBusyState, ReloadAllTextures and MOTDSystem are used by the game engine library but implemented in the engine device library. CMake only knew that the device depends on the engine, so the engine came last on the link line. GNU ld reads static libraries once, in order, so it never went back to the device library for them.
• WWDownload calls accept and listen but never linked ws2_32. It only came in through GameSpy, which is earlier on the link line.

MSVC's linker searches all libraries regardless of order, so MSVC builds were not affected.

• g_gameengine and z_gameengine declare INTERFACE g_gameenginedevice and INTERFACE z_gameenginedevice. CMake supports this cycle between static libraries and repeats them on the link line.
• corei_wwdownload links ws2_32.

Tested locally with msys2 MinGW-w64 (GCC 16.2) and the mingw-w64-i686 preset: generalsv.exe and generalszh.exe both link

AI disclosure

Used Clude Code with Opus 5.5 to make the CMake fix, i reviewed and tested the change.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-18T11:29:46.081000Z	d8152a7	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

• Last reviewed commit: 79addaa
• Latest commit, not reviewed: b36276c

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• Resume reviews
• Trigger review

No actionable comments were generated in the recent review.

Walkthrough

The CMake targets now declare additional interface link dependencies. CI invokes a reusable MinGW-w64 workflow to build Generals and GeneralsMD and upload build artifacts.

Changes

MinGW-w64 build support

Layer / File(s)	Summary
Interface link dependencies Core/Libraries/Source/WWVegas/WWDownload/CMakeLists.txt, Generals/Code/GameEngine/CMakeLists.txt, GeneralsMD/Code/GameEngine/CMakeLists.txt	corei_wwdownload links ws2_32 through its interface. The Generals and GeneralsMD engine targets declare their respective device libraries as interface dependencies.
Reusable MinGW build workflow .github/workflows/reusable-build-mingw.yml	Adds required game and preset inputs, installs the MinGW toolchain and Wine dependencies, caches CMake dependencies, configures and builds the selected game, and uploads matching build artifacts for 30 days.
CI build jobs .github/workflows/ci.yml	Adds separate Generals and GeneralsMD jobs that run on workflow dispatch or relevant game and shared-file changes. Both use the mingw-w64-i686 preset. The profiling preset is excluded because GCC cannot compile its MSVC inline assembly.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other

Sequence Diagram(s)

sequenceDiagram
  participant CI as CI workflow
  participant Build as Reusable MinGW workflow
  participant CMake as CMake
  participant Artifacts as Artifact upload
  CI->>Build: Invoke with game and preset
  Build->>CMake: Configure selected game
  CMake->>Build: Build game target
  Build->>Artifacts: Upload executables, DLLs, and debug symbols

Loading

Suggested labels: CI

Suggested reviewers: xezon, bobtista

Merge Risk: Low · previous review 79add

The MinGW builds add bounded exposure by retaining read-only checkout credentials unnecessarily. Disabling persistence is advisable, but no credential upload or concrete build failure was established.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title names the main change: adding MinGW-w64 i686 cross-compilation to the CI workflow. The raw summary confirms new MinGW-w64 release-build jobs for Generals and GeneralsMD that call a reusable …
Description check		The description relates directly to the changeset. It explains that the PR adds MinGW-w64 i686 jobs for Generals and GeneralsMD to CI, excludes the profile preset because it uses MSVC inline assembly,…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the main change: adding MinGW-w64 i686 cross-compilation to the CI workflow. The raw summary confirms new MinGW-w64 release-build jobs for Generals and GeneralsMD that call a reusable MinGW build workflow. The title is specific and does not describe unrelated work. The CMake link fixes are not in the title, but the title does not need to cover every detail.

Full details: Description check

Explanation

The description relates directly to the changeset. It explains that the PR adds MinGW-w64 i686 jobs for Generals and GeneralsMD to CI, excludes the profile preset because it uses MSVC inline assembly, and describes the CMake link fixes for the engine/device libraries and ws2_32 in WWDownload. It also gives local test results and an AI disclosure.

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 473c0098-0a39-455a-ad18-5697ee97b2f0

Commits

Reviewing files that changed from the base of the PR and between 9078bac and 79addaa.

Files selected for processing: 2

• .github/workflows/ci.yml
• .github/workflows/reusable-build-mingw.yml

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issue was found.

Summary

Adds MinGW-w64 release builds for both games and fixes their library links.

• CI builds MinGW versions of Generals and GeneralsMD on Ubuntu.
• The MinGW builds now include the settings needed to compile and link both games.

_{Reviews (5) · Last reviewed commit: "build(cmake): Enable SSE2 for MinGW-w64 ..." · Reviewed by Greptile}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟡 Minor · Avoid the cross-release WineHQ fallback. · ci.yml:227

.github/workflows/ci.yml:227
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Avoid the cross-release WineHQ fallback.

The workflow uses winehq-jammy.sources whenever the codename-specific download fails. On a non-Jammy runner, this can make apt use a repository for a different Ubuntu release. Fail when the source is unavailable or allow only explicitly supported codename mappings.

The downloaded .sources file already supplies apt’s Signed-By binding for the Jammy path, so the issue is not that apt ignores the downloaded key. Do not remove Wine setup solely because the executable targets do not link Wine: MinGW uses widl and Wine headers to generate the browser interfaces.

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2f90689d-1afd-4007-bc5a-603ad020fdb1

📥 Commits

Reviewing files that changed from the base of the PR and between d8152a7 and 9078bac.

📒 Files selected for processing: 1

• .github/workflows/ci.yml

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

[coderabbitai]

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Set persist-credentials: false on the checkout steps.

These two checkout steps leave the GITHUB_TOKEN in the local git config. The jobs run build scripts and Wine packages, and none of them needs to push to git. The credential adds exposure with no benefit. This is the zizmor artipacked finding. Disable credential persistence.

🔒️ Proposed fix

         uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
+        with:
+          persist-credentials: false

Apply this change at both Line 214 and Line 302.

Also applies to: 302-302

🧰 Tools

🪛 zizmor (1.30.1)

[warning] 213-214: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false

(artipacked)

Source: Linters/SAST tools

[OmarAglan]

outside the scope of this pr

[coderabbitai]

@OmarAglan Understood. Would you like me to open a follow-up issue for the checkout credential hardening?

You are interacting with an AI system.

[OmarAglan]

@xezon what do you think?

[xezon]

Unfortunately I do not know what this means. Is this a concern for this github repository?

[coderabbitai]

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

[OmarAglan]

Unfortunately I do not know what this means. Is this a concern for this github repository?

actions/checkout saves the job's GitHub token in .git/config so that later steps can run git push. persist-credentials: false stops it from saving the token. It only matters if a step uploads the .git folder or runs code that reads it. Here the token is read only (ci.yml sets permissions: contents: read), the artifacts only contain exe, dll and debug files, and no other checkout in this repo sets it. So it is not a concern here.

If we want it anyway, it should be done for all workflows in its own PR.

[Skyaero42]

More and more builds are being added to the already strained CI workflow. CI rebuilds with every push in every PR.
We need a better solutions for the CI workflow before more builds are being added.

Two suggestions

1. Break up the CI pipeline in a 'base' build (e.g. vc6 and win32 release only) and an extended build. The 'base' build is always triggered, the 'extended' build only manually. Both are required for a merge. master branch always builds extended
2. There is one CI pipeline, but it is triggered by hand for all PR's. Only master builds automatically.

[OmarAglan]

More and more builds are being added to the already strained CI workflow. CI rebuilds with every push in every PR. We need a better solutions for the CI workflow before more builds are being added.

Two suggestions

1. Break up the CI pipeline in a 'base' build (e.g. vc6 and win32 release only) and an extended build. The 'base' build is always triggered, the 'extended' build only manually. Both are required for a merge. master branch always builds extended
2. There is one CI pipeline, but it is triggered by hand for all PR's. Only master builds automatically.

good point, as in the last run the 4 MinGW jobs took 64 of 178 runner minutes, and at 16 to 19 minutes each they are the slowest jobs, so they also make the pr wait longer.

i can cut this pr down to the release preset only (so it will be 2 jobs instead of 4)

as far as i know the debug preset adds little for a compile and link check. A base and extended split sounds good to me, but it changes the whole workflow, so I think it is better as its own pr once you and @xezon agree on the approach. after that we can have MinGW to the extended set.

what do you think?

[xezon]

Can we remove this dependency? How many are there?

[OmarAglan]

i counted at least 3 calls that go from the engine into the device library

• OSDisplaySetBusyState in GameLogic.cpp
• ReloadAllTextures in ScriptEngine.cpp
• MOTDSystem in FunctionLexicon.cpp

The msvc linker searches all libraries, so it finds them in any order
instead the gnu ld reads each static library once in order, so it needs this line

removing these calls means moving them behind interfaces in engine code

can we rather do that in a separate pr?

[xezon]

Yes. Separate change.

MOTDSystem can be moved to Core.

Remove ReloadAllTextures and just call WW3DAssetManager::Get_Instance()->Release_All_Textures() I think.

And for OSDisplayWarningBox, OSDisplaySetBusyState you need to do something clever.

[OmarAglan]

opened the three changes as separate prs

• refactor(scriptengine): Remove ReloadAllTextures and call the asset manager directly #3672 removes ReloadAllTextures and calls WW3DAssetManager::Get_Instance()->Release_All_Textures() directly
• refactor(gui): Move MOTDSystem from GameEngineDevice to GameEngine #3673 moves MOTDSystem to Core/GameEngine
• refactor(gameengine): Replace OSDisplaySetBusyState with GameEngine::setBusyState #3674 replaces OSDisplaySetBusyState with a virtual GameEngine::setBusyState(). Win32GameEngine overrides it with the same code, like serviceWindowsOS()

OSDisplayWarningBox has no callers, so it does not need the device library at link time. I left it as it is. I can remove it in another PR if you want.

After these are merged I will rebase this PR and remove this line.