[cmake-developers] [PATCH] FindProtobuf: check version

[Ben Boeckel]

On Tue, Feb 02, 2016 at 09:29:10 +0100, Antonio Perez Barrero wrote:
> From: Antonio Perez Barrero <apbarrero at gmail.com>
> 
> Check found libraries version to match user required version, including
> EXACT match.
> 
> Protobuf compiler executable version is check to be aligned with found
> libraries, raising a warning message otherwise.
> 
> Module interface and private variables are renamed to honour
> recommendation to be aligned in case with module name.

Thanks, comments inline.

> diff --git a/Modules/FindProtobuf.cmake b/Modules/FindProtobuf.cmake
> index 2f13b09..34cd660 100644
> --- a/Modules/FindProtobuf.cmake
> +++ b/Modules/FindProtobuf.cmake
> @@ -6,47 +6,49 @@
>  #
>  # The following variables can be set and are optional:
>  #
> -# ``PROTOBUF_SRC_ROOT_FOLDER``
> +# ``Protobuf_SRC_ROOT_FOLDER``

Could this rename be split out into a separate patch? Backwards
compatibility still needs to be provided though, so the uppercase
versions will still need to exist.

> -function(PROTOBUF_GENERATE_CPP SRCS HDRS)
> +function(Protobuf_GENERATE_CPP SRCS HDRS)

Function case doesn't matter, so no need to duplicate it.

> +  if("${_Protobuf_COMMON_H_CONTENTS}" MATCHES "#define GOOGLE_PROTOBUF_VERSION ${_Protobuf_VERSION_REGEX}")

No need to dereference the content variable. Should be:

+  if(_Protobuf_COMMON_H_CONTENTS MATCHES "#define GOOGLE_PROTOBUF_VERSION ${_Protobuf_VERSION_REGEX}")

> +  math(EXPR Protobuf_MAJOR_VERSION "${Protobuf_VERSION} / 1000000")
> +  math(EXPR Protobuf_MINOR_VERSION "${Protobuf_VERSION} / 1000 % 1000")
> +  math(EXPR Protobuf_SUBMINOR_VERSION "${Protobuf_VERSION} % 1000")

Would string manipulation be better here?

> +  if(Protobuf_FIND_VERSION)
> +    # Set Protobuf_FOUND based on requested version.
> +    set(_Protobuf_VERSION "${Protobuf_MAJOR_VERSION}.${Protobuf_MINOR_VERSION}.${Protobuf_SUBMINOR_VERSION}")

The find_package_handle_standard_args function has a VERSION_VAR
argument which can do this logic.

Thanks,

--Ben