Details

Reviewers

brauch
kfunk
mwolff
rjvbb

Group Reviewers

KDevelop

Summary

This patch adds and invokes a script that will decorate the patch version string of development builds with the output from git describe --tags so that it includes the exact version of the code currently checked out.

This is something I often miss when looking at the version info in software installed from a development (git) snapshot, and I think should be useful in bug reports too.

The script returns the unchanged patch version if there is no .git directory, so this should be safe in release builds.

I'm not doing anything to the version reported for KDevPlatform; as far as I can tell this needs to be a purely numerical 3-level string.

Test Plan

Works as intended in my builds from the 5.2 branch. For instance, a triggered bug report generated by DrKonqi now has this as the 1st 2 lines:

Application: kdevelop (5.1.80-24-g6e3cfe2421)
 (Compiled from sources)

Diff Detail

Lint

Lint Skipped

Unit

Unit Tests Skipped

rjvbb created this revision.Oct 6 2017, 9:47 AM

Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptOct 6 2017, 9:47 AM

This only works on UNIX
This adds a hard build-time-dependency on git, doesn't it?

This revision now requires changes to proceed.Oct 6 2017, 10:45 AM

This won't need to use a script and sed if future tags drop the leading 'v' character (or if it's acceptable to have a non-numeric full version string).

The dependency on git only exists if the .git directory exists. I've taken that as an indication that git is already installed, but we can easily make this conditional on git being available. The extra information is helpful but not crucial.

Future tags won't drop the v, because all KDE apps always have the v there.

Does drkonqui still automatically select the right version in Bugzilla if we do this ...? (It does that, right?)

Looking at it, no, you definitely cannot do it like this. You overwrite KDevelop_VERSION_PATCH, which is e.g. exported to kdevelop-config.cmake and will require e.g. kdev-php to depend on "kdevelop VERSION 5.1.80-24-g5623457794"...

Also, you run this at build generation time, so if you do a change + commit which don't cause cmake to re-generate the build system, the version number won't change.

Also, as said, this will just cause the build to fail on Windows.

Does drkonqui still automatically select the right version in Bugzilla if we do this ...? (It does that, right?)

I haven't gone so far as to submit an actual bug report. I'd hope that some kind of pattern matching is done, either in DrKonqi or by the Bugzilla software itself. Which btw. only seems to know release versions.

Looking at it, no, you definitely cannot do it like this. You overwrite KDevelop_VERSION_PATCH, which is e.g. exported to kdevelop-config.cmake and will require e.g. kdev-php to depend on "kdevelop VERSION 5.1.80-24-g5623457794"...

I wasn't sure about that because I can find no evidence that this version is exported in a headerfile or .cmake file. What did I overlook?
Is it safer to rewrite just the full version string (and would it be acceptable/safe to include the 'v' in there - if not it can be removed in software)?

Also, you run this at build generation time, so if you do a change + commit which don't cause cmake to re-generate the build system, the version number won't change.

Also, as said, this will just cause the build to fail on Windows.

How many bug reports do you get that are generated automatically on MS Windows (via DrKonqi or the built-in bug reporter)?

I see 2 options, presuming that the feature is deemed of interest:
1 exclude MS Windows, which would make it possible (or simpler) to write something that always updates the full version string
2 don't a helper script, execute git directly from the CMake file to rewrite just a full version string used by KAboutData, and do the required post-processing in main.cpp

With option 2) it would be the developer's responsibility to take care of regenerating the version string, for instance by removing $build.dir/kdevelop-config.h in post-commit and post-fetch hooks. We could even provide instructions for that.

why?

Why not?

Don't you think it's useful to have get version indication from the binary that indicates what source version it was built from?

Yeah, I'd find this useful in principle. I don't always remember exactly when I last built/installed my system copy, which can be inconvenient when looking to see which recent commits might be responsible for a problem.

+1 for the option to note the git version somethere
-1 for the approach in the patch.

For getting the git version, you could have a look at the approach done e.g. in calligra or krita,have a look at:
https://cgit.kde.org/calligra.git/tree/CMakeLists.txt
https://cgit.kde.org/calligra.git/tree/cmake/modules/GetGitRevisionDescription.cmake

