Implement new PCL versioning scheme #1905

taketwo · 2017-06-26T21:48:30Z

(as discussed in #1682)

Current master (trunk) version is indicated by a "-dev" suffix appended to the latest released version. For instance, right now the latest released version is 1.8.0, therefore the master branch of PCL should build into version 1.8.0-dev.

In order to conditionally compile based on PCL version the PCL_VERSION_COMPARE macro should be used. It takes into account the development/release status of the build. A development version is always greater than its precedent released version and less than any of the future releases. For instance, 1.8.0-dev > 1.8.0 is true, as is 1.8.0-dev < 1.8.1.

No change is done to the existing C++ macros PCL_VERSION and PCL_VERSION_PRETTY. The first one is a numeric representation of the version (e.g. 100800 for version 1.8.0) and does not take into account development/release status to avoid breaking existing codebases. The second one is a string representation with "-dev" suffix for development versions (e.g. "1.8.0-dev" right now).

With the new versioning scheme the release process should involve the following steps:

Update the PCL_VERSION variable in the root "CMakeLists.txt" file:
1.1. Bump major/minor/revision number as appropriate;
1.2. Remove "-dev" suffix.
Tag release.
Append "-dev" suffix to the PCL_VERSION variable.

SergioRAgostinho

This looks pretty good. Can you just confirme me what exactly is being generated here https://github.com/PointCloudLibrary/pcl/blob/master/PCLConfigVersion.cmake.in#L3

taketwo · 2017-06-27T08:39:46Z

Good catch. I've double checked all occurrences of PCL_VERSION in scripts and updated them to use suffix-less version where appropriate (took OpenCV as an example).

SergioRAgostinho · 2017-06-28T15:26:36Z

I was a looking at this PR #1614 and realized that with this versioning scheme I don't know how to properly handle situations like this https://github.com/aliosciapetrelli/pcl/blob/84e45b5e5c79de3958face842bada15172f3f31e/doc/tutorials/content/sources/ReLOC/CMakeLists.txt#L5

At some this will be merged into 1.8.1-dev but you really can't compile the example (after it's merged) because the minimum version is 1.9. Is there a good way to solve this?

taketwo · 2017-06-28T16:06:18Z

The problem is that find_package does not allow us to express "any version greater than", only "any version greater or equal than". But we could use the tweak number to disambiguate the latest released version and the current development version. In other words, 1.8.1-dev will be a synonym of 1.8.1.1, and this is something find_package understands. So for ReLOC you would say find_package(PCL 1.8.1.1 REQUIRED)

Another option that I can exercise is to update the code responsible for version checking in CMake to support constructs like find_package(PCL 1.8.1-dev REQUIRED).

But either way, we end up with strange version numbers that won't exists in the changelog of the library. Though grepping and replacing them with the actual release version can be a part of the releasing protocol.

Off the top of my head I do not see any other clean way around it.

SergioRAgostinho · 2017-06-28T16:12:38Z

Another option that I can exercise is to update the code responsible for version checking in CMake to support constructs like find_package(PCL 1.8.1-dev REQUIRED)

I prefer this. It's more consistent with the nomenclature were trying to adopt.

Though grepping and replacing them with the actual release version can be a part of the releasing protocol

I don't see any other way around it

taketwo · 2017-06-28T16:14:33Z

OK, I'll check if this is possible.

taketwo · 2017-06-28T19:58:17Z

Unfortunately the find_package command is strict about the version argument:

find_package called with invalid argument "1.8.1-dev"

Only major[.minor[.patch[.tweak]]] format is supported. So, first option? Or other ideas?

SergioRAgostinho · 2017-06-29T00:46:03Z

I assume major, minor patch and tweak need to numerals, otherwise you would have been able to parse the patch appropriately.

Then the easiest is to use the tweak as 1 when it's a dev version.

taketwo · 2017-06-29T07:45:33Z

OK, so "1.8.1-dev" will be a synonym of "1.8.1.1".

I was thinking about the C++ macro PCL_VERSION which is constructed as Major * 100000 + Minor * 100 + Revision, e.g. 100801 for "1.8.1". Although there is an "official" macro function PCL_VERSION_COMPARE to check version compatibility, doing simple #if PCL_VERSION >= 100800 is still popular in user codebases. Perhaps we can change the way PCL_VERSION is constructed to incorporate tweak number (i.e. development status) and without breaking backwards compatibility. The formula can be Major * 10000 + Minor * 1000 + Revision * 10 + Tweak, e.g. 108010 for "1.8.1" and 108011 for "1.8.1-dev". All new versions will be strictly greater than all released old versions.

taketwo · 2017-06-29T07:49:09Z

On the other hand, why bothering. Unlike CMake, in source code we can use strictly greater comparison and write #if PCL_VERSION > 100801 to disambiguate between 1.8.1 and 1.8.1-dev.

