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

tools/version.sh: fix compilation of shallow clones #220

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

Conversation

guijan
Copy link
Contributor

@guijan guijan commented Dec 19, 2024

See the commit message for details.

Also, maybe we don't need to keep track of this SVN revision number stuff now that Aegisub is on git? That would remove a bit of code.

@Totto16
Copy link

Totto16 commented Dec 19, 2024

#165 is similar / does the same.
But not sure which one is better, but both solve the same problem

version.sh assumed that commit 16cd907
existed, this isn't true when the source tree is a shallow clone.

Tested on OpenBSD 7.6 with a few uncommitted patches to fix unrelated
compilation issues.
@guijan guijan force-pushed the fix-shallow-clones branch from e716fd5 to 2cf5323 Compare December 19, 2024 17:18
@petzku
Copy link
Contributor

petzku commented Dec 19, 2024

Also, maybe we don't need to keep track of this SVN revision number stuff now that Aegisub is on git? That would remove a bit of code.

As I understand, this exists to keep the revision numbers consistent? Seeing as the total rev-count on git up to SVN r6962 (i.e. 16cd907) is only 5849. That said, this could also be accomplished without breaking shallow clones (though producing incorrect revision numbers; your solution is certainly better in that regard) using $(git rev-parse --count HEAD) + 1113 instead.

@guijan
Copy link
Contributor Author

guijan commented Dec 19, 2024

though producing incorrect revision numbers

Aegisub/src/version.cpp

Lines 81 to 86 in 921249c

int GetSVNRevision() {
#ifdef BUILD_GIT_VERSION_NUMBER
return BUILD_GIT_VERSION_NUMBER;
#else
return 0;
#endif

I just went with what the code already seemed to want to do, although the #else branch is never triggered because the current meson.build and version.sh from master will always define BUILD_GIT_VERSION_NUMBER for shallow clones

@petzku
Copy link
Contributor

petzku commented Dec 19, 2024

Yeah, this behavior is quite reasonable; i just meant that if one were to blindly evaluate

git_revision=$(expr 1113 + $(git rev-list --count HEAD))

you would end up with an incorrect version number (likely 1114) for a shallow clone, rather than 0.

@petzku
Copy link
Contributor

petzku commented Dec 19, 2024

Either way, the logic to keep counting SVN revision numbers seems reasonable to me, if only for familiarity -- since for a long while (while upstream was stale), Aegisub builds were most commonly referred to by their revision numbers. Changing the logic for this would mean a change in how those revision numbers are calculated, making new revision numbers no longer sequential with the old ones (though, by my understanding, there is no real need for them)

@FichteFoll
Copy link
Contributor

FichteFoll commented Dec 19, 2024

The SVN revision number is not a thing we should bend over backwards to maintain. Especially if we consider the repo setup to be usable now with more frequent releases and CI builds, I would rather go the git describe route and fall back to just the commit hash if no git tag can be found, which will also work in any shallow clone. It's just that the version number of the closest tag in the history is not always available in that case, but I find hard-coding the latest release version a reasonable approach.

In short, the logic I propose to build a version number/string is:

  1. if "official release", take it from the hard-coded version number. This requires keeping this number up to date, of course.
  2. else, build it from the hard-coded version number + number of commits since the lastest tag (if available) + the commit hash. May also want to throw in dev somewhere to indicate that this is a dev build.

Edit: forgot to comment on the actual PR. For now, this is probably a reasonable workaround and better than #165.

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