> 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.
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. - 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 > >
_______________________________________________ Kde-buildsystem mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-buildsystem