SergioRAgostinho

@jspricke Any final comment? Just to be sure we're not overlooking anything at this point.

It is set to 1 for development versions and 0 for release versions. Development versions are identified by a "-dev" prefix. This commit drops unused PCL_CANDIDATE_VERSION CMake variable.

…versions A development version is always greater than its precedent released version and less than any of the future releases.

jspricke

Thanks for looking into it. I'm generally fine, except the one comment inline.

Also, I've seen other project integration the git describe output into it, but we can think about that later.

jspricke · 2017-06-30T04:44:11Z

cmake/pcl_utils.cmake

+    set(PCL_VERSION_PLAIN "${PCL_MAJOR_VERSION}.${PCL_MINOR_VERSION}.${PCL_REVISION_VERSION}")
+    if(${PCL_VERSION} MATCHES "^[0-9]+\\.[0-9]+\\.[0-9]+-dev$")
+        set(PCL_DEV_VERSION 1)
+        set(PCL_VERSION_PLAIN "${PCL_VERSION_PLAIN}.1")


I would propose to use 90 or 99 instead of 1 to make it easier distinguishable.

prefer 99 between the two.

taketwo · 2017-06-30T07:45:34Z

Interesting, I did not know the describe command. For example, executing right now in my clone the following command:

git describe --tags --long

I get:

pcl-1.8.0-234-g16ef868
^          ^     ^
|          |     |
|          |     SHA of HEAD
|          |
|          number of commits since last tag
|
last tag

We can use the number of commits since last tag as the tweak number instead of the arbitrary "1", "90", or "99". One problem, however, is that this won't work in source trees downloaded as archive, not by cloning.

taketwo · 2017-06-30T08:21:06Z

Actually, after reading up in this topic, I do not think it is a good idea. For example, we will run into problems already after 1.8.1 release. Recall that we agreed to make a release branch, revert one of the PRs there, and eventually tag. The mainline PCL will continue with the master branch, it will never have this tag in it's history. Consequence: git describe will still report the number of commits since 1.8.0 release.

Jochen, can you give us some examples of projects that use it? (I see it in OpenCV, but only in CMake for reporting general build configuration to the user, not for tweak versioning.)

This variable is just like PCL_VERSION, but with "-dev" suffix (if any) replaced by ".99" tweak number. fu

SergioRAgostinho · 2017-06-30T10:37:32Z

Actually, after reading up in this topic, I do not think it is a good idea. For example, we will run into problems already after 1.8.1 release. Recall that we agreed to make a release branch, revert one of the PRs there, and eventually tag. The mainline PCL will continue with the master branch, it will never have this tag in it's history. Consequence: git describe will still report the number of commits since 1.8.0 release.

😞 The approach sounded really nice.

Shall I squash merge this or do we need to keep the 'new version mechanism' and the 'bumping of current version' in different commits?

taketwo · 2017-06-30T11:17:42Z

I like granular commits, but I don't care.

SergioRAgostinho · 2017-06-30T11:53:01Z

Thanks Sergey :)

jspricke · 2017-06-30T12:24:00Z

@taketwo sorry, I don't have a project at hand. Regarding the describe, I think basing it of the last tag in the branch is the right thing and we could still decode it (and for sure using the sha1), but we can reevaluate that at an other point.

SergioRAgostinho · 2017-07-02T21:27:16Z

By the way guys I just found this here

Release versions and release candidate versions of CMake use the format:

<major>.<minor>.<patch>[-rc<n>]
where the component is less than 20000000. Development versions of CMake use the format:

<major>.<minor>.<date>[-<id>]
where the component is of format CCYYMMDD and may contain arbitrary text. This represents development as of a particular date following the . feature release.

Also

Note CMake versions 2.8.2 through 2.8.12 used three components for the feature level. Release versions represented the bug-fix level in a fourth component, i.e. <major>.<minor>.<patch>[.<tweak>][-rc<n>]. Development versions represented the development date in the fourth component, i.e. <major>.<minor>.<patch>.<date>[-<id>].

CMake versions prior to 2.8.2 used three components for the feature level and had no bug-fix component. Release versions used an even-valued second component, i.e. <major>.<even-minor>.<patch>[-rc<n>]. Development versions used an odd-valued second component with the development date as the third component, i.e. <major>.<odd-minor>.<date>.

The CMAKE_VERSION variable is defined by CMake 2.6.3 and higher. Earlier versions defined only the individual component variables.

taketwo · 2017-07-03T06:22:35Z

Well, this concerns only versioning of CMake itself. As a wrote before, find_package does not accept suffixed versions.

SergioRAgostinho · 2017-07-03T10:57:11Z

I just revised it to be sure. I though that perhaps versions prior to CMake 3 didn't accept it.

