Review Request 129316: Add new cmake module `FindLibinotify.cmake`.

[Adriaan de Groot]

> On Nov. 9, 2016, 9:21 a.m., Adriaan de Groot wrote:
> > This adds a cmake module to do the right kind of checking (for inotify) for the platform:
> >   - on linux, check for the kernel subsystem / header
> >   - on freebsd, check for the port / header
> > So it's still going to have knock-on effects (or subsequent reviews and changes) for consumers of inotify?
> > 
> > The name FindLibinotify doesn't really make sense for the linux case (where it's not a library). Should it be named FindInotify instead?
> 
> Tobias Berner wrote:
>     Yes, it require further changes as in other stuff, as ${LIBINOTIFY_INCLUDE_DIRS} ${LIBINOTIFY_LIBRARIES} needs to be added to the consumers.
> 
> Adriaan de Groot wrote:
>     I would suggest naming it FindInotify then, since that's the functionality it's looking for; also makes the change look less weird on Linux, where Libinotify doesn't make sense.
>     
>     Some notes:
>      - on linux, sys/inotify.h always (?) exists
>      - on FreeBSD, /usr/local/include/sys/inotify.h is installed from a package not related to the kernel
>     
>     So the check for sys/inotify.h in this module is OK on both systems.
>     
>     Shouldn't the if(Linux) branch also set LIBINOTIFY_LIBS and LIBINOTIFY_INCLUDE_DIRS (to empty values), to avoid warnings later?

Would you mind 2-clause BSD licensing this? That makes legal review easier for the downstreams KDE has (since clause 3 then doesn't need to be checked).


- Adriaan de


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129316/#review100741
-----------------------------------------------------------


On Nov. 3, 2016, 7:30 a.m., Tobias Berner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129316/
> -----------------------------------------------------------
> 
> (Updated Nov. 3, 2016, 7:30 a.m.)
> 
> 
> Review request for Extra Cmake Modules, Adriaan de Groot, Gleb Popov, and Raphael Kubo da Costa.
> 
> 
> Repository: extra-cmake-modules
> 
> 
> Description
> -------
> 
> FreeBSD also has inotify. However it is a library and not a kernel subsystem.
> 
> 
> With that change, targets checking for sys/inotify.h could switch from something like 
> ```
> option(ENABLE_INOTIFY "Try to use inotify for directory monitoring" ON)
> if(ENABLE_INOTIFY)
>     include(CheckIncludeFiles)
>     check_include_files(sys/inotify.h SYS_INOTIFY_H_FOUND)
>     set(HAVE_SYS_INOTIFY_H ${SYS_INOTIFY_H_FOUND})
> endif()
> ```
> to 
> ```
> option(ENABLE_INOTIFY "Try to use inotify for directory monitoring" ON)
> if(ENABLE_INOTIFY) 
>     find_package(Libinotify)
>     set_package_properties(Libinotify PROPERTIES
>                                                                   PURPOSE "Filesystem alteration notifications using inotify")
>     set(HAVE_SYS_INOTIFY_H ${LIBINOTIFY_FOUND})
> else()
> set(HAVE_SYS_INOTIFY_H FALSE)
> endif()
> ```
> [from kcoreaddons], and append `${LIBINOTIFY_INCLUDE_DIRS}` to the `include_directories`, aswell as `${LIBINOTIFY_LIBRARIES}` to the link-libraires.
> 
> 
> Diffs
> -----
> 
>   find-modules/FindLibinotify.cmake PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129316/diff/
> 
> 
> Testing
> -------
> 
> Used in the unofficial KDE FreeBSD ports.
> 
> 
> Thanks,
> 
> Tobias Berner
> 
>

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