Skip to content

Handle Missing Tags in Versioning by Setting Default to 0 #3359

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

Merged
merged 4 commits into from
Mar 21, 2025

Conversation

ahnaf-tahmid-chowdhury
Copy link
Contributor

Problem

When forking and cloning OpenMC, all tags are not always present in the resulting clone. This causes issues with CMake's git-based versioning pathway:

  1. Failure case: If git is available but no tags exist, version detection fails.

  2. Incorrect versioning: The git SHA is correct, but the displayed version is incorrect.

Solution

  • Updated GetVersionFromGit.cmake to follow the setuptools_scm convention.

  • If no git tags are found, the version defaults to 0.0.0 instead of failing.

  • A warning message is displayed, instructing users to run git fetch --tags.

Notes

Users who wish to fetch tags can do so manually:

git fetch --tags

Ready for Review

Tagging @pshriwise and @MicahGale for feedback.

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Looks good to me but I'll hold off on merging in case @pshriwise or @MicahGale have any feedback.

if(VERSION_STRING STREQUAL "")
message(FATAL_ERROR "No git tags found. Run 'git fetch --tags' and try again.")
set(VERSION_STRING "0.0.0")
message(WARNING "No git tags found. Version set to 0. Run 'git fetch --tags' to ensure proper versioning.")
Copy link
Contributor

Choose a reason for hiding this comment

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

There's more to it than just fetching for forks. One must add the upstream remove (this repo) and fetch tags from there. If the tags aren't in their forked repo this won't help sadly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think maybe drop the "here's how to fix it" bit, because it's a bit too nuanced for cmake. I think the bigger thing is having a guide in the developer docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think more detail in the dev guide, yes. The warning here can provide direction to that section ideally.

Copy link
Contributor

@MicahGale MicahGale left a comment

Choose a reason for hiding this comment

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

I think updating developer docs should be part of this PR, unless this needs to be deployed asap.

@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

I have created a new section in the doc for this since it is optional. However, we can update the documentation if needed.

Copy link
Contributor

@MicahGale MicahGale left a comment

Choose a reason for hiding this comment

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

The docs look good and really help with "minimizing astonishment"

Copy link
Contributor

@MicahGale MicahGale left a comment

Choose a reason for hiding this comment

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

LGTM (GH didn't include comment in last review???)

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Teeny tiny clarification and then I'm happy. Thanks @ahnaf-tahmid-chowdhury!

@pshriwise pshriwise enabled auto-merge (squash) March 20, 2025 20:09
@ahnaf-tahmid-chowdhury
Copy link
Contributor Author

It seems the workflow run was canceled. Is there anything else to update @pshriwise?

@pshriwise pshriwise merged commit 3f3649d into openmc-dev:develop Mar 21, 2025
25 of 26 checks passed
ahnaf-tahmid-chowdhury added a commit to ahnaf-tahmid-chowdhury/OpenMC that referenced this pull request Mar 24, 2025
ahnaf-tahmid-chowdhury added a commit to ahnaf-tahmid-chowdhury/OpenMC that referenced this pull request Mar 29, 2025
ahnaf-tahmid-chowdhury added a commit to ahnaf-tahmid-chowdhury/OpenMC that referenced this pull request Mar 29, 2025
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.

4 participants