> On Nov. 9, 2016, 10: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?
>
> Adriaan de Groot wrote:
> 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).
Sure, I don't have any issue with dropping the third clause.
I'll update/rename the file as soon as I find time.
- Tobias
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129316/#review100741
-----------------------------------------------------------
On Nov. 3, 2016, 8: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, 8: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
>
>
