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

Use git to retrieve version instead of manually change PREMAKE_VERSION. #2405

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jarod42
Copy link
Contributor

@Jarod42 Jarod42 commented Dec 31, 2024

What does this PR do?

Handle PREMAKE_VERSION from git tag

How does this PR change Premake's behavior?

No behavior changed.

Anything else we should know?

gitintegration setup, so premake5 is launched at each checkout.

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits
  • Align documentation to your changes

premake5.lua Outdated
local function autoversion_h()
local git_tag, errorCode = os.outputof("git describe --tag --always")
if errorCode == 0 then
print("git description: ", git_tag)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this debug print and should it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it more as information than debugging.

@tritao
Copy link
Contributor

tritao commented Dec 31, 2024

Overall looks good and great improvement over the current way of handling the version string.

But long-term (maybe as future PR) I think it makes sense to provide a core API for this task (generating a config file), which is a pretty common requirement for many projects.

We can take a look at inspiration from CMake's configure_file.

@Jarod42
Copy link
Contributor Author

Jarod42 commented Dec 31, 2024

We can take a look at inspiration from CMake's configure_file.

A simple function taking "template_path", "output_path" and a table for mapping seems sufficient.
Just turn that function more generic and with proper naming.

But outside scope of that PR IMO.

@samsinsane
Copy link
Member

As previously discussed, I'm not a fan of this approach. Here are some of my issues:

  1. It always executes on checkout, this is unnecessary in my opinion. The only time we truly care about the version is when someone is using a tagged build.
  2. It changes PREMAKE_VERSION to be a different format. 5.0.0-beta3 -> v5.0.0-beta3 and 5.0.0-dev to <latest tag>-<num-commits-ahead>-g<hash>. The desire to automate was to avoid the dev->beta3->dev churn, this churn exists to avoid having builds say they're a specific tag when they're not.
  3. The git archive header doesn't seem to work on a master archive. On my machine I get beta1 even though I have beta4 in my repo.
  4. It relies on __has_include otherwise the version is "UNKNOWN" - using Lua to determine the version and only using a single header would work so much better than relying on C23 features to be available.
  5. If __has_include exists, but premake_git_version.h isn't generated for some reason, then premake_archive_version.h will always be pulled in instead, which for Git-based builds will always set the version to "$Format:%(describe)$". This needs to be handled better so that useful information is stored in the version.

@Jarod42
Copy link
Contributor Author

Jarod42 commented Jan 1, 2025

  1. yes, executed at each checkout
  2. I didn't spot the extra prefix "v", but indeed noticed that "dev" version is more (too?) precise.
  3. lightweight tag versus annotated tag and also tags outside of main branch (only parent1 is used).That should be fixed on the way to tag version.
    4.5. We might get rid of __has_include and replace the check by the result of the generation to set some #define

I don't see a way to "fix" 1. and 2.
We can fix 3. with proper tag.
I might fix 4.5, but if 1.2. are blocking, the fix of 4.5. is useless. So feel free to close the PR in that case.

Just noticed that "script/package.lua" would be broken too with that change (and can/should be fixed).

@Jarod42 Jarod42 marked this pull request as draft January 1, 2025 16:44
@Jarod42 Jarod42 force-pushed the premake_version branch 3 times, most recently from e56ef4e to 5d7b959 Compare January 2, 2025 10:17
@Jarod42
Copy link
Contributor Author

Jarod42 commented Jan 2, 2025

  1. gitintegration removed
  2. Changed to use only exact tag, or file from source-package, and default to 5.0.0-dev
  3. lightweight tags are no longer ignored.
    1. __has_include removed, and use defines instead

@Jarod42 Jarod42 marked this pull request as ready for review January 2, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants