-
Notifications
You must be signed in to change notification settings - Fork 617
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
[Shared] Add git tag to version string #1176
base: master
Are you sure you want to change the base?
Conversation
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 makes good use of previously implemented features that were laying dormant, helps in many situations when debugging a users issues to know their build. LGTM.
Adding the revision is really useful when trying to figure out why someone experiences a bug and it would've been helpful for debugging issues on multiple occasions in the past. Thanks for working on this pretty much instantly after I suggested it. However I am not sure how useful the lower version string |
Good points. I've swapped the short commit hash for the git tag ( |
The short hash isn't being applied correctly in the action builds CMAKE step as expected, I made a commit here that fixes the issue, as well as some other housekeeping that the workflow file could do with. |
Also, it doesn't get the hash correctly if for example you make the build from cmakegui, and it gets run in build folder, this commit fixes it: taysta@87f5d1c |
b89e19e
to
acf618a
Compare
replace ~160 lines of cmake with a single call to git 🤷
Co-authored-by: tayst <[email protected]>
1535da8
to
b3455b5
Compare
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 it
@@ -214,26 +247,22 @@ jobs: | |||
if: ${{ matrix.build_type == 'Release' }} | |||
working-directory: ${{ github.workspace }}/install/JediAcademy | |||
shell: bash | |||
run: | | |||
chmod +x openjk.${{ matrix.arch }}.app/Contents/MacOS/openjk.${{ matrix.arch }} |
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.
Why is the chmod +x
no longer needed?
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.
@mrwonko taring a folder itself doesn't require the permission for any files to be executed as an application, taring requires pretty much nothing in that regard, you only run "tar"; the purpose of the line was to have the executable within the tar archive with permissions to run already, so that when somebody downloads the release his executable can be run automatically; now the file user will have to manually apply permissions so that the executable runs
so it's not "not needed" but I guess it's an idea to make sure that people know what they're doing and not running executables on auto?
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 don't know why it's "no longer" needed, but it's not needed. It was also inconsistently applied but I can't remember how 🙃 (only SP? missing SP? missing JK2?)
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.
Were we ever using 7za instead of tar? Because the 7z tool cannot/will not preserve executable bits.
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 purpose of the line was to have the executable within the tar archive with permissions to run already, so that when somebody downloads the release his executable can be run automatically; now the file user will have to manually apply permissions so that the executable runs
But why would anybody download the binaries without the intent to run them?
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 don't know why it's "no longer" needed, but it's not needed. It was also inconsistently applied but I can't remember how 🙃 (only SP? missing SP? missing JK2?)
Were we ever using 7za instead of tar? Because the 7z tool cannot/will not preserve executable bits.
It was at some point in the past, but it was changed (#1128) to not use 7z and instead to tar with chmod +x to be able to distribute the binaries ready to run, a fix from jamme's workflow. I must have forgotten to add the lines for SP and OpenJO in the workflow as I was back-porting the change from my jka mp only fork. The line may have been added to mac as a precaution, as the issue was with running the linux executable without doing chmod +x every time manually.
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.
So, is it needed for Linux to run the distributed files or not? If its not needed, we can resolve this and finally merge.
replace ~160 lines of cmake with a single call to git 🤷
If the git tag can't be obtained (e.g. no
git
on path) it will default toUNKNOWN
So we have:
version
cvar (serverinfo) for the engine (also displayed in console)gamename
cvar (serverinfo) for the module (also printed on module load)Future work
Make the command execute at compile time, not just configuration time.
Not an issue for official builds, but for local dev the commit hash will stick around until you reconfigure.