Really mean of Kitware to be able to use a very convenient version number for themselves and not allow the same type of functionality for the rest :(

I like the idea of the tweak version during the development stage as a date in yyyyMMDD format though.

taketwo · 2017-07-03T11:07:31Z

I like the idea of the tweak version during the development stage as a date in yyyyMMDD format though.

Sounds attractive!

SergioRAgostinho reviewed Jun 27, 2017

View reviewed changes

SergioRAgostinho added this to the pcl-1.8.1 milestone Jun 27, 2017

SergioRAgostinho approved these changes Jun 27, 2017

View reviewed changes

taketwo requested a review from jspricke June 27, 2017 11:30

taketwo added the needs: more work Specify why not closed/merged yet label Jun 28, 2017

SergioRAgostinho approved these changes Jun 29, 2017

View reviewed changes

taketwo force-pushed the version-dev branch from 6269c02 to 413d01c Compare June 29, 2017 12:39

taketwo added 3 commits June 29, 2017 14:39

Introduce a new CMake and C++ macro variable "PCL_DEV_VERSION"

a75823c

It is set to 1 for development versions and 0 for release versions. Development versions are identified by a "-dev" prefix. This commit drops unused PCL_CANDIDATE_VERSION CMake variable.

Update PCL_VERSION_COMPARE macro function to account for development …

83a1088

…versions A development version is always greater than its precedent released version and less than any of the future releases.

Add a unit test for PCL_VERSION_COMPARE macro function

84474ba

taketwo force-pushed the version-dev branch from 413d01c to 6bb37ee Compare June 29, 2017 12:40

taketwo removed the needs: more work Specify why not closed/merged yet label Jun 29, 2017

jspricke approved these changes Jun 30, 2017

View reviewed changes

taketwo added 2 commits June 30, 2017 10:51

Add PCL_VERSION_PLAIN and use where appropriate

48729a8

This variable is just like PCL_VERSION, but with "-dev" suffix (if any) replaced by ".99" tweak number. fu

Bump current version to "1.8.0-dev"

b8917e8

taketwo force-pushed the version-dev branch from 6bb37ee to b8917e8 Compare June 30, 2017 08:51

SergioRAgostinho merged commit 0381c68 into PointCloudLibrary:master Jun 30, 2017

taketwo deleted the version-dev branch June 30, 2017 11:57

SergioRAgostinho mentioned this pull request Sep 24, 2018

CMake Set temporarily the policy CMP0074 to OLD #2454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new PCL versioning scheme #1905

Implement new PCL versioning scheme #1905

taketwo commented Jun 26, 2017

SergioRAgostinho left a comment

taketwo commented Jun 27, 2017

SergioRAgostinho commented Jun 28, 2017

taketwo commented Jun 28, 2017

SergioRAgostinho commented Jun 28, 2017

taketwo commented Jun 28, 2017

taketwo commented Jun 28, 2017

SergioRAgostinho commented Jun 29, 2017

taketwo commented Jun 29, 2017

taketwo commented Jun 29, 2017

SergioRAgostinho left a comment

jspricke left a comment

jspricke Jun 30, 2017

SergioRAgostinho Jun 30, 2017

taketwo commented Jun 30, 2017

taketwo commented Jun 30, 2017

SergioRAgostinho commented Jun 30, 2017

taketwo commented Jun 30, 2017

SergioRAgostinho commented Jun 30, 2017

jspricke commented Jun 30, 2017

SergioRAgostinho commented Jul 2, 2017

taketwo commented Jul 3, 2017

SergioRAgostinho commented Jul 3, 2017

taketwo commented Jul 3, 2017

Implement new PCL versioning scheme #1905

Implement new PCL versioning scheme #1905

Conversation

taketwo commented Jun 26, 2017

SergioRAgostinho left a comment

Choose a reason for hiding this comment

taketwo commented Jun 27, 2017

SergioRAgostinho commented Jun 28, 2017

taketwo commented Jun 28, 2017

SergioRAgostinho commented Jun 28, 2017

taketwo commented Jun 28, 2017

taketwo commented Jun 28, 2017

SergioRAgostinho commented Jun 29, 2017

taketwo commented Jun 29, 2017

taketwo commented Jun 29, 2017

SergioRAgostinho left a comment

Choose a reason for hiding this comment

jspricke left a comment

Choose a reason for hiding this comment

jspricke Jun 30, 2017

Choose a reason for hiding this comment

SergioRAgostinho Jun 30, 2017

Choose a reason for hiding this comment

taketwo commented Jun 30, 2017

taketwo commented Jun 30, 2017

SergioRAgostinho commented Jun 30, 2017

taketwo commented Jun 30, 2017

SergioRAgostinho commented Jun 30, 2017

jspricke commented Jun 30, 2017

SergioRAgostinho commented Jul 2, 2017

taketwo commented Jul 3, 2017

SergioRAgostinho commented Jul 3, 2017

taketwo commented Jul 3, 2017