Override v2.13.0-555-g11e37a0bd from git describe with v2.13.6 #9649

Al2Klimov · 2023-02-02T14:25:55Z

Before

[root@localhost ~]# icinga2 --version
icinga2 - The Icinga 2 network monitoring daemon (version: r2.13.0-1)

After

RPM

[root@localhost ~]# icinga2 --version
icinga2 - The Icinga 2 network monitoring daemon (version: v2.13.0-556-ga1fd8e465)

.deb

No improved. What the heck is happening here?

Windows

PS C:\Users\aklimov>  & 'C:\Program Files\ICINGA2\sbin\icinga2.exe' --version
icinga2.exe - The Icinga 2 network monitoring daemon (version: v2.13.0-556-ga1fd8e465)

Al2Klimov · 2023-02-02T14:38:56Z

Ah, here we go:

https://git.icinga.com/packages/icinga2/-/blob/9281beaddea263041f2fafe7944c26abc0236f41/debian/rules#L70

Al2Klimov · 2023-02-02T15:31:14Z

TODO

https://git.icinga.com/packages/icinga2/-/merge_requests/4

It unlocks these changes to take effect:

root@c75443973575:/# icinga2 --version
icinga2 - The Icinga 2 network monitoring daemon (version: v2.13.0-556-ga1fd8e465)

julianbrost · 2023-02-03T09:54:28Z

We just had a discussion on this which I'll try to summarize here:

The package tooling was recently changed to generate versions like 2.13.6+573.g2ea866248-1675163600 for snapshot packages (note the .6 instead of .0 as it would be produced by git describe). The suggestion we came up with to make package versions and --version output somewhat consistent was to base the non-release versions on a hardcoded version string (should be 2.13.6 now and has to be updated in master as part of the release workflow) and augment it with the commit hash for telling the exact version. The number of commits in between is removed as it's mostly relevant for ordering different versions required by package managers, should not be a big deal if this misses in --version.

My suggestion would be to take this as an opportunity to entirely replace the current ICINGA2_VERSION file which seems to be a leftover from when the rpm spec file was still part of this repository and the version number was parsed from there. Instead, I would add a file like version.cmake that just sets a variable to the hardcoded version and another one to the commit placeholder.

Additionally, the CMake files have to be updated to also use that information instead of git describe when building directly from a git checkout.

As an additional feature, it would be nice if the commit date was included in icinga2 --version as this provides a simple way for the average user to judge how old a version is.

julianbrost · 2023-02-03T11:22:01Z

From a quick look at the code, this doesn't seem to do what we discussed earlier, i.e. it still uses git describe. If there's a reason for this, please explain it.

Al2Klimov · 2023-02-03T11:25:26Z

It includes commits count and commit hash in the version so it's formatted similar to the packages.

julianbrost

As per #9649 (comment), I'd still add something like set(ICINGA2_VERSION "2.13.7") to git-archive-version.cmake and then rename the file to something like version.cmake (I haven't checked if that variable or file name would cause name conflicts) and remove the ICINGA2_VERSION file.

julianbrost · 2023-02-21T17:03:15Z

git-archive-version.cmake

@@ -0,0 +1 @@
+set(GIT_ARCHIVE_VERSION [=[$Format:%(describe)$]=])


A small comment that this version is set by git archive wouldn't hurt (and phrasing it something like that would make it correct in both cases, i.e. when looking at the file with the placeholder and the file with the actual describe string).

julianbrost · 2023-02-21T17:07:14Z

CMakeLists.txt

+include(git-archive-version.cmake)
+if(GIT_VERSION MATCHES "-NOTFOUND$" AND NOT GIT_ARCHIVE_VERSION MATCHES "Format")
+  set(GIT_VERSION "${GIT_ARCHIVE_VERSION}")
+endif()


Do I understand correctly that this prefers GetGitRevisionDescription and GIT_ARCHIVE_VERSION set by git archive is only used as a fallback?

If both find a version, I'd prefer the one from git archive as if that's set, it means that the current source tree went through git archive. If GetGitRevisionDescription additionally finds commit information, odds are that this has been imported in some other git which probably doesn't have the version information we want here.

This would also mean that if GIT_ARCHIVE_VERSION was set, we wouldn't even have to bother GetGitRevisionDescription.

Al2Klimov · 2023-02-22T12:22:06Z

Windows

https://git.icinga.com/packaging/windows-icinga2/-/pipelines/28960

PS C:\Users\aklimov> & 'C:\Program Files\ICINGA2\sbin\icinga2.exe' --version
icinga2.exe - The Icinga 2 network monitoring daemon (version: v2.13.42-645-g3abc7a704)

Al2Klimov · 2023-02-22T12:31:20Z

