fix version recorded in releases #1047

germasch · 2020-09-24T17:35:41Z

If downloaded as a tarball release, there will be no info from git
to determine the release, so it ends up v0.0.0. If that's the case,
we'll now use the release specified in the project() command.

Fixes #1046. This requires the version to be updated at every release, but there's really no way around that since a release doesn't have access to the git history. I suppose it could be automated as part of a script that tags and prepares a new release.

google-cla · 2020-09-24T17:35:46Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

germasch · 2020-09-24T17:37:47Z

@googlebot I signed it!

LebedevRI · 2020-09-24T17:49:24Z

CMakeLists.txt

+if ("${GIT_VERSION}" STREQUAL "v0.0.0")
+  set(VERSION "${benchmark_VERSION}")
+else()
+  string(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" VERSION ${GIT_VERSION})
+endif()


This is fine if it works on it's own.

No, it doesn't work on its own, because when there's no version info from git, it has no way of knowing what to set VERSION to.

Close:(Google:master)

Close:(cla:no)

LebedevRI · 2020-09-24T17:50:49Z

CMakeLists.txt

@@ -13,7 +13,7 @@ foreach(p
  endif()
 endforeach()

-project (benchmark CXX)
+project (benchmark VERSION 1.5.2 LANGUAGES CXX)


While this will work, i'm not okay with this.
This will falsely claim that every build of every commit is the released version.
Such a change (this particular line) should be the last commit before release,
and reverted immediately after.

IOW, this should be documented in "how to release benchmark", but NOT changed now.

Well, it's a question of what you consider "claim". The cmake version for this project will be set to 1.5.2, which yes, is incorrect (or at least incomplete) after additional commits are added. But before, that version wasn't set at all, so it doesn't really have any impact in real life, it's not even printed.

What already happens right now is that if you install master as-is, in its install directory, the config file will contain a version of 1.5.2. git describe will say 1.5.2-<commit>, and -dirty may be added if there are changes in the work tree, but that info is only printed. What's recorded in the install is just 1.5.2, and yes, that's not correct, but it's already the case.

But anyway, being aware of this issue, the first solution I proposed in #1046 would not add a version to project() in master, so it would continue to determine the version based on git describe (with still the same issue that what's recorded is just the info from the tag, even if we're ahead of that tag). But it wouldn't change existing behavior at all. It would, however, require to make a branch at release time and add the particular release version to the project() command at that point, so that a tarball made from that release branch will install benchmark with the proper version info. (It's not really required to necessarily a branch, it could also be another part of the release process to add the version info project() before making the tarball, but in that case the tarball wouldn't correspond exactly to the tagged version).

It would, however, require to make a branch at release time and add the particular release version to the project() command at that point, so that a tarball made from that release branch will install benchmark with the proper version info.

Why would it require a branch?

I'm not a fan of creating a branch for a release, and i'm not a fan of adding a commit just to reverse it after the release. I think having the project include a "base version" and having git describe override that with a more detailed version is appropriate.

A release branch would allow you to add a commit that does

- project (benchmark CXX) + project (benchmark VERSION 1.5.2 LANGUAGES CXX)

outside of master.

The alternatives that I can see are:

add the commit above tomaster, tag the release, revert it.

don't ever commit the change above, but apply it locally and make the release tarball.

The version of the release has to become part of what's in the tarball in one way or other (it doesn't have to come from the project() command, either, but that's the standard way of specifying a version). I don't really care which way, but it'd be nice to be able to use a release via find_package(benchmark 1.5.2).

FWIW, the reason that I started looking at this is that benchmark=1.5.2 installed via conda thinks it's version 0.0.0.

how would it be wrong? if git version responds something then we override the project version with that (is that maybe not possible?).

ie, we should:

set the version to the last release in the project statement

call the script to calculate the git version and set that (or at least output it to the cmake logs... we only really need it for debugging and triage right?).

on release, update the project version to the new tag before tagging.

if we were on cmake 3.15 then we could use cmake_project_include_before to calculate a git version and return the "current version" if none is set, i think. then the release step would be to change the default version in that script.

how would it be wrong?

if git version responds something then we override the project version with that (is that maybe not possible?).

Unfortunately, that only works if the whole .git is there.
E.g. what's the version of https://github.com/google/benchmark/archive/master.zip ?
It's really ugly/messy to handle in full generality, that is why i'm suggesting pre-tag/post-tag commits.

ie, we should:

set the version to the last release in the project statement

call the script to calculate the git version and set that (or at least output it to the cmake logs... we only really need it for debugging and triage right?).

I guess. Packagers know what sources they've downloaded, and can and should use appropriate version in any case.

on release, update the project version to the new tag before tagging.

if we were on cmake 3.15 then we could use cmake_project_include_before to calculate a git version and return the "current version" if none is set, i think. then the release step would be to change the default version in that script.

how would it be wrong?

if git version responds something then we override the project version with that (is that maybe not possible?).

Unfortunately, that only works if the whole .git is there.
E.g. what's the version of https://github.com/google/benchmark/archive/master.zip ?

oh i hadn't thought of that one. so that would show the version as the previous release but all we'd know it was at a commit past that. and we don't have a dirty flag for that case. got it.

i wonder how much that's a problem really. if someone downloads the master.zip and installs it and then it doesn't work, we can suggest they get a tagged release (most would) or go from source (others would do that), which would both have appropriate versions set.

It's really ugly/messy to handle in full generality, that is why i'm suggesting pre-tag/post-tag commits.

ie, we should:

set the version to the last release in the project statement

call the script to calculate the git version and set that (or at least output it to the cmake logs... we only really need it for debugging and triage right?).

I guess. Packagers know what sources they've downloaded, and can and should use appropriate version in any case.

on release, update the project version to the new tag before tagging.

if we were on cmake 3.15 then we could use cmake_project_include_before to calculate a git version and return the "current version" if none is set, i think. then the release step would be to change the default version in that script.

Currently, benchmark does the following when installing from a git repo (this behavior is unchanged by this PR):

-- git Version: v1.5.2-7c7e3a75 -- Version: 1.5.2

When installed, the installed benchmarkConfigVersion.cmake has

set(PACKAGE_VERSION "1.5.2")

So the installed version does not record that it's from a version past the 1.5.2 release. Maybe this should be changed (but the next question then would be how you'd want it changed -- 1.5.2-dev? I'm not sure whether cmake would consider that to be compatible when 1.5.2 is requested. This would be a (possibly breaking) change from current behavior. If you do want to preserve that information, maybe a better option would be to generate a benchmark.h that contains #define BENCHMARK_VERSION "v1.5.2-7c7e3a75")

This PR only changes behavior when .git is not available.

Instead of current behavior

-- git Version: v0.0.0 -- Version: 0.0.0 # <- gets recorded when installed

It will use the version from the project command (this would happen both with the 1.5.2 release, and a later master.zip.)

-- git Version: v0.0.0 -- Version: 1.5.2 # <- gets recorded when installed

I don't think it's unreasonable to do:

last commit before release is to put VERSION 1.5.3 into the project() command

tag the release

next commit changes it to VERSION 1.5.3-dev

Though the way things currently are, if you install from the master branch then, you'll get a recorded 1.5.3 version if it was installed from a repo with .git, and you'll get a recorded 1.5.3-dev if installed from master.zip, which doesn't really seem desirable.

i like the idea of having the version be set as part of the release and then updated to something between the release and the next release (1.5.3.1 or 1.5.3-dev). if you get the master.zip and install it then you'd need to use that tag to match it, which i think is fair. you're explicitly looking for the dev (non official release) version.

wdyt, @LebedevRI ?

germasch · 2020-10-02T04:06:30Z

Since this still says "change requested", I'd be happy to change it in whatever way you guys prefer.

To summarize the problem once again: Usually, benchmark obtains its version from git describe. This version is printed during the configure step, and recorded when doing make install in the accompanying cmake config file.

This is all good except for actual releases, which are tarballs that don't have attached git information. Right now this means if you install from a tarball, you'll end up with version 0.0.0, which means that find_package(benchmark 1.5.2) doesn't work downstream. That's true for conda install benchmark right now, and probably most package managers unless the package manager fetches benchmark from git rather than using a tarball.

Since the tarball doesn't have git info, the release has to be recorded into it somehow. The approach here is to set it in the project command. It will only be used if there's no associated git repo, otherwise the version info from git will be used. The only impact is for a human looking at CMakeLists.txt in master seeing 1.5.2, which might be confusing, but otherwise that number is entirely ignored in master, since by definition master is a branch in a git repo.

germasch · 2021-04-26T14:44:46Z

Since #1118 got merged by @dominichamon, it reignited some hope that the version problem addressed here could also be fixed ;)

I don't want to rehash the whole discussion here, but a problem definitely exists, and I'd be happy to patch it in whatever way you guys prefer.

dmah42 · 2021-04-26T15:58:17Z

I'm warming up to this PR.

However, I just patched it locally and the output has Version set to v1.5.3 (thanks for updating ;)) and the "git version" set to v1.5.3-8fe961d8. There's nothing telling me that this version is actually not v1.5.3 after all.

I wonder if we should include the commit hash of the tagged version either in the Version or in the tag description. The former may not be possible (i think the patch and tweak parts have to be integer?) but the latter would then just be part of the releasing instructions and would be available through "git describe v1.5.3" for example.

germasch · 2021-04-26T16:14:04Z

So first here's a not directly related issue: For me, at least, on current master I get

$ git describe 
v1.5.2-38-gd882be1

That is, it doesn't pick up 1.5.3 at all. I believe the reason is that v1.5.3 is not an annotated tag, and I think that may have happened with the last release, too. One solution to that is to use an annotated tag, the other is to use

$ git describe --tags
v1.5.3-3-gd882be1

I'm not entirely firm with cmake's version handling, but yes, I believe if one doesn't use integers for all components (major.minor.patch.tweak), things like version comparisons are likely to break.

So how about this? For a benchmark version installed from a git tree, we could generate the version automatically, e.g., 1.5.3.3 where the tweak level is 3 commits after the 1.5.3 release. For a benchmark version installed from a tarball, it'd use whatever is hardcoded in the CMakeLists.txt, e.g., 1.5.3.

That still means that as part of the release process, CMakeLists.txt has to be updated -- but it's unavoidable that the tarball has to contain the version information somewhere, since it's not available through git at that point.

dmah42 · 2021-04-26T16:28:53Z

So first here's a not directly related issue: For me, at least, on current master I get
$ git describe 
v1.5.2-38-gd882be1
That is, it doesn't pick up 1.5.3 at all. I believe the reason is that v1.5.3 is not an annotated tag, and I think that may have happened with the last release, too.

can you git pull --tags? i annotated whenever i release.

One solution to that is to use an annotated tag, the other is to use
$ git describe --tags
v1.5.3-3-gd882be1
I'm not entirely firm with cmake's version handling, but yes, I believe if one doesn't use integers for all components (major.minor.patch.tweak), things like version comparisons are likely to break.

fair enough.

So how about this? For a benchmark version installed from a git tree, we could generate the version automatically, e.g., 1.5.3.3 where the tweak level is 3 commits after the 1.5.3 release. For a benchmark version installed from a tarball, it'd use whatever is hardcoded in the CMakeLists.txt, e.g., 1.5.3.

That still means that as part of the release process, CMakeLists.txt has to be updated -- but it's unavoidable that the tarball has to contain the version information somewhere, since it's not available through git at that point.

I'm ok with updating the CMakeLists.txt as part of the release (hopefully i remember!). Where i'm uncomfortable is understanding exactly what that version maps to. What we're doing if we don't include something in Version based on the commit (even if it's .0 if HEAD, .1 if not) is "moving the tag", or at least muddying the debugging and triage waters.

If downloaded as a tarball release, there will be no info from git to determine the release, so it ends up v0.0.0. If that's the case, we'll now use the release specified in the project() command, which needs to be updated for each new release.

germasch · 2021-04-26T19:37:16Z

That is, it doesn't pick up 1.5.3 at all. I believe the reason is that v1.5.3 is not an annotated tag, and I think that may have happened with the last release, too.

can you git pull --tags? i annotated whenever i release.

Well, I tried, but according to git show, v1.5.3 is still a lightweight tag, whereas v1.5.2 is an annotated tag. I made a new clone, and the same is true. No idea why...

I'm ok with updating the CMakeLists.txt as part of the release (hopefully i remember!). Where i'm uncomfortable is understanding exactly what that version maps to. What we're doing if we don't include something in Version based on the commit (even if it's .0 if HEAD, .1 if not) is "moving the tag", or at least muddying the debugging and triage waters.

So I implemented things now in a way that this happens when working off of a git repo: The actual tagged version (ie., presumably a release) generates, e.g., version "1.5.4". The next commit after that will be considered version "1.5.4.1", etc.

When git is not available (tarball), it'll use whatever is in CMakeLists.txt, presumably "1.5.4" as well. So I think as part of the release process, the very last thing would be a commit that updates the project version in CMakeLists.txt. and then tagging that commit and generating the tarball from it.

That being said, lots of stuff broke in the CI, I gotta take a look what happened.

dmah42 · 2021-04-27T08:11:22Z

That is, it doesn't pick up 1.5.3 at all. I believe the reason is that v1.5.3 is not an annotated tag, and I think that may have happened with the last release, too.

can you git pull --tags? i annotated whenever i release.

Well, I tried, but according to git show, v1.5.3 is still a lightweight tag, whereas v1.5.2 is an annotated tag. I made a new clone, and the same is true. No idea why...

$ git push origin --tags
To github.com:google/benchmark.git
 ! [rejected]        v1.5.3 -> v1.5.3 (already exists)
error: failed to push some refs to 'github.com:google/benchmark.git'
hint: Updates were rejected because the tag already exists in the remote.

/shrug

I'm ok with updating the CMakeLists.txt as part of the release (hopefully i remember!). Where i'm uncomfortable is understanding exactly what that version maps to. What we're doing if we don't include something in Version based on the commit (even if it's .0 if HEAD, .1 if not) is "moving the tag", or at least muddying the debugging and triage waters.

So I implemented things now in a way that this happens when working off of a git repo: The actual tagged version (ie., presumably a release) generates, e.g., version "1.5.4". The next commit after that will be considered version "1.5.4.1", etc.

this makes sense as per git describe which includes the last version + # commits + hash. for local use, this will help give an indication of how far a given run is from main HEAD (which will be some version + some commits).

When git is not available (tarball), it'll use whatever is in CMakeLists.txt, presumably "1.5.4" as well. So I think as part of the release process, the very last thing would be a commit that updates the project version in CMakeLists.txt. and then tagging that commit and generating the tarball from it.

as long as this is in docs/releasing.md then i should remember to do it :)

That being said, lots of stuff broke in the CI, I gotta take a look what happened.

dmah42 · 2021-04-27T08:13:40Z

-- git version: v1.5.3-8-gce2e3c9d normalized to 1.5.3.8
-- Version: 1.5.3.8

:D

dmah42 · 2021-04-27T08:14:25Z

i'll wait for @LebedevRI to approve too.

germasch · 2021-04-27T13:52:01Z

-- git version: v1.5.3-8-gce2e3c9d normalized to 1.5.3.8
-- Version: 1.5.3.8

:D

Well, it's great that it works for you as intended, but I'm still having the tag issue:

-- git version: v1.5.2-40-g9f935925 normalized to 1.5.2.40
-- Version: 1.5.2.40

You might want to try from a fresh clone, as I suspect you'll hit the same issue. I think my best suggestion is to add --tags to git describe, so it'll not care about whether the tag is annotated or not, that'll work around whatever is going on with that tag.

That way, lightweight tags will also be taken into account, which should never hurt, but it'll help in cases where, for some mysterious reason or other, annotated tags don't make it into a clone.

germasch · 2021-04-29T17:08:10Z

I went ahead and added --tags. It fixes the remaining issue (of the latest tag not being annotated in my clone), and I can't think of a case where it would hurt.

dmah42 · 2021-05-06T09:08:17Z

I went ahead and added --tags. It fixes the remaining issue (of the latest tag not being annotated in my clone), and I can't think of a case where it would hurt.

I did a fresh clone and all my tags are annotated. anyway, i agree it shouldn't matter.

dmah42 · 2021-05-06T09:08:52Z

@LebedevRI just waiting for your approval to get this in.

dmah42 · 2021-05-20T16:27:47Z

while i'm waiting for @LebedevRI, can you also update docs/releasing.md to include the instructions to update the version in CMakeLists?

dmah42 · 2021-05-20T16:37:51Z

while i'm waiting for @LebedevRI,

from irc: "17:29:23 < LebedevRI> no change of opinion since original, just ignore me" which i'll choose to take as approval.

@germasch please let me know if you can make those doc changes, and then i'll get this merged.

germasch · 2021-05-20T16:52:18Z

@germasch please let me know if you can make those doc changes, and then i'll get this merged.

Sorry, I had missed this part, now done.

dmah42 · 2021-05-20T16:59:34Z

thank you!

* cmake: fix handling the case where `git describe` fails * cmake: fix version recorded in releases If downloaded as a tarball release, there will be no info from git to determine the release, so it ends up v0.0.0. If that's the case, we'll now use the release specified in the project() command, which needs to be updated for each new release. * cmake: add `--tags` to `git describe` That way, lightweight tags will also be taken into account, which should never hurt, but it'll help in cases where, for some mysterious reason or other, annotated tags don't make it into a clone. * update releasing.md

google-cla bot added the cla: no label Sep 24, 2020

google-cla bot added cla: yes and removed cla: no labels Sep 24, 2020

LebedevRI requested changes Sep 24, 2020

View reviewed changes

germasch mentioned this pull request Sep 28, 2020

[REVIEW] cmake: prefer locally installed thirdparty packages rapidsai/rmm#572

Merged

germasch mentioned this pull request Oct 20, 2020

[BUG] Redundant dependency acquisition rapidsai/rmm#393

Closed

google deleted a comment from akagamy55 Feb 18, 2021

germasch force-pushed the pr/fix-release-version branch from 7c7e3a7 to 8fe961d Compare April 26, 2021 14:41

cmake: fix handling the case where git describe fails

4e0e6e7

germasch force-pushed the pr/fix-release-version branch from 8fe961d to fc78581 Compare April 26, 2021 18:25

cmake: fix version recorded in releases

9f93592

If downloaded as a tarball release, there will be no info from git to determine the release, so it ends up v0.0.0. If that's the case, we'll now use the release specified in the project() command, which needs to be updated for each new release.

germasch force-pushed the pr/fix-release-version branch from fc78581 to 9f93592 Compare April 26, 2021 19:37

dmah42 approved these changes Apr 27, 2021

View reviewed changes

cmake: add --tags to git describe

060a363

That way, lightweight tags will also be taken into account, which should never hurt, but it'll help in cases where, for some mysterious reason or other, annotated tags don't make it into a clone.

dmah42 added the next-release PRs or Issues that should be included in the next release label May 20, 2021

update releasing.md

3c604f1

dmah42 merged commit a4bcd93 into google:master May 20, 2021

dalg24 mentioned this pull request Dec 27, 2022

Require benchmark 1.5.4, update nightly containers to Ubuntu 22.04 arborx/ArborX#799

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix version recorded in releases #1047

fix version recorded in releases #1047

germasch commented Sep 24, 2020

google-cla bot commented Sep 24, 2020

germasch commented Sep 24, 2020

LebedevRI Sep 24, 2020

germasch Sep 24, 2020

akagamy55 Feb 18, 2021

akagamy55 Feb 18, 2021

LebedevRI Sep 24, 2020

germasch Sep 24, 2020

LebedevRI Sep 25, 2020

dmah42 Sep 25, 2020

germasch Sep 25, 2020

dmah42 Oct 12, 2020

LebedevRI Oct 12, 2020 •

edited

Loading

dmah42 Oct 12, 2020

germasch Oct 12, 2020

dmah42 Oct 20, 2020 •

edited

Loading

germasch commented Oct 2, 2020

germasch commented Apr 26, 2021

dmah42 commented Apr 26, 2021

germasch commented Apr 26, 2021

dmah42 commented Apr 26, 2021

germasch commented Apr 26, 2021

dmah42 commented Apr 27, 2021

dmah42 commented Apr 27, 2021

dmah42 commented Apr 27, 2021

germasch commented Apr 27, 2021

germasch commented Apr 29, 2021

dmah42 commented May 6, 2021

dmah42 commented May 6, 2021

dmah42 commented May 20, 2021

dmah42 commented May 20, 2021

germasch commented May 20, 2021

dmah42 commented May 20, 2021

fix version recorded in releases #1047

fix version recorded in releases #1047

Conversation

germasch commented Sep 24, 2020

google-cla bot commented Sep 24, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

germasch commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LebedevRI Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmah42 Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

germasch commented Oct 2, 2020

germasch commented Apr 26, 2021

dmah42 commented Apr 26, 2021

germasch commented Apr 26, 2021

dmah42 commented Apr 26, 2021

germasch commented Apr 26, 2021

dmah42 commented Apr 27, 2021

dmah42 commented Apr 27, 2021

dmah42 commented Apr 27, 2021

germasch commented Apr 27, 2021

germasch commented Apr 29, 2021

dmah42 commented May 6, 2021

dmah42 commented May 6, 2021

dmah42 commented May 20, 2021

dmah42 commented May 20, 2021

germasch commented May 20, 2021

dmah42 commented May 20, 2021

LebedevRI Oct 12, 2020 •

edited

Loading

dmah42 Oct 20, 2020 •

edited

Loading