Pinocchio: Checking version

[gabrielebndn]

In src/utils/version.hpp, we have the methods printVersion(delimiter) and checkVersionAtLeast(major_version, minor_version, patch_version).

However, in cmake/config.hh.cmake, we already have the cmake-computed macro PINOCCHIO_VERSION. The only differences are

1. The automatically-computed string can include commit info (which is better, I think)
2. The function allows to choose the delimiter (not a very useful feature, I would say).

Also, in src/macros.hpp, we have PINOCCHIO_VERSION_AT_LEAST(x,y,z), which does exactly the same thing as checkVersionAtLeast.

So, the methods in src/utils/version.hpp are essentially duplicates.

I have two suggestions:
1) Deprecate the above methods and recommend the macros instead. Indeed, the whole header src/utils/version.hpp could be deprecated.
2) Centralizing PINOCCHIO_VERSION_AT_LEAST to the cmake submodule, putting it inside cmake/config.hh.cmake. This is the object of https://github.com/jrl-umi3218/jrl-cmakemodules/pull/276

This would save lots of boilerplate, especially the second suggestion, which would automatically provide all of our packages with a uniform interface. What do you think?

[jcarpent]

The macro PINOCCHIO_VERSION_AT_LEAST is only for compilation purposes, to enable some features, and so on, according to the version of Pinocchio installed on the computer.

The checkVersionAtLeast is for runtime, like in Python if needed.

I'm not against putting PINOCCHIO_VERSION_AT_LEAST inside the jrl-cmake.

[gabrielebndn]

In C++, I would say checkVersionAtLeast is completely redundant. It could be turned into a simple wrapper around the macro. There is no need to have it. Compare Eigen, for instance, they only have the macro. In Python, there will be the need of a wrapper. So in the end it's only a matter of where the wrapper is put: in the core C++ or in the bindings only. I favor the second solution, because I think it is better not to provide duplicates in general.

As for printVersion, I really see no need of it if the macro PINOCCHIO_VERSION is there. In Python, this same macro can just be copied to __version__. This is where Python users would expect this information to be.

[jcarpent]

In my case, I'm using checkVersionAtLeast in some of my codes in Python.
I do not think we should discuss to suppress such basic functions now. They are not offending anyone and I have currently some usage.

[gabrielebndn]

It just that from https://github.com/humanoid-path-planner/hpp-fcl/pull/93 we had the idea of centralizing version-related commands, but as I looked into them I noticed that the version string is already centralized and that the version check is better centralized as a macro, so they are redundant and we might as well remove them.

I fully understand the need of checkVersionAtLeast in Python, I just do not understand why it should also be available as a C++ function. But as you wish, not a big deal. Then we might just simplify it by using the macro.

printVersion, I would remove it, but again, as you wish.

[nim65s]

checkVersionAtLeast in python would be just a pinocchio.__version__ >= (1, 2, 3) if version was a tuple with Major, Minor, Patch numbers.

[gabrielebndn]

checkVersionAtLeast in python would be just a pinocchio.__version__ >= (1, 2, 3) if version was a tuple with Major, Minor, Patch numbers.

That is so cool, didn't know that!

[cmastalli]

The macro PINOCCHIO_VERSION_AT_LEAST is only for compilation purposes, to enable some features, and so on, according to the version of Pinocchio installed on the computer.

The checkVersionAtLeast is for runtime, like in Python if needed.

I'm not against putting PINOCCHIO_VERSION_AT_LEAST inside the jrl-cmake.

We shouldn't add package related code in jrl-cmake. It is a bad idea, imagine that every package does it.

[cmastalli]

You can always expose the Pinocchio macros in Python, despite that I don't think it's a good approach. The Pythonic way of doing things is as @nim65s suggested us (through __version__).

[gabrielebndn]

The macro PINOCCHIO_VERSION_AT_LEAST is only for compilation purposes, to enable some features, and so on, according to the version of Pinocchio installed on the computer.
The checkVersionAtLeast is for runtime, like in Python if needed.
I'm not against putting PINOCCHIO_VERSION_AT_LEAST inside the jrl-cmake.

We shouldn't add package related code in jrl-cmake. It is a bad idea, imagine that every package does it.