Debian

root@3fde8a90b3c0:/i2# icinga2 --version
icinga2 - The Icinga 2 network monitoring daemon (version: v2.13.42-645-g3abc7a704)

Al2Klimov · 2023-02-22T12:35:40Z

CentOS 7

[root@3b254eab9e9f /]# icinga2 --version
icinga2 - The Icinga 2 network monitoring daemon (version: [=[v2.13.0-645-g3abc7a704]=])

😅

Al2Klimov · 2023-02-22T13:34:25Z

Do you still think CMakeficating the ~~version~~ git archive version file is a good idea?

julianbrost · 2023-02-23T13:45:00Z

(version: [=[v2.13.0-645-g3abc7a704]=])

What's going on here? Is that [=[ syntax (which I haven't seen before by the way) only supported by newer versions of CMake or something like that?

Do you still think CMakeficating the ~~version~~ git archive version file is a good idea?

Would there be anything wrong with just using regular " for the string literal? Neither " nor ]=] should ever show up in a version number.

(version: v2.13.42-645-g3abc7a704)

Also, where does this 42 come from?

Al2Klimov · 2023-02-23T14:01:23Z

(version: [=[v2.13.0-645-g3abc7a704]=])

What's going on here? Is that [=[ syntax (which I haven't seen before by the way) only supported by newer versions of CMake or something like that?

Seems so.

Do you still think CMakeficating the ~~version~~ git archive version file is a good idea?

Would there be anything wrong with just using regular " for the string literal? Neither " nor ]=] should ever show up in a version number.

But then we had $ inside " ". A variable expansion.

(version: v2.13.42-645-g3abc7a704)

Also, where does this 42 come from?

My version file bump for PoC sake.

julianbrost · 2023-02-23T14:41:53Z

TIL: we're actually still using CMake < 3.0 anywhere (that is 2.8.12 on CentOS 7 (and therefore probably also RHEL 7 but I haven't checked explicitly)). Then doing it inside of CMake files would be pretty annoying, so extra files it is.

Al2Klimov · 2023-02-23T15:29:28Z

TODO

Al2Klimov · 2023-02-24T09:56:37Z

WIndows

Al2Klimov · 2023-02-24T10:02:15Z

Debian

still has problems. But this PR does what it shall do.

Setting up icinga2-bin (2.13.0+643.ge85b82934-1+debian11) ...
enabling default icinga2 features
Enabling feature checker. Make sure to restart Icinga 2 for these changes to take effect.
Enabling feature notification. Make sure to restart Icinga 2 for these changes to take effect.
Enabling feature mainlog. Make sure to restart Icinga 2 for these changes to take effect.
Processing triggers for libc-bin (2.31-13+deb11u5) ...
Processing triggers for ca-certificates (20210119) ...
Updating certificates in /etc/ssl/certs...
0 added, 0 removed; done.
Running hooks in /etc/ca-certificates/update.d...
done.
root@0e8a6aa0b46b:/# icinga2 --version
icinga2 - The Icinga 2 network monitoring daemon (version: v2.13.42-643-ge85b82934)

Al2Klimov · 2023-02-24T10:05:10Z

CentOS

same.

Installed:
  icinga2.x86_64 0:2.13.0+643.ge85b82934-1.el7

Dependency Installed:
  boost169-chrono.x86_64 0:1.69.0-2.el7                           boost169-context.x86_64 0:1.69.0-2.el7
  boost169-coroutine.x86_64 0:1.69.0-2.el7                        boost169-date-time.x86_64 0:1.69.0-2.el7
  boost169-filesystem.x86_64 0:1.69.0-2.el7                       boost169-iostreams.x86_64 0:1.69.0-2.el7
  boost169-program-options.x86_64 0:1.69.0-2.el7                  boost169-regex.x86_64 0:1.69.0-2.el7
  boost169-system.x86_64 0:1.69.0-2.el7                           boost169-thread.x86_64 0:1.69.0-2.el7
  icinga2-bin.x86_64 0:2.13.0+643.ge85b82934-1.el7                icinga2-common.x86_64 0:2.13.0+643.ge85b82934-1.el7
  libedit.x86_64 0:3.0-12.20121213cvs.el7                         libicu.x86_64 0:50.2-4.el7_7

Complete!
[root@c94b14ea13af /]# icinga2 --version
icinga2 - The Icinga 2 network monitoring daemon (version: v2.13.42-643-ge85b82934)

Al2Klimov · 2023-02-24T10:09:02Z

openSUSE

Surprise, surprise...