I would propose to put the very git version only in the UI aboutdata strings and also only for development builds (which opens the question how to define whether it is a development build).

I would propose to put  the very git version only in the UI aboutdata strings and also only for development builds

That's what I intended, using the presence of the .git directory as a criterium. When using git describe you still get the regular release version string if you're exactly on the commit corresponding to a release.

Updated following Friedrich's suggestion.

The output from git describe is now used for a new variable that is only used by KAboutData; the leading 'v' is stripped by CMake. If the command fails (silently) that variable is set to MAJOR,MINOR.PATCH, i.e. identical to the existing VERSION macro.

I haven't tested if this indeed regenerates the version string after each commit (or fetch).

rjvbb set the repository for this revision to R32 KDevelop.Oct 6 2017, 3:01 PM

Complete revision.
(Shooting off the hip is not recommended)

rjvbb set the repository for this revision to R32 KDevelop.Oct 6 2017, 3:08 PM

kfunk requested changes to this revision.Oct 6 2017, 4:57 PM

This revision now requires changes to proceed.Oct 6 2017, 4:57 PM

In D8158#152647, @rjvbb wrote:

Does drkonqui still automatically select the right version in Bugzilla if we do this ...? (It does that, right?)

I haven't gone so far as to submit an actual bug report. I'd hope that some kind of pattern matching is done, either in DrKonqi or by the Bugzilla software itself. Which btw. only seems to know release versions.

Please don't hope -- test it. If the problem is a non-existing release version, fake a released version for testing.

Looking at it, no, you definitely cannot do it like this. You overwrite KDevelop_VERSION_PATCH, which is e.g. exported to kdevelop-config.cmake and will require e.g. kdev-php to depend on "kdevelop VERSION 5.1.80-24-g5623457794"...

I wasn't sure about that because I can find no evidence that this version is exported in a headerfile or .cmake file. What did I overlook?
Is it safer to rewrite just the full version string (and would it be acceptable/safe to include the 'v' in there - if not it can be removed in software)?

Also, you run this at build generation time, so if you do a change + commit which don't cause cmake to re-generate the build system, the version number won't change.

Also, as said, this will just cause the build to fail on Windows.

How many bug reports do you get that are generated automatically on MS Windows (via DrKonqi or the built-in bug reporter)?

There's no DrKonqi on Windows. But people are using the bug reporter. Not that it matters in this discussion though. We'd like to have cross-platform solutions.

But that point is now moot anyway, now that you use that CMake module.

I see 2 options, presuming that the feature is deemed of interest:
1 exclude MS Windows, which would make it possible (or simpler) to write something that always updates the full version string
2 don't a helper script, execute git directly from the CMake file to rewrite just a full version string used by KAboutData, and do the required post-processing in main.cpp

With option 2) it would be the developer's responsibility to take care of regenerating the version string, for instance by removing $build.dir/kdevelop-config.h in post-commit and post-fetch hooks. We could even provide instructions for that.

app/main.cpp
379	Unintended change.

In D8158#152716, @kfunk wrote:

Please don't hope -- test it. If the problem is a non-existing release version, fake a released version for testing.

https://bugs.kde.org/show_bug.cgi?id=385450

Also works with a hardwired 5.1.2-21-gcfad738 version string.

Removed the unintended hunk.

rjvbb set the repository for this revision to R32 KDevelop.Oct 6 2017, 8:57 PM

I can now confirm that the full version string is indeed re-determined after a git pull incorporated upstream changes.

This version moves the new version string macro to a dedicated headerfile which is included only in the single file using it (app/main.cpp). This avoids unnecessary rebuilds.
(main.cpp is quite big, is it worth the trouble moving the KAboutData set-up to a dedicated file?)

rjvbb set the repository for this revision to R32 KDevelop.Oct 8 2017, 8:50 AM

kfunk requested changes to this revision.Oct 8 2017, 9:08 AM

kfunk added inline comments.

