Review Request 112448: FindSamba add PkgConfig fallback

[Mark Gaiser]

> On Sept. 7, 2013, 8:48 p.m., Christophe Giboudeaux wrote:
> > cmake/modules/FindSamba.cmake, lines 24-30
> > <http://git.reviewboard.kde.org/r/112448/diff/1/?file=186323#file186323line24>
> >
> >     I recommend using PC_SAMBA_INCLUDEDIR and PC_SAMBA_LIBDIR as hints for the find_path / find_library calls instead
> >     
> >     eg:
> >     if(NOT WIN32)
> >       find_package(PkgConfig)
> >       if(PKG_CONFIG_FOUND)
> >         pkg_check_modules(PC_SAMBA smbclient)
> >       endif()
> >     endif()
> >     
> >     find_path(SAMBA_INCLUDE_DIR NAMES libsmbclient.h HINTS ${PC_SAMBA_INCLUDEDIR})
> >     
> >     find_library(SAMBA_LIBRARIES NAMES smbclient HINTS ${PC_SAMBA_LIBDIR})
> 
> Mark Gaiser wrote:
>     While it certainly looks cleaner, isn't it a bit strange to do it like that? I mean, it sounds so strange to let pkg-config search for samba and then fall back to the other search stuff to again find samba..
> 
> Alexander Neundorf wrote:
>     The if(NOT WIN32) is not needed. If pkgconfig is not found, pkg_check_modules() simply does nothing.
>     
>     Interesting point.
>     But I agree with Christophe. That way, i.e. use pkg-config to give hints to the actual find-calls, the result is in the same form in all cases, whether it has been found with or without pkg-config.
>     I.e. the SAMBA_INCLUDE_DIR and SAMBA_LIBRARIES are cached cmake variables and can be edited by the user.
>     When using pkg-config results directly, this is different.
> 
> Mark Gaiser wrote:
>     It makes no sense to me.. The way i made it the pkg-config route is being taken when the current search stuff fails and either SAMBA_INCLUDE_DIR or SAMBA_LIBRARIES is empty. If it is, the fallback will use pkg-config and fill those vars if it can.
>     
>     In other terms, pkg-config is a fallback! In your proposal pkg-config is not a fallback but is always used (when found) just to provide an extra hint.. That seems wrong to me.

<quote from Alexander Neundorf because reviewboard wasn't working>
Hmm, I can't load http://git.reviewboard.kde.org currently...

I understand what you mean.
There are other people, who insist on that if pkg-config found something, this
should get priority. So doing it that way will simply be more consistent with
other modules.

When doing it as you propose, which variables end up in the cache ?

Alex
<end quote>

I don't really know how to answer that but i suppose you want to know which vars pkgconfig is setting. That would be the following (according to: http://www.cmake.org/Wiki/CMake:How_To_Find_Libraries#How_package_finding_works):
<NAME>_FOUND
<NAME>_INCLUDE_DIRS or <NAME>_INCLUDES
<NAME>_LIBRARIES or <NAME>_LIBRARIES or <NAME>_LIBS
<NAME>_DEFINITIONS

My initial proposal re-filles SAMBE_INCLUDE_DIR with the data from <NAME>_INCLUDE_DIRS because pkgconfig apparently adds a "s".


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112448/#review39550
-----------------------------------------------------------


On Sept. 2, 2013, 1:04 p.m., Mark Gaiser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112448/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2013, 1:04 p.m.)
> 
> 
> Review request for Build System.
> 
> 
> Description
> -------
> 
> FindSamba couldn't find samba on my machine. Perhaps because it's Samba 4 and it's include folder is /usr/include/samba-4.0/. However, "pkg-config --cflags-only-I smbclient" worked just fine and returned the correct include path. This patch falls back to pkg-config if it couldn't find samba.
> 
> 
> Diffs
> -----
> 
>   cmake/modules/FindSamba.cmake c4df80f 
> 
> Diff: http://git.reviewboard.kde.org/r/112448/diff/
> 
> 
> Testing
> -------
> 
> Works, finds samba without issues.
> 
> 
> Thanks,
> 
> Mark Gaiser
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20130909/8ef5f80a/attachment.html>