Re: [cmake-developers] [PATCH] FindProtobuf: fix wrong library list for Unix

[Antonio Pérez Barrero]

El mar., 9 feb. 2016 a las 11:13, Rolf Eike Beer (<e...@sf-mail.de>)
escribió:

> Am 09.02.2016 11:10, schrieb Antonio Pérez Barrero:
> >>
> >> On 02/08/2016 04:24 PM, Antonio Pérez Barrero wrote:
> >> > Yes, it is possible, but currently it's not looking for the debug
> >> > library with a different name, just in a different path that is
> >> > unlikely to exist in a Unix system.
> >>
> >> The fact that a separate find_library is called for it and the result
> >> is stored in a separate (user-settable) cache entry means that it is
> >> possible to get two different values.  This convention is well
> >> established
> >> in several find modules.
> >>
> >> > Having the PROTOBUF_LIBRARIES variable getting the afore mentioned
> >> > value messes up the client code using `find_package(Protobuf)` on
> Unix.
> >>
> >> The only supported use for PROTOBUF_LIBRARIES is to pass it to
> >> target_link_libraries, and that interprets the 'optimized' and 'debug'
> >> keywords.  This is shown in the module documentation example:
> >>
> >> target_link_libraries(bar ${PROTOBUF_LIBRARIES})
> >>
> >> The documentation makes no other guarantees about the variable value.
> >>
> >
> > Thanks for your explanantion, that makes sense. I had issues because I
> > was
> > appending the value of PROTOBUF_LIBRARIES to a list and then removing
> > duplicates before calling `target_link_libraries(bar ${LIST})`. So when
> > the
> > PROTOBUF_LIBRARIES values was
> > `optimized;/usr/lib/libprotobuf.so;debug;/usr/lib/libprotobuf.so` I was
> > missing the link flag for /usr/lib/libprotobuf.so in Debug builds.
> > Skipping
> > the duplicates removal step solves my issue.
> >
> > Anyway, the root cause is the `find_library` is finding the same
> > library
> > for optimized and debug configuration. So, would a patch like this be
> > benefial?
> >
> > diff --git a/Modules/FindProtobuf.cmake b/Modules/FindProtobuf.cmake
> > index 2f13b09..35929a4 100644
> > --- a/Modules/FindProtobuf.cmake
> > +++ b/Modules/FindProtobuf.cmake
> > @@ -224,7 +224,7 @@ function(_protobuf_find_libraries name filename)
> > PATHS
> > ${PROTOBUF_SRC_ROOT_FOLDER}/vsprojects/${_PROTOBUF_ARCH_DIR}Debug)
> > mark_as_advanced(${name}_LIBRARY_DEBUG)
> >
> > -   if(NOT ${name}_LIBRARY_DEBUG)
> > +   if((NOT ${name}_LIBRARY_DEBUG) OR (${name}_LIBRARY STREQUAL
> > ${name}_LIBRARY_DEBUG))
> ># There is no debug library
> >set(${name}_LIBRARY_DEBUG ${${name}_LIBRARY} PARENT_SCOPE)
> >set(${name}_LIBRARIES ${${name}_LIBRARY} PARENT_SCOPE)
>
> Just use SelectLibraryConfigurations, that does the same automatically
> if they are identical, and also handles the cases where only debug, but
> not release is found.
>

Thanks, I tried that and got it working. I'll submit another patch as the
change covers a related but different issue.

Regards,
Antonio



> Greetings,
>
> 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
-- 

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

[Antonio Pérez Barrero]

>
> > +  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: fix wrong library list for Unix

[Antonio Pérez Barrero]