CMakeLists.txt
133	Please just amend the version as specified by CMake (that is, re-use VERSION_MAJOR, VERSION_MINOR, VERSION_PATCH). We don't want to fully overwrite the things set in CMakeLists.txt. Something along: `set(KDevelop_FULLVERSION_STRING "${KDEVELOP_VERSION_STRING} (git ${SHA1}")` Please check what Krita does in their CMakeLists.txt, I'd like something similar. FWIW, they use this to get the SHA1: `get_git_head_revision(GIT_REFSPEC GIT_SHA1)`
134	Print this in a more human-readable way, i.e. `Setting KDevelop version to: ...`
cmake/modules/GetGitRevisionDescription.cmake
2	This copy is not current. I'm getting: makeobj[0]: Entering directory `/home/kfunk/devel/build/kf5/kdevelop-5.2' [0/1] Re-running CMake... CMake Warning at cmake/modules/GetGitRevisionDescription.cmake:121 (message): " describe 8bab57d14644f7f1d2a263a243745014bce5745c " failed with exit code No such file or directory, out= Call Stack (most recent call first): CMakeLists.txt:124 (git_describe) Please use the most recent version from https://github.com/rpavlik/cmake-modules/blob/master (it seems to work fine with that version from there).
kdevelop-fullversion.h.cmake
1 ↗	(On Diff #20464)	kdevelop-fullversion.h -> kdevelop_fullversion.h. `FULLVERSION` -> `KDevelop_FULLVERSION_STRING`. Then this is consistent with kdevelop_version.h as generated by `ecm_setup_version()`.

This revision now requires changes to proceed.Oct 8 2017, 9:08 AM

rjvbb added inline comments.Oct 8 2017, 10:54 AM

CMakeLists.txt
133	We're not overwriting anything, we're adding an additional full version string. That's not unheard of, this is exactly what's done in Apple's Info.plist files. A "numeric" version for use in code, and a human-readable string that is to be presented to users. I can achieve the same thing the way Krita does things, but that's more complicated (cf. the script I used in the initial release). Sven pointed out that we cannot change VERSION_PATCH because that variable is used by dependent code. Whatever approach I will use I find it crucial that the output from `git describe` is used. Git hashes are random, so using only them will not allow checking at a glance which of two versions is older or newer (and that's part of what I want to achieve with this patch).
cmake/modules/GetGitRevisionDescription.cmake
2	Sorry, I am working against the 5.2 branch (it's not a huge change that introduces wildly different behaviour after all).
kdevelop-fullversion.h.cmake
1 ↗	(On Diff #20464)	Shouldn't config-kdevelop.h be made consistent too then?

kfunk added inline comments.Oct 8 2017, 12:02 PM

CMakeLists.txt
133	We're not overwriting anything, we're adding an additional full version string. That's not unheard of, this is exactly what's done in Apple's Info.plist files. A "numeric" version for use in code, and a human-readable string that is to be presented to users. You're basically overwriting/invalidating the `major.minor.patch` version which was previously shown in the About dialog as set by CMake. I can achieve the same thing the way Krita does things, but that's more complicated (cf. the script I used in the initial release). Sven pointed out that we cannot change VERSION_PATCH because that variable is used by dependent code. It's not more complicated. Please forget about Krita again, it probably only confuses you. And where did I talk about changing VERSION_PATCH? `set(KDevelop_FULLVERSION_STRING "${KDEVELOP_VERSION_STRING} (git ${SHA1}")` doesn't "change" VERSION_PATCH. Whatever approach I will use I find it crucial that the output from git describe is used. Git hashes are random, so using only them will not allow checking at a glance which of two versions is older or newer (and that's part of what I want to achieve with this patch). Okay, then please use: `set(KDevelop_FULLVERSION_STRING "${KDEVELOP_VERSION_STRING} (git ${GIT_FULL_VERSION}")` That'll show e.g. '5.1.2' twice, but that doesn't harm.
cmake/modules/GetGitRevisionDescription.cmake
2	Hm? I was asking you to update the copy of those GetGit*.cmake files. What does that have to do with KDevelop's 5.2 branch?
kdevelop-fullversion.h.cmake
1 ↗	(On Diff #20464)	Not as part of this patch, no. Unrelated.

rjvbb added inline comments.Oct 8 2017, 12:58 PM

CMakeLists.txt
133	I interpreted `re-use VERSION_MAJOR, VERSION_MINOR, VERSION_PATCH` to suggest that the fuller version should be obtained from them directly. I'll play with `string replace`, it should be possible to remove `${MAJOR}.${MINOR}.${PATCH}` from the `git describe` return. That way the full version won't show 5.x.y twice if the version from the CMake version agrees with the current tag. Duplicate may not harm but avoiding them is more elegant, don't you think?
cmake/modules/GetGitRevisionDescription.cmake
2	My bad, I read too quickly. But then I also don't understand why you're getting an error and I don't.

rjvbb marked 4 inline comments as done.Oct 8 2017, 2:40 PM

Updated as discussed.

The full version string is now determined with a more elaborate mechanism:

KDevelop_FULL_VERSION is set to ${MAJOR}.${MINOR}.${PATCH}. This is the string used in builds from source trees lacking the .git directory.
GIT_FULL_VERSION is set to the output of git describe --tags

If 2) succeeds:

v${KDevelop_FULL_VERSION} is removed from ${GIT_FULL_VERSION}
if the result is different (removal succeeded), it is appended to KDevelop_FULL_VERSION, giving a single version "number" if this doesn't succeed, GIT_FULL_VERSION is appended to KDevelop_FULL_VERSION (in parentheses).

Steps 3&4 will generate a full version string that's either of the form x.y.z-cnt-gSHA1 or x.y.z if HEAD points to a release version.

rjvbb set the repository for this revision to R32 KDevelop.Oct 8 2017, 2:57 PM

Note: Change now LGTM (though I have some local changes). We still have a problem, thus can't merge as-is: When reporting a bug via Help -> Report Bug, and when KAboutData's version is filled with a git-describe like string, the 'version' field on bugs.kde.org is no longer preselected properly.

I've triggered a discussion about this on kde-frameworks-devel: https://mail.kde.org/pipermail/kde-frameworks-devel/2017-October/050178.html with some ideas how to resolve this nicely.

Rename variable slightly

That's strange, I tested this (yesterday?) and it worked fine. I did use a faked 5.1.2 version (bare and with git-describe decoration), and got the 5.1.2 version selected on BKO. There was no 5.1.80 version yet.

When I try now (with 5.1.80-21-gcfad738) I still get 5.1.2 selected, so my test may have passed purely by chance.

Did you try if a bare-bones 5.1.80 version string gives the desired result?!

Not sure what you tried. This logic is very simple.

The KDE bug report dialog (in kxmlgui.git) will generate this URL:
https://bugs.kde.org/enter_bug.cgi?format=guided&product=kdevelop&version=$VERSION

$VERSION is replace by KAboutData's version attribute.

For instance:
https://bugs.kde.org/enter_bug.cgi?format=guided&product=kdevelop&version=5.1.80 => pre-selects '5.1.80'
https://bugs.kde.org/enter_bug.cgi?format=guided&product=kdevelop&version=5.1.80-foo => pre-selects 'unspecified'

I personally think it's useless to have this, but if others want to have it, fair enough. I just hate that this means a partial rebuild + relink when committing/rebasing without changing code.

I also personally think that this functionality, if desired, should be moved to a central place, maybe extra-cmake-modules.

kdevelop_fullversion.h.cmake
1 ↗	(On Diff #20488)	missing license header

This revision now requires changes to proceed.Oct 8 2017, 7:00 PM

In D8158#153432, @mwolff wrote:

I personally think it's useless to have this, but if others want to have it, fair enough. I just hate that this means a partial rebuild + relink when committing/rebasing without changing code.

Well, that's needed unfortunately. Though for us that's really just the kdevelop target which needs to be rebuilt.

I also personally think that this functionality, if desired, should be moved to a central place, maybe extra-cmake-modules.

Agreed. That functionality could be part of ECMSetupVersion.cmake some day (opt-in of course).

https://bugs.kde.org/enter_bug.cgi?format=guided&product=kdevelop&version=5.1.80-foo => pre-selects 'unspecified'

Not for me, I get 5.1.2 preselected for anything that's not known. So logically when I tried 5.1.2-10-gaga I also got 5.1.2 . I concluded that BKO does simple pattern matching...

In D8158#153441, @rjvbb wrote:
https://bugs.kde.org/enter_bug.cgi?format=guided&product=kdevelop&version=5.1.80-foo => pre-selects 'unspecified'
Not for me, I get 5.1.2 preselected for anything that's not known. So logically when I tried 5.1.2-10-gaga I also got 5.1.2 . I concluded that BKO does simple pattern matching...

[21:15:28] <milian> I get "git master"
[21:16:44] <milian> maybe the last used version?
[21:19:06] <FLHerne> kfunk: I get '5.1.0'
[21:19:10] <FLHerne> (consistently...)
[21:19:15] <kfunk> maybe last used, indeed
[21:19:32] <kfunk> whatever, for sure, it doesn't do 'pattern matching' with the version string supplied

Well, that's needed unfortunately. Though for us that's really just the kdevelop target which needs to be rebuilt.

As I said, we can put the method that sets up KAboutData in its own implementation file, to avoid rebuilding all of main.cpp . Of course that probably won't change a log if you use LTO (you'd still not be rebuilding all of main.cpp twice).

rjvbb marked 2 inline comments as done.Oct 8 2017, 8:32 PM

kfunk updated this revision to Diff 20488.
Rename variable slightly

Where did that go on phab page? What variable, what new name?

Changed the casing of the KDEVELOP_FULL_VERSION_STRING define to make it consistent with the defines from kdevelop_version.h

Variable name case adapted as requested.

rjvbb set the repository for this revision to R32 KDevelop.Oct 9 2017, 9:11 AM

rebased

What happened to this one? Wasn't there some work forked out of this to extend KAboutData to support the additional VCS version info in some generic way?

In D8158#171757, @kossebau wrote:

What happened to this one? Wasn't there some work forked out of this to extend KAboutData to support the additional VCS version info in some generic way?

This Diff here is blocked -- we need the new API in KAboutData first. On my TODO list; you can happily take the task though.

Relevant mailing list discussion: https://www.mail-archive.com/kde-frameworks-devel@kde.org/msg49724.html

@rjvbb Hi, please Abandon this review, as the reply has been that this should be solved on KDE Frameworks level, in KAboutData (as well as with respective new ECM module for getting the VCS/git revision info into the buildsystem).

apol commandeered this revision.Jul 9 2019, 5:45 PM

apol abandoned this revision.

apol added a reviewer: rjvbb.

		Path
M		CMakeLists.txt (24 lines)
M		app/main.cpp (3 lines)
A	M	cmake/modules/GetGitRevisionDescription.cmake (168 lines)
A	M	cmake/modules/GetGitRevisionDescription.cmake.in (41 lines)
A	M	kdevelop_fullversion.h.cmake (27 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	20386			Oct 6 2017, 9:31 AM	★	★
Diff 2	20392		Updated following Friedrich's suggestion.	Oct 6 2017, 3:00 PM	★	★
Diff 3	20393		Complete revision.	Oct 6 2017, 3:07 PM	★	★
Diff 4	20405		Removed the unintended hunk.	Oct 6 2017, 8:56 PM	★	★
Diff 5	20464		This version moves the new version string macro to a dedicated headerfile which…	Oct 8 2017, 8:49 AM	★	★
Diff 6	20478		Updated as discussed.	Oct 8 2017, 2:57 PM	★	★
Diff 7	20488	cf498d9	Rename variable slightly	Oct 8 2017, 5:48 PM	★	★
Diff 8	20511		Variable name case adapted as requested.	Oct 9 2017, 9:10 AM	★	★
Diff 9	20535		rebased	Oct 9 2017, 7:32 PM	★	★

KDevelop: decorate patch version string in development builds
AbandonedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 20511

CMakeLists.txt

app/main.cpp

cmake/modules/GetGitRevisionDescription.cmake

cmake/modules/GetGitRevisionDescription.cmake.in

kdevelop_fullversion.h.cmake

KDevelop: decorate patch version string in development buildsAbandonedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 20511

CMakeLists.txt

app/main.cpp

cmake/modules/GetGitRevisionDescription.cmake

cmake/modules/GetGitRevisionDescription.cmake.in

kdevelop_fullversion.h.cmake

KDevelop: decorate patch version string in development builds
AbandonedPublic
Actions

Revision Contents
Changeset List