@cmastalli please check https://github.com/jrl-umi3218/jrl-cmakemodules/pull/276 to see what we mean by "centralizing PINOCCHIO_VERSION_AT_LEAST". We are actually talking about a macro ${PACKAGE}_VERSION_AT_LEAST to be put in the cmake-generated config file, where ${PACKAGE} depends on the specific package and is computed by cmake, and becomes PINOCCHIO in the case of Pinocchio. That is how we are already handling ${PACKAGE}_VERSION, ${PACKAGE}_MAJOR_VERSION, ${PACKAGE}_MINOR_VERSION and ${PACKAGE}_PATCH_VERSION.

[cmastalli]

The macro PINOCCHIO_VERSION_AT_LEAST is only for compilation purposes, to enable some features, and so on, according to the version of Pinocchio installed on the computer.
The checkVersionAtLeast is for runtime, like in Python if needed.
I'm not against putting PINOCCHIO_VERSION_AT_LEAST inside the jrl-cmake.

We shouldn't add package related code in jrl-cmake. It is a bad idea, imagine that every package does it.

@cmastalli please check jrl-umi3218/jrl-cmakemodules#276 to see what we mean by "centralizing PINOCCHIO_VERSION_AT_LEAST". We are actually talking about a macro ${PACKAGE}_VERSION_AT_LEAST to be put in the cmake-generated config file, where ${PACKAGE} depends on the specific package and is computed by cmake, and becomes PINOCCHIO in the case of Pinocchio. That is how we are already handling ${PACKAGE}_VERSION, ${PACKAGE}_MAJOR_VERSION, ${PACKAGE}_MINOR_VERSION and ${PACKAGE}_PATCH_VERSION.

Ok, this is reasonable; sorry for the noise. However I still support the __version__. It is what I expect as an Python user

[gabrielebndn]

The macro PINOCCHIO_VERSION_AT_LEAST is only for compilation purposes, to enable some features, and so on, according to the version of Pinocchio installed on the computer.
The checkVersionAtLeast is for runtime, like in Python if needed.
I'm not against putting PINOCCHIO_VERSION_AT_LEAST inside the jrl-cmake.

We shouldn't add package related code in jrl-cmake. It is a bad idea, imagine that every package does it.

@cmastalli please check jrl-umi3218/jrl-cmakemodules#276 to see what we mean by "centralizing PINOCCHIO_VERSION_AT_LEAST". We are actually talking about a macro ${PACKAGE}_VERSION_AT_LEAST to be put in the cmake-generated config file, where ${PACKAGE} depends on the specific package and is computed by cmake, and becomes PINOCCHIO in the case of Pinocchio. That is how we are already handling ${PACKAGE}_VERSION, ${PACKAGE}_MAJOR_VERSION, ${PACKAGE}_MINOR_VERSION and ${PACKAGE}_PATCH_VERSION.

Ok, this is reasonable; sorry for the noise. However I still support the __version__. It is what I expect as an Python user

I do agree on that. We already have it in Pinocchio. The point is wether to remove other options or not. Following @nim65s's suggestion, I think checkVersionAtLeast is also redundant in Python.

[gabrielebndn]

checkVersionAtLeast in python would be just a pinocchio.__version__ >= (1, 2, 3) if version was a tuple with Major, Minor, Patch numbers.

@nim65s :your suggestion is cool, but __version__ is a string. As such, it needs parsing and/or third-party modules to checked the way we do it with checkVersionAtLeast (see https://stackoverflow.com/questions/11887762/how-do-i-compare-version-numbers-in-python) So, I think a wrapper is still useful.

[nim65s]

I think a __version_tuple__ = tuple(int(n) for n in __version__.split('.')) should be enough as long as we only une integers in our version numbers. Or we could be more fancy and use a namedtuple.

[jcarpent]

It seems that checkVersionAtLeast is much more simplet ;)

[gabrielebndn]

It seems that checkVersionAtLeast is much more simplet ;)

I do agree.__version_tuple__ is completely nonstandard. We can expose the version numbers one by one (we are doing it in Pinocchio) and then use the tuple comparison, but still (PINOCCHIO_MAJOR_VERSION,PINOCCHIO_MINOR_VERSION,PINOCCHIO_PATCH_VERSION) >= (2,1,3) is long, tedious, and error-prone to write. A checkVersionAtLeast method is simple and intuitive. But, I still think, only in Python ;)

[nim65s]

if checkVersionAtLeast is just a bindings to https://github.com/jrl-umi3218/jrl-cmakemodules/pull/276, why not.