>
> On 02/08/2016 04:24 PM, Antonio Pérez Barrero wrote:
> > Yes, it is possible, but currently it's not looking for the debug
> > library with a different name, just in a different path that is
> > unlikely to exist in a Unix system.
>
> The fact that a separate find_library is called for it and the result
> is stored in a separate (user-settable) cache entry means that it is
> possible to get two different values.  This convention is well established
> in several find modules.
>
> > Having the PROTOBUF_LIBRARIES variable getting the afore mentioned
> > value messes up the client code using `find_package(Protobuf)` on Unix.
>
> The only supported use for PROTOBUF_LIBRARIES is to pass it to
> target_link_libraries, and that interprets the 'optimized' and 'debug'
> keywords.  This is shown in the module documentation example:
>
> target_link_libraries(bar ${PROTOBUF_LIBRARIES})
>
> The documentation makes no other guarantees about the variable value.
>

Thanks for your explanantion, that makes sense. I had issues because I was
appending the value of PROTOBUF_LIBRARIES to a list and then removing
duplicates before calling `target_link_libraries(bar ${LIST})`. So when the
PROTOBUF_LIBRARIES values was
`optimized;/usr/lib/libprotobuf.so;debug;/usr/lib/libprotobuf.so` I was
missing the link flag for /usr/lib/libprotobuf.so in Debug builds. Skipping
the duplicates removal step solves my issue.

Anyway, the root cause is the `find_library` is finding the same library
for optimized and debug configuration. So, would a patch like this be
benefial?

diff --git a/Modules/FindProtobuf.cmake b/Modules/FindProtobuf.cmake
index 2f13b09..35929a4 100644
--- a/Modules/FindProtobuf.cmake
+++ b/Modules/FindProtobuf.cmake
@@ -224,7 +224,7 @@ function(_protobuf_find_libraries name filename)
PATHS
${PROTOBUF_SRC_ROOT_FOLDER}/vsprojects/${_PROTOBUF_ARCH_DIR}Debug)
mark_as_advanced(${name}_LIBRARY_DEBUG)

-   if(NOT ${name}_LIBRARY_DEBUG)
+   if((NOT ${name}_LIBRARY_DEBUG) OR (${name}_LIBRARY STREQUAL
${name}_LIBRARY_DEBUG))
   # There is no debug library
   set(${name}_LIBRARY_DEBUG ${${name}_LIBRARY} PARENT_SCOPE)
   set(${name}_LIBRARIES ${${name}_LIBRARY} PARENT_SCOPE)

---

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

[Antonio Pérez Barrero]

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: fix wrong library list for Unix

[Antonio Pérez Barrero]

El lun., 8 feb. 2016 a las 19:28, Brad King ()
escribió:

> On 02/08/2016 11:47 AM, Antonio Perez Barrero wrote:
> > Avoid looking for debug library unless configuring for MSVC.
> [snip]
> > -   find_library(${name}_LIBRARY_DEBUG
> > -   NAMES ${filename}
> > -   PATHS
> ${PROTOBUF_SRC_ROOT_FOLDER}/vsprojects/${_PROTOBUF_ARCH_DIR}Debug)
> > -   mark_as_advanced(${name}_LIBRARY_DEBUG)
> > +   if(MSVC)
> > +   find_library(${name}_LIBRARY_DEBUG
> > +   NAMES ${filename}
> > +   PATHS
> ${PROTOBUF_SRC_ROOT_FOLDER}/vsprojects/${_PROTOBUF_ARCH_DIR}Debug)
> > +   mark_as_advanced(${name}_LIBRARY_DEBUG)
> > +   endif()
>
> It is possible someone builds a debug-named library on unix too.
> See `select_library_configurations` and its uses in other find
> modules.  That is the conventional way to normalize libraries
> across configurations.
>
> Yes, it is possible, but currently it's not looking for the debug library
with a different name, just in a different path that is unlikely to exist
in a Unix system. Having the PROTOBUF_LIBRARIES variable getting the afore
mentioned value messes up the client code using `find_package(Protobuf)` on
Unix.

I might be wrong, but I think this patch solves one issue, while not
introducing another for debug libraries under Unix, since it wasn't working
either before applying it.

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