Re: [cmake-developers] [PATCH] FindProtobuf: check version
On 02/12/2016 02:23 AM, Antonio Perez Barrero wrote: > Check found libraries version to match user required version. Thanks. Applied with minor tweaks and a test case: FindProtobuf: check version https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=bb7a41ab -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] FindProtobuf: check version
>
> > + math(EXPR Protobuf_MAJOR_VERSION "${Protobuf_VERSION} / 100")
> > + math(EXPR Protobuf_MINOR_VERSION "${Protobuf_VERSION} / 1000 % 1000")
> > + math(EXPR Protobuf_SUBMINOR_VERSION "${Protobuf_VERSION} % 1000")
>
> Would string manipulation be better here?
I don't know how to make it better. I took FindBoost.cmake code as template
here. Which could be a better way?
Thanks,
Antonio
--
Powered by www.kitware.com
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Kitware offers various services to support the CMake community. For more
information on each offering, please visit:
CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] FindProtobuf: check version
On Mon, Feb 08, 2016 at 15:29:50 +, Antonio Pérez Barrero wrote: > thanks for feedback, I'll rework the patch. I was planning to actually > submit three patches covering the following: > > * Version checking > * Interface variables rename, keeping backwards compatibility. > * Fix a bug when setting Protobuf_LIBRARIES on Un*x systems. Looks like a sensible breakdown to me. > So what's the best way to submit them: > > * A separate format-patch for each. This one. Sending it as a series of 3 patches is best (and easy if you use git send-email). Best is that the earlier patches fix bugs and such so that if they are OK, they're more easily applied independently if later patches have issues which need resolving yet. Thanks, --Ben -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] FindProtobuf: check version
Hi,
thanks for feedback, I'll rework the patch. I was planning to actually
submit three patches covering the following:
* Version checking
* Interface variables rename, keeping backwards compatibility.
* Fix a bug when setting Protobuf_LIBRARIES on Un*x systems.
So what's the best way to submit them:
* A separate format-patch for each.
* An all together patch.
* Fork github's repo, create a branch and send a link here.
Thanks,
Antonio
El mar., 2 feb. 2016 a las 16:18, Rolf Eike Beer ()
escribió:
> > +
> > + set(Protobuf_VERSION 0)
>
> Just set it to an empty string.
>
> > + set(Protobuf_LIB_VERSION "")
> > + file(STRINGS ${_Protobuf_COMMON_HEADER} _Protobuf_COMMON_H_CONTENTS
> > REGEX "#define GOOGLE_PROTOBUF_VERSION ")
>
> I would replace the spaces with "[ \t]+" so a formatting change will not
> break the extraction, also below.
>
> > + set(_Protobuf_VERSION_REGEX "([0-9]+)")
>
> No need for that variable, just put it into the matches argument below.
>
> > + if("${_Protobuf_COMMON_H_CONTENTS}" MATCHES "#define
> > GOOGLE_PROTOBUF_VERSION ${_Protobuf_VERSION_REGEX}")
> > +set(Protobuf_VERSION "${CMAKE_MATCH_1}")
> > + endif()
> > + unset(_Protobuf_COMMON_H_CONTENTS)
> > +
> > + math(EXPR Protobuf_MAJOR_VERSION "${Protobuf_VERSION} / 100")
> > + math(EXPR Protobuf_MINOR_VERSION "${Protobuf_VERSION} / 1000 %
> > 1000")
> > + math(EXPR Protobuf_SUBMINOR_VERSION "${Protobuf_VERSION} % 1000")
>
> You are not required to export these variables, only if you expect it to
> be useful for something beyond version checking. If anyone needs
> specific version fiddling Protobuf_VERSION can be used.
>
> Eike
>
--
Powered by www.kitware.com
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Kitware offers various services to support the CMake community. For more
information on each offering, please visit:
CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] FindProtobuf: check version
+
+ set(Protobuf_VERSION 0)
Just set it to an empty string.
+ set(Protobuf_LIB_VERSION "")
+ file(STRINGS ${_Protobuf_COMMON_HEADER} _Protobuf_COMMON_H_CONTENTS
REGEX "#define GOOGLE_PROTOBUF_VERSION ")
I would replace the spaces with "[ \t]+" so a formatting change will not
break the extraction, also below.
+ set(_Protobuf_VERSION_REGEX "([0-9]+)")
No need for that variable, just put it into the matches argument below.
+ if("${_Protobuf_COMMON_H_CONTENTS}" MATCHES "#define
GOOGLE_PROTOBUF_VERSION ${_Protobuf_VERSION_REGEX}")
+set(Protobuf_VERSION "${CMAKE_MATCH_1}")
+ endif()
+ unset(_Protobuf_COMMON_H_CONTENTS)
+
+ math(EXPR Protobuf_MAJOR_VERSION "${Protobuf_VERSION} / 100")
+ math(EXPR Protobuf_MINOR_VERSION "${Protobuf_VERSION} / 1000 %
1000")
+ math(EXPR Protobuf_SUBMINOR_VERSION "${Protobuf_VERSION} % 1000")
You are not required to export these variables, only if you expect it to
be useful for something beyond version checking. If anyone needs
specific version fiddling Protobuf_VERSION can be used.
Eike
--
Powered by www.kitware.com
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Kitware offers various services to support the CMake community. For more
information on each offering, please visit:
CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] [PATCH] FindProtobuf: check version
On Tue, Feb 02, 2016 at 09:29:10 +0100, Antonio Perez Barrero wrote:
> From: Antonio Perez Barrero
>
> 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} / 100")
> + 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
--
Powered by www.kitware.com
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Kitware offers various services to support the CMake community. For more
information on each offering, please visit:
CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers
