[Nima-Rahimi-Foroushaani]

No description provided.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: b6d464d4-7a8d-44a2-bc2e-ef04032ab19e

📥 Commits

Reviewing files that changed from the base of the PR and between e572d87 and 1ba4981.

📒 Files selected for processing: 2

• .github/workflows/ci.yml
• Core/GameEngine/CMakeLists.txt

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

Walkthrough

The CI workflow adds change detection as a dependency of the Generals MD replay-check job. The GameEngine CMake configuration adds compiler-specific forced includes when precompiled headers are disabled.

Changes

CI job dependency

Layer / File(s)	Summary
Replay-check job dependencies .github/workflows/ci.yml	replaycheck-generalsmd now depends on both detect-changes and build-generalsmd-vc6.

Precompiled-header fallback

Layer / File(s)	Summary
Forced include fallback Core/GameEngine/CMakeLists.txt	When precompiled headers are disabled, the configuration force-includes BasePrecomp.h before PreRTS.h. It uses /FI options for MSVC and -include options for other compilers.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: helmutbuhler, xezon

Merge Risk: ⚪ Minimal · up to 1ba49

The CI dependency and precompiled-header fallback are ready to merge after normal checks.

Pre-merge checks: 3 · 1

Failed checks: 1 inconclusive

Check name	Status	Explanation	Resolution
Description check		No pull request description was provided, so the changes and their purpose are not documented.	Add a description that summarizes the CI dependency update and the forced include handling added to Core/GameEngine/CMakeLists.txt.

Passed checks: 3 passed

Check name	Status	Explanation
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.
Title check		The title accurately describes the CI dependency change, but it does not identify the Docker build fix or the CMake compiler include changes. It is still related to a real part of the pull request.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Confidence Score: 5/5

[Low impact] The PR appears safe to merge.

Summary

Adds detect-changes as a direct dependency of replaycheck-generalsmd.

• GeneralsMD replay checks wait for the change detector and VC6 build.

_{Reviews (2) · Last reviewed commit: "fix(ci): To access the indirect dependen..." · Reviewed by Greptile}

[xezon]

This looks unexpected. BasePrecomp.h should come through core_libraries

[Nima-Rahimi-Foroushaani]

It does indeed. However, the forced inclusion of PreRTS.h comes before the ones of BasePrecomp.h which breaks the compilation.

[Skyaero42]

is this and #3523 related?

[tintinhamans]

tested #3523 rebased onto current main, with one extra trig.h include in ICoord2D.h for Sqrt.

clean Docker/VC6 build of z_generals then passes without this fallback. i'd handle the missing includes in #3523 and keep the CI dependency fix separate.

[stephanmeesters]

Looks like we can get PCH working in the Docker build with a little tweak (/Zm100 in space of /Zm1000):

diff --git a/resources/dockerbuild/entrypoint.sh b/resources/dockerbuild/entrypoint.sh
index 1fe64c12ea..06cff60626 100644
--- a/resources/dockerbuild/entrypoint.sh
+++ b/resources/dockerbuild/entrypoint.sh
@@ -16,10 +16,11 @@ if [ "${FORCE_CMAKE:-}" = "true" ] || [ ! -f  build/docker/build.ninja  ]; then
         -DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=ONLY \
         -DCMAKE_FIND_ROOT_PATH_MODE_INCLUDE=ONLY \
         -DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ONLY \
-        -DCMAKE_DISABLE_PRECOMPILE_HEADERS=1 \
+        -DCMAKE_DISABLE_PRECOMPILE_HEADERS=OFF \
+        "-DCMAKE_CXX_FLAGS=/DWIN32 /D_WINDOWS /GX /Zm100" \
         -DCMAKE_C_COMPILER_WORKS=1 \
         -DCMAKE_CXX_COMPILER_WORKS=1 \
-        -B /build/cnc/build/docker
+        -B Z:/build/cnc/build/docker
 fi
 
 cd /build/cnc/build/docker 

[Nima-Rahimi-Foroushaani]

I removed the commit, related to the docker based build problem. I assume it will be handled in #3523.

[tintinhamans]

Looks good. The replay check reads needs.detect-changes.outputs but only had build-generalsmd-vc6 in needs, and GitHub only exposes jobs you list directly. It worked anyway, but this is the right way.

Could you rename it to something like ci: Add detect-changes to replay check needs and add a short description?

[Skyaero42]

Looks like we can get PCH working in the Docker build with a little tweak (/Zm100 in space of /Zm1000):

Apparently it is better not to rely on PCH:
#3523 (comment)

[stephanmeesters]

Looks like we can get PCH working in the Docker build with a little tweak (/Zm100 in space of /Zm1000):

Apparently it is better not to rely on PCH: #3523 (comment)

I think the root cause that PCH was disabled on Docker was that CMake defaulted to a compiler setting that requested a lot of memory unnecessarily (/Zm1000). But having the project not rely on PCH to be able to build is also good.