Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

Razish
Copy link
Member

@Razish Razish commented Sep 30, 2023

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 to UNKNOWN

So we have:

  • version cvar (serverinfo) for the engine (also displayed in console)
  • gamename cvar (serverinfo) for the module (also printed on module load)

image

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.

@Razish Razish requested a review from a team as a code owner September 30, 2023 07:39
Copy link
Contributor

@taysta taysta left a 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.

@Daggolin
Copy link
Contributor

Daggolin commented Oct 3, 2023

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 OpenJK-MP: v1.0.1.0 is. The 1.0.1.0 originates from the last official jka patch where they used a tool for version numbers. OpenJK just targets 1.01, not 1.00 and the string implies you are running v1.0.1.0 of OpenJK, but in reality that string was shown on OpenJK for years (since 2013 I would guess) without actually holding any meaning other than OpenJK being based on the 1.0.1.0 release of vanilla jka. I think it would be far more useful if there was an actual OpenJK version in the lower line. For instance derived from the latest git tag. JK2MV just displays JK2MV: v1.4-386-gHASH when the latest tag on the branch is 1.4 and the branch is 386 commits ahead of that tag. That way it's easy for developers and users to determine if the build used by someone is close to the latest release/tag or far ahead. The lower line of the version info on OpenJK could use the git tag in a similar way.

@Razish
Copy link
Member Author

Razish commented Oct 5, 2023

Good points. I've swapped the short commit hash for the git tag (v1.2.3-4ga1b2c3d) and normalised all references to versions across the code.
I think I need to fix Win32 resources still.

@Razish Razish mentioned this pull request Oct 11, 2023
@taysta
Copy link
Contributor

taysta commented Oct 29, 2023

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.

taysta@5c25d46

@taysta
Copy link
Contributor

taysta commented Nov 3, 2023

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

@Razish Razish changed the title [Shared] Add short git hash [Shared] Add git tag to version string Feb 16, 2024
Copy link

@bartrpe bartrpe left a 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 }}
Copy link
Contributor

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?

Copy link

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?

Copy link
Member Author

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?)

Copy link
Member

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.

Copy link
Contributor

@mrwonko mrwonko May 2, 2024

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?

Copy link
Contributor

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.

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants