-
Notifications
You must be signed in to change notification settings - Fork 89
[GITHUB] Add weekly pre-releases workflow for Generals and GeneralsMD #929
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
What does this change produce then? |
If these are our 'official' pre-releases then they need to be properly versions. |
I can adjust for a semVer schema if it looks better. |
It will produce zip files weekly with the main executables for generals, worldbuilder and w3dview both for both games. |
It's not about looking better, it's about having traceability when somebody reports an issue or bug. |
d248576
to
b668cf2
Compare
@xezon @roossienb I’ve just updated the pull request to use semantic versioning. The version is now based on When the project is ready for a stable release, we just need to update the version file to I ran the workflow on my fork and everything went smoothly: ![]() ![]() |
In a discussion with @Mauller he told me that they did nightly builds in which they used the latest commit number as the build number in the application. This makes it easy to track back. |
It’s fine to change the logic to whatever is needed. But just need to decide what will be the pattern. All approaches have its own pros and cons. Using latest commit it’s a good approach but we can get more than one commit in a day, and this would be a problem to track anyway. What about to mix the approaches like |
I think most important is that the version is also shown in-game (so when you open the options menu, in the bottom). |
I totally agree with you. But I don’t have enough knowledge in C language. Can you guys help me to do that? |
I don't think it should have a proper version number tag, only official releases should get a version tag and otherwise should show the commit and if the tree was dirty when it was built. Vanilla Conquer uses a GitWatcher cmake module to populate the variables used to configure this file which is then referred to when configuring the version. |
09d1dd0
to
a801bfb
Compare
I think I’m getting close to add the build version to game. I believe in a few days I will be able to update this pr with the modifications. |
@xezon @roossienb @OmniBlade @tintinhamans I've update the first comment of this PR with the latest changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there needs to be some thought as to how these releases are differentiated from our "official" releases. As I've already commented, I think semver should be reserved for final release versions.
I also don't get what the vesion text files get us that can't be just stored in git itself as tags?
.github/workflows/base-version.txt
Outdated
@@ -0,0 +1 @@ | |||
0.9.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this versioning should be left out of the weekly builds completely personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. We can make the versioning enterily independent of the CI.
.github/workflows/weekly-release.yml
Outdated
echo "changed=true" >> $GITHUB_OUTPUT | ||
fi | ||
|
||
calculate-version: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What benefit do we get creating these files to be moved around rather than just calculating the information and storing it in variables where the workflow requires it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed by build-toolchain and will be used in C++ counterpart here and here
I do think our previews and releases should only contain the vc6 + tools builds, no win32, no extras. Because the win32 is not meant to be used by players at this time. |
What about this approach?
This allows us to:
|
If the CMake can generate the version, then is it fine to use that instead? |
Although I don’t have experience in c/c++ coding, I can try to do it. What will be the version pattern? Just the git version? |
I can do that, because I already have all the code here to facilitate this. |
Great! Just to make sure we're on the same page, for this PR I need to:
Anything else? |
Yes that would be good. So for versioning I have prepared change #1219 that uses a CMake script to generate an additional git version file for C++ which looks like so: GeneralsGameCode/resources/gitinfo/gitinfo.cpp.in Lines 20 to 28 in 0f0352e
We can generate another version of this in CMake that the CI could read, in Json format or just Plain text: whatever works. Additionally we write game version information in GeneralsGameCode/GeneralsMD/Code/Main/CMakeLists.txt Lines 30 to 47 in 477be26
For in game versioning, I currently settled on:
Which in turn should look like so:
For VERSION_BUILDUSER we do not yet have logic in place to actually generated the string from CI. So that is something we would need to add (in this change). I think CI needs to:
Do you think you can work with this information? Do you need assistance? I can help with the CMake side of things. I think we need to finalize #1219 first. |
#1219 was merged and CMake now generates git version information. The git version information is generated in the resources target. And the game version information is generated in the z_generals, g_generals targets. There will be more changes necessary to get the CI involved. I have another change #1291 lined up that makes some changes to the game version information. After this we need to make another pass on git + game version how we combine them better. I think game version also needs to be moved to resources target, so that it could also be reused in tools! |
Hey @xezon sorry for a long delay, i'm facing some personal issues and not had much time last weeks. I Intend to work on CI side this weekend. I keep in touch if i have some trouble in the C++ part. I'm also available to schedule some pair programming if needed. |
154a6aa
to
23fb099
Compare
@xezon I have updated the code on GitHub to change the release logic and add |
Just another question: I noticed in the project README that Linux compatibility is listed as 100%. Would it be possible to provide build instructions for Linux? If so, I could consider adding a Linux build to the CI process as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a sample for what this workflow currently generates? Is it complete or is it missing anything?
@@ -112,6 +116,10 @@ jobs: | |||
"-DRTS_BUILD_ZEROHOUR=${{ inputs.game == 'GeneralsMD' && 'ON' || 'OFF' }}", | |||
"-DRTS_BUILD_GENERALS=${{ inputs.game == 'Generals' && 'ON' || 'OFF' }}" | |||
) | |||
|
|||
if ("${{ inputs.release }}") { | |||
$buildFlags += "-DVERSION_BUILDUSER='${{ inputs.release }}'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does inputs.release
evaluate to? The variable name is strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a mistake and thought that simply adding the VERSION_BUILDUSER parameter to CMake would be enough, but now, looking more closely, I realize there is some logic in the C++ part that I need to review as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just changed the input name to release-name
and cmake flag to RELEASE_NAME
and will work on C++ counterpart. I'm also open to suggestions for a better variable name.
1e3014b
to
02f4b33
Compare
- name: Create GitHub Release | ||
uses: softprops/action-gh-release@v2 | ||
with: | ||
tag_name: preview-${{ needs.get-date.outputs.date }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this the format for the release name/tag should be preview-$DATE, but the most recent runs on your repo don't reflect that despite it being 2 weeks since you last pushed the branch? I'm guessing you need to push it to main for that to happen or something? Just wanted to check the output before I gace it the okay.
Also, do we want to call it "preview" which implies its just an early release of what could be final when in practice that won't be the case? Maybe just prefix "weekly" instead? Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"weekly" is also fine
Maybe "weekly-preview" :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Guys,
@OmniBlade my fork uses a different versioning schema (semVer) - in this schema 0.x versions implies that is a pre/beta release. For superhackers project we agreed to use a date-based versioning. Just need to decide if we want to user preview-$date
or weekly-preview-$date
and I will change in asap.
imho using weekly
or nightly
naming automatcaly implies in a non production-ready version so the preview
tag could not necessary.
@xezon just send a change to override the version name using the RELEASE_NAME
build flasg in toolchain. This cmake change is suggested by copilot, so i'm not sure if it will work properly if you can suggest something better i would appreciate.
I'm also available for a pair programming if you guys find useful, it will be a great opportunity for me to learning a bit of c++ from more skilled engineers =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the version string now look in the game?
8fa4434
to
9d81587
Compare
.github/workflows/weekly-release.yml
Outdated
- name: Download GeneralsMD VC6 Artifacts | ||
uses: actions/download-artifact@v4 | ||
with: | ||
name: GeneralsMD-vc6+t+e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to simply use the entire GeneralsMD-vc6+t (not GeneralsMD-vc6+t+e). It has all useful tools and the game. No need to peek inside the archive and filter its contents again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
- name: Create GitHub Release | ||
uses: softprops/action-gh-release@v2 | ||
with: | ||
tag_name: preview-${{ needs.get-date.outputs.date }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the version string now look in the game?
.github/workflows/weekly-release.yml
Outdated
include: | ||
- preset: "vc6" | ||
tools: true | ||
extras: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extras would not be necessary here. We do not need to release the extra tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -110,6 +114,10 @@ jobs: | |||
"-DRTS_BUILD_ZEROHOUR=${{ inputs.game == 'GeneralsMD' && 'ON' || 'OFF' }}", | |||
"-DRTS_BUILD_GENERALS=${{ inputs.game == 'Generals' && 'ON' || 'OFF' }}" | |||
) | |||
|
|||
if ("${{ inputs.release-name }}") { | |||
$buildFlags += "-DRELEASE_NAME='${{ inputs.release-name }}'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to set the following defines:
VERSION_BUILDUSER="The Super Hackers"
VERSION_BUILDLOC="https://github.com/TheSuperHackers/GeneralsGameCode"
VERSION_BUILDNUM=<The number of total builds>
For version number we currently use the Git Revision number which is ok.
The files that are generated for VERSION_BUILDUSER etc probably need updating to:
file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/GeneratedVersion.h
"#pragma once
#ifndef VERSION_BUILDNUM
#define VERSION_BUILDNUM ${BuildNumber}
#endif
#ifndef VERSION_LOCALBUILDNUM
#define VERSION_LOCALBUILDNUM 0
#endif
#ifndef VERSION_BUILDUSER
#define VERSION_BUILDUSER ${BuildUser}
#endif
#ifndef VERSION_BUILDLOC
#define VERSION_BUILDLOC ${BuildLocation}
#endif
"
)
or a variation thereof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
d04501c
to
9746f58
Compare
@xezon , I've made some major changes to the codebase here. Summary of ChangesThe primary modification is how the version string is displayed on the options screen.
How It Works
![]() |
What this PR does?
This PR complements #1171
Creates a github action to create a weekly release with the latest code changes. It will be scheduled to every moday.
https://github.com/fbraz3/GeneralsGameCode/actions/runs/15689781515

It also can be mannualy triggered, and it have parameters to bypass the code changes check to force a new build

The release notes also have a changelog with all commits since the last release - for the first release it will be limited to the last 10 commits
