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

Generate Version.h from current tag name #1610

Merged
merged 10 commits into from
Jan 4, 2017
Merged

Conversation

ericwa
Copy link
Collaborator

@ericwa ericwa commented Jan 3, 2017

Fixes #1601

Falls back to a version of 0.0.1 if the tag can't be parsed. We could just fail the build, alternatively, although in that case we'd need to tag some older commit in order to merge this without breaking the build.

The package file names changed, now looks like TrenchBroom-MacOSX-test-1571-26-g4c54a3c7-Debug.dmg (where test-1571-26-g4c54a3c7 is the git describe output. So with a proper tag it would be something like TrenchBroom-MacOSX-v2.0.0-rc1-Debug.dmg)

I tested on VS2015 and Xcode so far. I think I'll push a temporary tag to this branch to check all of the CI builds and Linux in particular.

Remove the major/minor/patch version from the package name,
since the "git describe" output has the same numbers,
and that is now part of the package name
@ericwa
Copy link
Collaborator Author

ericwa commented Jan 3, 2017

Test tag is here: https://github.com/kduske/TrenchBroom/releases/tag/v0.0.0-test
Seems to have worked so far

@kduske
Copy link
Collaborator

kduske commented Jan 3, 2017

Falls back to a version of 0.0.1 if the tag can't be parsed. We could just fail the build, alternatively, although in that case we'd need to tag some older commit in order to merge this without breaking the build.

I think there are two separate cases:

  • No tag to parse - this should not fail, and the version should be 0.0.0, even though that conflicts with Apple's requirements for CFBundleVersion (first integer should be greater than 0). Let's see what happens.
  • Tag cannot be parsed due to being malformatted. This should fail.

@ericwa
Copy link
Collaborator Author

ericwa commented Jan 3, 2017

No tag to parse - this should not fail

This will never happen, though, unless someone starts a fresh git history with no tags. So IMO we can just make it fail.
edit: git describe fails with a nonzero exit status if it can't find a tag.

Tag cannot be parsed due to being malformatted. This should fail.

Ok, I'll adjust that.

@kduske
Copy link
Collaborator

kduske commented Jan 3, 2017

I think we can also remove

  • cmake/publish.bat.in
  • cmake/publish.sh.in
  • cmake/upload.bat.in
  • cmake/upload.sh.in

and the rules to generate those from the cmake scripts. All future releases will be done on github and I'm going to retire the old site entirely.

Copy link
Collaborator

@kduske kduske left a comment

Choose a reason for hiding this comment

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

Some minor changes.

LIST(GET VERSION_PARTS 1 ${VERSION_MINOR})
LIST(GET VERSION_PARTS 2 ${VERSION_MAINTENANCE})
MACRO(GET_APP_VERSION GIT_DESCRIBE VERSION_MAJOR VERSION_MINOR VERSION_MAINTENANCE)
STRING(REGEX MATCH "v([0-9]+)[.]([0-9]+)[.]([0-9]+)" GIT_DESCRIBE_MATCH ${GIT_DESCRIBE})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose that you can't use \d and \d? Because this will also match 0x3#2, it doesn't enforce the dots, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think cmake has predefined character classes like \d, at least it's not documented: https://cmake.org/cmake/help/v3.0/command/string.html

However, at first I tried using \. to match a literal ., and it seems that using backslashes in this regex cause visual studio to break with a "can't parse escape sequence" error... typical cmake nonsense!

I think the [.] bit will only match literal . characters though, which is what we want. Characters that normally have special meanings like . are just literals when used in a character class ,the only special one is - for ranges.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright!

SET(${VERSION_MINOR} ${CMAKE_MATCH_2})
SET(${VERSION_MAINTENANCE} ${CMAKE_MATCH_3})
ELSE()
MESSAGE(WARNING "Couldn't parse major/minor/maintenance versions from git describe output '${GIT_DESCRIBE}'")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please let it fail in this case.

LIST(GET VERSION_PARTS 1 ${VERSION_MINOR})
LIST(GET VERSION_PARTS 2 ${VERSION_MAINTENANCE})
MACRO(GET_APP_VERSION GIT_DESCRIBE VERSION_MAJOR VERSION_MINOR VERSION_MAINTENANCE)
STRING(REGEX MATCH "v([0-9]+)[.]([0-9]+)[.]([0-9]+)" GIT_DESCRIBE_MATCH ${GIT_DESCRIBE})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check if ${GIT_DESCRIBE} is empty and set to a default of 0.0.0.

#define BUILD_TYPE "@APP_BUILD_TYPE@"
#define BUILD_ID_STR "@GIT_DESCRIBE@ [@APP_BUILD_TYPE@]"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please get rid of the square brackets. Now that I saw them in the welcome frame, I think they look out of place.

@kduske kduske merged commit daf1228 into release/v2.0.0 Jan 4, 2017
@kduske kduske deleted the feature/1601 branch November 6, 2017 20:23
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.

None yet

2 participants