(48/48) Installing: icinga2-ido-pgsql-2.13.0+643.ge85b82934-1.x86_64 ..........................................................[done]
Created symlink /etc/systemd/system/default.target.wants/ca-certificates.path -> /usr/lib/systemd/system/ca-certificates.path.
Created symlink /etc/systemd/system/getty.target.wants/[email protected] -> /usr/lib/systemd/system/[email protected].
Created symlink /etc/systemd/system/multi-user.target.wants/kbdsettings.service -> /usr/lib/systemd/system/kbdsettings.service.
Created symlink /etc/systemd/system/multi-user.target.wants/remote-fs.target -> /usr/lib/systemd/system/remote-fs.target.
Created symlink /etc/systemd/user/timers.target.wants/systemd-tmpfiles-clean.timer -> /usr/lib/systemd/user/systemd-tmpfiles-clean.timer.
Created symlink /etc/systemd/user/basic.target.wants/systemd-tmpfiles-setup.service -> /usr/lib/systemd/user/systemd-tmpfiles-setup.service.
Executing %posttrans scripts ..................................................................................................[done]
fc6ee6cc9f1b:/ # icinga2 --version
icinga2 - The Icinga 2 network monitoring daemon (version: v2.13.42-643-ge85b82934)

Al2Klimov · 2023-02-24T10:20:34Z

If we build all products' packages from one system, wouldn't it be smarter that the version is overridden only at one place – that one system? I.e. it assembles the version as needed and places it in a PRODUCT_VERSION file (literally) into the source. CMake reads it and compiles it in, Go go:embeds it and PHP... does its PHP things. This way we wouldn’t have to cherry-pick the version files.

julianbrost · 2023-02-24T10:45:29Z

If we build all products' packages from one system

Which we don't do, prime example: docker images. Also doesn't hurt to have proper version information in local development builds or in externally maintained packages.

Al2Klimov · 2023-02-24T12:30:49Z

Oh, our images could do the same as packages.
Forget local development builds. We know what we do.
If you mean e.g. the BSD ports with external, well, IMAO it's their task to get things as they desire.

lippserd · 2023-02-24T13:47:55Z

If we build all products' packages from one system, wouldn't it be smarter that the version is overridden only at one place – that one system? I.e. it assembles the version as needed and places it in a PRODUCT_VERSION file (literally) into the source. CMake reads it and compiles it in, Go go:embeds it and PHP... does its PHP things. This way we wouldn’t have to cherry-pick the version files.

I'd like too keep version files for the moment instead of changing everything.

Al2Klimov · 2023-02-24T13:56:54Z

Then what's your plan on getting the package version right? (See my three comments starting with #9649 (comment) ) Lookup the latest tag in GitLab CI and merge like I did here?

Al2Klimov · 2023-04-05T12:44:27Z

@julianbrost Do you agree with me that we shall first get the package versions right in GitLab and then continue here with fixing icinga2 --version?

julianbrost · 2023-04-11T15:15:04Z

What would you change there? At the moment, it generates version numbers like 2.13.7+654.g41065e30c which looks fine.

Al2Klimov · 2023-04-12T09:36:29Z

Yes, almost.

with 2.13.7 from ./ICINGA2_VERSION -> v2.13.7-555-g11e37a0bd

Al2Klimov added bug Something isn't working area/cli Command line helpers consider backporting Should be considered for inclusion in a bugfix release labels Feb 2, 2023

Al2Klimov added this to the 2.14.0 milestone Feb 2, 2023

Al2Klimov self-assigned this Feb 2, 2023

cla-bot bot added the cla/signed label Feb 2, 2023

Al2Klimov marked this pull request as ready for review February 2, 2023 15:31

Al2Klimov removed their assignment Feb 2, 2023

Al2Klimov requested a review from julianbrost February 3, 2023 10:56

julianbrost requested changes Feb 21, 2023

View reviewed changes

Al2Klimov force-pushed the git-archive-version.cmake branch from 7971188 to 4e52b82 Compare February 22, 2023 11:15

Al2Klimov changed the title ~~If ./.git/ missing, fall back to git archive generated version if any~~ Prefer git archive generated version if any Feb 22, 2023

Al2Klimov self-assigned this Feb 23, 2023

Al2Klimov force-pushed the git-archive-version.cmake branch from 4e52b82 to d3fc61f Compare February 23, 2023 15:26

Al2Klimov removed their assignment Feb 24, 2023

Al2Klimov changed the title ~~Prefer git archive generated version if any~~ Override v2.13.0-555-g11e37a0bd from git describe with v2.13.6 Feb 24, 2023

Al2Klimov requested review from lippserd and removed request for lippserd February 24, 2023 10:20

Al2Klimov assigned lippserd Feb 24, 2023

Al2Klimov added the needs feedback We'll only proceed once we hear from you again label Feb 24, 2023

