Repository navigation
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe weekly release workflow adds MSVC builds for Generals and GeneralsMD. It packages both builds with translated compatibility notices and attaches the archives to the release. ChangesWeekly release builds
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant WeeklyWorkflow
participant GeneralsBuild
participant GeneralsMDBuild
participant ReleaseAction
WeeklyWorkflow->>GeneralsBuild: run weekly MSVC build with FFmpeg disabled
WeeklyWorkflow->>GeneralsMDBuild: run weekly MSVC build with FFmpeg enabled
GeneralsBuild->>WeeklyWorkflow: provide MSVC artifact
GeneralsMDBuild->>WeeklyWorkflow: provide MSVC artifact
WeeklyWorkflow->>ReleaseAction: attach MSVC archives and release notes
Suggested reviewers: Merge Risk: 🔵 Low · up to
|
| 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 clearly identifies the main change: weekly CI will ship VC6 and MSVC builds. |
| Description check | |
The description relates to the changeset by mentioning MSVC fixes, VC6 decisions, CI testing, README guidance, and GitHub release text. |
- Autopilot · 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.
Comment @coderabbitai help to get the list of available commands.
 [Medium impact] The PR changes the weekly game downloads and their build pipeline. Fix the historical workflow's support for older preset names before merging. Findings
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
ec9a251f-4986-4724-ba48-5d3c5047342f
📒 Files selected for processing (4)
.github/workflows/modern-readme.txt.github/workflows/weekly-release.ymlCMakePresets.jsoncmake/config-retail.cmake
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| name: Build Generals${{ matrix.preset && '' }} | ||
| permissions: | ||
| contents: read | ||
| packages: write |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
git diff --no-ext-diff --unified=50 b805c12ee1aedc0a4b241006803b8e04bbf288a6 22015ea93b1157b5307bbb39331b83d21d8dfd72 -- .github/workflows/weekly-release.yml
printf '\n--- HEAD workflow with line numbers ---\n'
git show 22015ea93b1157b5307bbb39331b83d21d8dfd72:.github/workflows/weekly-release.yml | nl -ba
printf '\n--- reusable workflow references in weekly-release ---\n'
git show 22015ea93b1157b5307bbb39331b83d21d8dfd72:.github/workflows/weekly-release.yml | grep -nE 'uses:|permissions:|packages:|secrets:|release:|upload|publish|ghcr|docker|package' || true
printf '\n--- workflow files at head ---\n'
git ls-tree -r --name-only 22015ea93b1157b5307bbb39331b83d21d8dfd72 .github/workflows
printf '\n--- relevant build and package operations ---\n'
rg -n -i --glob '!weekly-release.yml' 'packages: write|secrets: inherit|workflow_call|upload-artifact|ghcr|docker push|gh release|actions/upload|packages' .github/workflows .github 2>/dev/null | head -240 || trueRepository: TheSuperHackers/GeneralsGameCode
Length of output: 23797
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- reusable workflow at reviewed head ---'
git show 22015ea93b1157b5307bbb39331b83d21d8dfd72:.github/workflows/reusable-build-toolchain.yml | nl -ba | sed -n '1,215p'
printf '\n--- secret references and package-cache bindings in reusable workflow ---\n'
git show 22015ea93b1157b5307bbb39331b83d21d8dfd72:.github/workflows/reusable-build-toolchain.yml | grep -nEi 'secrets|token|GitHubPackages|binarycache|binary.cache|packages|preset|cache' || true
printf '\n--- change history for relevant reusable workflow sections in PR diff ---\n'
git diff --no-ext-diff --unified=8 b805c12ee1aedc0a4b241006803b8e04bbf288a6 22015ea93b1157b5307bbb39331b83d21d8dfd72 -- .github/workflows/reusable-build-toolchain.ymlRepository: TheSuperHackers/GeneralsGameCode
Length of output: 12892
🌐 Web query:
GitHub official docs reusable workflows secrets inherit GITHUB_TOKEN automatically available called workflow
💡 Result:
**Yes.** GitHub’s docs say a reusable workflow is automatically granted access to `github.token` and `secrets.GITHUB_TOKEN`; you don’t need to pass `GITHUB_TOKEN` with `secrets: inherit`. ([docs.github.com](https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations?utm_source=openai))
`secrets: inherit` is for passing the caller’s other secrets to a called workflow. The called workflow’s token permissions are still constrained by the caller’s permissions—`inherit` doesn’t grant extra `GITHUB_TOKEN` access. ([docs.github.com](https://docs.github.com/en/actions/how-tos/reuse-automations/reuse-workflows?learn=getting_started&learnProduct=actions&utm_source=openai))
Citations:
- 1: https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations?utm_source=openai
- 2: https://docs.github.com/en/actions/how-tos/reuse-automations/reuse-workflows?learn=getting_started&learnProduct=actions&utm_source=openai
Remove secrets: inherit from both build jobs.
The reusable workflow uses the automatically available GITHUB_TOKEN; it does not need the caller’s other secrets. Keep packages: write because the modern vcpkg builds use the package binary cache.
Suggested fix
@@
game: "Generals"
preset: ${{ matrix.preset }}
tools: ${{ matrix.tools }}
extras: ${{ matrix.extras }}
- secrets: inherit
@@
game: "GeneralsMD"
preset: ${{ matrix.preset }}
tools: ${{ matrix.tools }}
extras: ${{ matrix.extras }}
- secrets: inheritSources: Learnings, Linters/SAST tools
There was a problem hiding this comment.
Will this cause trouble again? We had trouble with this in the past.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline .github/workflows/reusable-build-toolchain.yml --items all || true
printf '\n--- reusable workflow ---\n'
nl -ba .github/workflows/reusable-build-toolchain.yml | sed -n '1,300p'
printf '\n--- workflow secret bindings ---\n'
rg -n -C3 'secrets\.|secrets:|packages:|GitHubPackages|VCPKG_BINARY_SOURCES|uses: \./' .github/workflowsLength of output: 20972
🤖 web_search executed:
GitHub Actions official documentation reusable workflow automatically access github.token secrets.GITHUB_TOKEN secrets inherit token permissions caller packages write
Length of output: 1495
@xezon, removing secrets: inherit should not break these build jobs.
I checked .github/workflows/reusable-build-toolchain.yml:
- It uses only
secrets.GITHUB_TOKEN, for the NuGet binary cache at Lines 136–137. - GitHub automatically provides that token to reusable workflows.
- Both weekly callers retain
packages: write, which permits binary-cache uploads.
This differs from .github/workflows/reusable-check-replays.yml. That workflow needs custom R2_* secrets. Removing secret forwarding there would cause trouble.
I have not verified a live weekly run or identified the earlier failure. Please link the earlier failed run if you want me to compare its cause. For runtime confirmation, manually dispatch the weekly workflow with force_changed: true and check the modern builds’ NuGet cache operations.
You are interacting with an AI system.
22015ea to
884d838
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Do not store the cache token in clear text. · reusable-build-toolchain.yml:136-145
.github/workflows/reusable-build-toolchain.yml:136-145
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDo not store the cache token in clear text.
NuGet writes this password as
ClearTextPasswordin the runner’s user-levelNuGet.Config. The later CMake steps run under the same account and can read it. The new weekly vcpkg jobs grantpackages: write, so code in those steps can use the token to publish packages. Remove-StorePasswordInClearTextto use NuGet’s Windows-encrypted storage. This protects the password at rest; it does not isolate it from malicious code running as the same account.Suggested fix
-Source "https://nuget.pkg.github.com/${{ github.repository_owner }}/index.json" ` - -StorePasswordInClearText ` -Name GitHubPackages `
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
6de533c6-034e-4b5b-a045-9da607a73ced
📒 Files selected for processing (1)
.github/workflows/weekly-release.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.
|
I'm not a fan of this and it may confuse players on which version to download. |
There are good reasons for still choosing vcpkg.
We can also close the (github) RELEASE and ask if GO wants both TSH vc6 and msvc blob in their release. No more juggling 3 exe's for users. |
|
No we keep weekly releases for reviewing and testing purposes. They are not so much for every day players, but developers and testers and lurkers. |
|
Related #2322 |
ae00b0e to
1c896b8
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
0656d24b-dd85-4e63-be9b-707f87343b05
📒 Files selected for processing: 4
.github/workflows/msvc-readme.txt.github/workflows/weekly-release.ymlCMakePresets.jsoncmake/config-retail.cmake
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
| name: Build Generals${{ matrix.preset && '' }} | ||
| permissions: | ||
| contents: read | ||
| packages: write |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '85,145p' .github/workflows/weekly-release.yml
sed -n '1,15p' .github/workflows/reusable-build-toolchain.yml
sed -n '45,65p' .github/workflows/reusable-build-toolchain.yml
sed -n '120,155p' .github/workflows/reusable-build-toolchain.ymlRepository: TheSuperHackers/GeneralsGameCode
Length of output: 4976
🌐 Web query:
GitHub Actions reusable workflow caller job permissions GITHUB_TOKEN permissions apply to all steps called workflow access token
💡 Result:
**Yes.** Put `permissions` on the caller’s reusable-workflow job (`jobs.<job_id>.permissions`). That permission set applies to the `GITHUB_TOKEN` available to jobs in the called workflow—and therefore to its steps/actions that use that token. If omitted on the caller job, the called workflow gets the repository’s default token permissions. ([docs.github.com](https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations?utm_source=openai))
The called workflow can keep or reduce those permissions, but **cannot elevate them**. So grant the needed scopes on the caller job; a separate token explicitly passed as a secret is a different credential. ([docs.github.com](https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations?utm_source=openai))
```yaml
jobs:
call-workflow:
uses: org/repo/.github/workflows/reusable.yml@main
permissions:
contents: read
pull-requests: write
```
Citations:
- 1: https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations?utm_source=openai
- 2: https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations?utm_source=openai
🏁 Script executed:
printf '%s\n' '--- weekly-release diff ---'
git diff --unified=20 02b5b56e22fa6a1ca06a3fc2f7648e646e9f56cb 1c896b8f12fecbf589ec02f32c2b86ff1a001016 -- .github/workflows/weekly-release.yml
printf '%s\n' '--- reusable workflow relevant blocks ---'
nl -ba .github/workflows/reusable-build-toolchain.yml | sed -n '1,240p'
printf '%s\n' '--- vcpkg references in workflow files ---'
rg -n -F --glob '.github/workflows/*.yml' -e 'vcpkg' -e 'GITHUB_TOKEN' -e 'VCPKG_BINARY_SOURCES' .github/workflows || test "$?" -eq 1Repository: TheSuperHackers/GeneralsGameCode
Length of output: 22188
🏁 Script executed:
printf '%s\n' '--- vc6 preset definitions ---'
rg -n -C 12 -F -- 'vc6-weekly' CMakePresets.json . || test "$?" -eq 1
printf '%s\n' '--- vcpkg build-configuration references ---'
rg -n -i -C 3 --glob '!**/.git/**' --glob '!**/vcpkg.json' --glob '!**/vcpkg-configuration.json' -e 'vcpkg' -e 'VCPKG_BINARY_SOURCES' -e 'CMAKE_TOOLCHAIN_FILE' CMakePresets.json CMakeLists.txt cmake .github || test "$?" -eq 1Repository: TheSuperHackers/GeneralsGameCode
Length of output: 37301
Restrict packages: write to the vcpkg build jobs.
Both caller jobs grant packages: write to every matrix entry, including vc6-weekly. The VC6 preset does not use the vcpkg toolchain, and the reusable workflow guards vcpkg setup and NuGet credential configuration with contains(inputs.preset, 'vcpkg'). The VC6 builds therefore receive package-publish authority without using the binary-cache path. Split the VC6 and vcpkg invocations so only the vcpkg jobs receive packages: write.
Suggested fix
matrix:
include:
- - preset: "vc6-weekly"
- tools: true
- extras: false
- preset: "win32-vcpkg-weekly"
tools: true
extras: false
ffmpeg: false
@@
secrets: inherit
+ build-generals-vc6:
+ needs: [detect-scm-changes, get-date]
+ if: needs.detect-scm-changes.outputs.changed == 'true'
+ name: Build Generals VC6
+ permissions:
+ contents: read
+ uses: ./.github/workflows/reusable-build-toolchain.yml
+ with:
+ game: "Generals"
+ preset: "vc6-weekly"
+ tools: true
+ extras: false
+ secrets: inherit
+
build-generalsmd:
@@
matrix:
include:
- - preset: "vc6-weekly"
- tools: true
- extras: false
- preset: "win32-vcpkg-weekly"
tools: true
extras: false
ffmpeg: true
@@
secrets: inherit
+ build-generalsmd-vc6:
+ needs: [detect-scm-changes, get-date]
+ if: needs.detect-scm-changes.outputs.changed == 'true'
+ name: Build GeneralsMD VC6
+ permissions:
+ contents: read
+ uses: ./.github/workflows/reusable-build-toolchain.yml
+ with:
+ game: "GeneralsMD"
+ preset: "vc6-weekly"
+ tools: true
+ extras: false
+ secrets: inherit
+
create-release:
@@
- needs: [build-generals, build-generalsmd, get-date]
+ needs: [build-generals, build-generalsmd, build-generals-vc6, build-generalsmd-vc6, get-date]
1c896b8 to
b6b05bc
Compare
mirelle7
changed the title
|
|
||
| ### Builds | ||
| - **VC6**: retail-compatible defaults. | ||
| - **MSVC**: vcpkg, retail compatibility off and all gameplay fixes on. |
There was a problem hiding this comment.
It is MSVC19 according to CI logs
How about we name them
..win32-vc6..
..win32-msvc19..
This would require a refactor of CI target names before this change here. The CMakePresets.json also does not mention MSVC19 in the preset names.
There was a problem hiding this comment.
im thinking:
windows-x86-vc6
windows-x86-msvc19
Would also be clear for future possible releases:
windows-x64-msvc19
windows-arm64-msvc19
windows-x86-mingwgcc
macos-universal-clang
web-wasm32-emscripten
3ds-armv6k-devkitarm
ps2-r5900-ps2sdk
etc
It pins neither an architecture nor a compiler and no CI job builds it.
The win32 and win32-vcpkg presets built the same MSVC binary and only differed in where the dependencies come from. Drop the win32-vcpkg presets: the win32 presets fetch dependencies with FetchContent and use vcpkg when its toolchain file is passed. CI selects it with a new vcpkg input on the reusable build workflow and marks those builds +v, keeping the same six MSVC builds per game as before.
vc6 -> windows-x86-vc6, win32 -> windows-x86-msvc19 and mingw-w64-i686 -> windows-x86-mingwgcc, keeping their -profile, -debug, -releaselog and -weekly suffixes. CI jobs, artifact names, the Docker build and docs follow the new names.
Add an MSVC (vcpkg) weekly build next to the existing VC6 build, with retail compatibility and retail gameplay bugs disabled. The VC6 zips keep their original names; the MSVC zips get an -msvc suffix and a multi-language README. The release page explains which zip to download. Add RTS_BUILD_OPTION_PRESERVE_RETAIL_BEHAVIOR as a separate CMake option for the PRESERVE_ defines.
The VC6 weekly builds do not use the vcpkg binary cache, so split each game's matrix into a VC6 job (contents: read) and a vcpkg job (contents: read, packages: write). # Conflicts: # .github/workflows/weekly-release.yml
…STITUTE With ENABLE_GAMETEXT_SUBSTITUTES=0 the macro drops its substitute argument, so the defaultGameTitle variable was unused and C4189 broke the build.
Add the windows-x86-msvc19-nonretail preset, which turns off retail compatibility, the preserved retail behavior and the optional engine features, so their code paths compile in CI. It is a CI-only build and is not shipped by the weekly release.
|
status update: found some broken configurations in the non-retail CI, will need to send PRs to get them to build |
b6b05bc to
0c01f2d
Compare
|
This can also add MinGW-w64 i686 cross-compilation support that will be added on pr #3552 , can this be done? |
|
Ready for review. Do I need to split out fix(gameengine): Pass the default game title directly to FETCH_OR_SUBSTITUTE ? |
| "-DRTS_BUILD_${{ matrix.game == 'Generals' && 'GENERALS' || 'ZEROHOUR' }}_TOOLS=ON" | ||
| ) | ||
| cmake --preset vc6 $buildFlags | ||
| cmake --preset windows-x86-vc6 $buildFlags |
There was a problem hiding this comment.
The workflow checks out the requested commit, then always configures with windows-x86-vc6. Commits from before this rename—including this PR's base—only have the vc6 preset, so historical builds now fail before compilation.
Select the preset supported by the checked-out commit, then use that name for building and its output directory for collecting artifacts.
Knowledge Base Used:
5 participants
A lot of our work isn't exposed in the RELEASE channel.
CI was tested on my own repo and the game has been confirmed running.
Msvc version includes all fixes both gameplay and bugs. Committee still decides for vc6.
Added README and github release text to tell the user which version to download.