lippserd removed their assignment Feb 24, 2023

lippserd removed the needs feedback We'll only proceed once we hear from you again label Feb 24, 2023

lippserd assigned Al2Klimov Mar 30, 2023

Al2Klimov removed their assignment Apr 12, 2023

Al2Klimov requested a review from julianbrost April 12, 2023 11:10

Al2Klimov removed this from the 2.14.0 milestone May 5, 2023

Al2Klimov removed the consider backporting Should be considered for inclusion in a bugfix release label Dec 13, 2023

Al2Klimov added 2 commits February 3, 2025 16:17

Prefer git archive generated version if any

90a5306

Override v2.13.0-555-g11e37a0bd from git describe

9509027

with 2.13.7 from ./ICINGA2_VERSION -> v2.13.7-555-g11e37a0bd

Al2Klimov force-pushed the git-archive-version.cmake branch from d3fc61f to 9509027 Compare February 3, 2025 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override v2.13.0-555-g11e37a0bd from git describe with v2.13.6 #9649

Override v2.13.0-555-g11e37a0bd from git describe with v2.13.6 #9649

Al2Klimov commented Feb 2, 2023

Al2Klimov commented Feb 2, 2023

Al2Klimov commented Feb 2, 2023

julianbrost commented Feb 3, 2023 •

edited

Loading

julianbrost commented Feb 3, 2023

Al2Klimov commented Feb 3, 2023

julianbrost left a comment

julianbrost Feb 21, 2023

julianbrost Feb 21, 2023

Al2Klimov commented Feb 22, 2023

Al2Klimov commented Feb 22, 2023

Al2Klimov commented Feb 22, 2023

Al2Klimov commented Feb 22, 2023 •

edited

Loading

julianbrost commented Feb 23, 2023

Al2Klimov commented Feb 23, 2023

julianbrost commented Feb 23, 2023

Al2Klimov commented Feb 23, 2023 •

edited

Loading

Al2Klimov commented Feb 24, 2023

Al2Klimov commented Feb 24, 2023

Al2Klimov commented Feb 24, 2023

Al2Klimov commented Feb 24, 2023

Al2Klimov commented Feb 24, 2023

julianbrost commented Feb 24, 2023

Al2Klimov commented Feb 24, 2023

lippserd commented Feb 24, 2023

Al2Klimov commented Feb 24, 2023

Al2Klimov commented Apr 5, 2023

julianbrost commented Apr 11, 2023

Al2Klimov commented Apr 12, 2023

		@@ -0,0 +1 @@
		set(GIT_ARCHIVE_VERSION [=[$Format:%(describe)$]=])

Override v2.13.0-555-g11e37a0bd from git describe with v2.13.6 #9649

Are you sure you want to change the base?

Override v2.13.0-555-g11e37a0bd from git describe with v2.13.6 #9649

Conversation

Al2Klimov commented Feb 2, 2023

Before

After

RPM

.deb

Windows

Al2Klimov commented Feb 2, 2023

Al2Klimov commented Feb 2, 2023

TODO

julianbrost commented Feb 3, 2023 • edited Loading

julianbrost commented Feb 3, 2023

Al2Klimov commented Feb 3, 2023

julianbrost left a comment

Choose a reason for hiding this comment

julianbrost Feb 21, 2023

Choose a reason for hiding this comment

julianbrost Feb 21, 2023

Choose a reason for hiding this comment

Al2Klimov commented Feb 22, 2023

Windows

Al2Klimov commented Feb 22, 2023

Debian

Al2Klimov commented Feb 22, 2023

CentOS 7

Al2Klimov commented Feb 22, 2023 • edited Loading

julianbrost commented Feb 23, 2023

Al2Klimov commented Feb 23, 2023

julianbrost commented Feb 23, 2023

Al2Klimov commented Feb 23, 2023 • edited Loading

TODO

Al2Klimov commented Feb 24, 2023

WIndows

Al2Klimov commented Feb 24, 2023

Debian

Al2Klimov commented Feb 24, 2023

CentOS

Al2Klimov commented Feb 24, 2023

openSUSE

Al2Klimov commented Feb 24, 2023

julianbrost commented Feb 24, 2023

Al2Klimov commented Feb 24, 2023

lippserd commented Feb 24, 2023

Al2Klimov commented Feb 24, 2023

Al2Klimov commented Apr 5, 2023

julianbrost commented Apr 11, 2023

Al2Klimov commented Apr 12, 2023

julianbrost commented Feb 3, 2023 •

edited

Loading

Al2Klimov commented Feb 22, 2023 •

edited

Loading

Al2Klimov commented Feb 23, 2023 •

edited

Loading