Re: [libgpiod PATCH] Add cmake support

[Andreas Pokorny <andreas.pokorny@xxxxxxxxx>]

Hi,

Am Mi., 15. Sept. 2021 um 13:39 Uhr schrieb Alexander Dahl <ada@xxxxxxxxxxx>:
>
> Hello Andreas,
>
> just a quick glance over it, but some things catched my eye even
> without proper review. See below.

Nice, looked proper enough for me!

> Am Wed, Sep 15, 2021 at 12:59:58PM +0200 schrieb Andreas Pokorny:
> > This is a wip cmake support for libgpiod. It allows to integrate
> > libgpiod as part of a bigger cmake project built via the new package
> > management feature of cmake called FetchContent or as git submodule.
> > ...
> > +cmake_minimum_required(VERSION 3.14)
>
> What's the actual CMake feature you are using requiring 3.14, or is it
> the version you happened to have?

3.14 for the FetchContent_MakeAvailable, 3.13 for the options policy

> > +project(libgpiod VERSION 2.0.0 LANGUAGES C CXX)
> > [...]
> > +check_symbol_exists(ioctl "sys/ioctl.h" HAVE_IOCTL)
> > +set(CMAKE_REQUIRED_DEFINITIONS -D_GNU_SOURCE)
>
> This is no CMake variable, you should not use that prefix for your own
> variables.

check_symbol_exists are old macro facilities that use the variable above
to influence the compile test.

> > +check_symbol_exists(asprintf "stdio.h" HAVE_ASPRINTF)
> > +check_symbol_exists(scandir "dirent.h" HAVE_SCANDIR)
> > +check_symbol_exists(alphasort "dirent.h" HAVE_ALPHASORT)
> > [...]
> > +  set_target_properties(gpiomockup PROPERTIES
> > +    VERSION ${PROJECT_VERSION}
> > +    SOVERSION ${GPIOD_MOCK_SOVERSION}
>
> You should use only the major version part for SOVERSION property.

Ah yes I forgot about the revision and age stuff.


> > +    PUBLIC_HEADER tests/mockup/gpio-mockup.h
> > +    )
> > +  target_compile_options(gpiomockup PRIVATE -Wall -Wextra
> > -fvisibility=hidden -include ${CMAKE_BINARY_DIR}/config.h)
>
> This is compiler specific and might conflict with what the user
> wanting to build this thing wants.
>
> > +  target_link_libraries(gpiomockup PRIVATE ${KMOD_LDFLAGS}
> > ${UDEV_LDFLAGS} Threads::Threads)
>
> If you're using PkgConfig and recent CMake anyways, please use the
> IMPORTED_TARGET option of pkg_check_modules and use that for
> target_link_libraries.
>
> > +  target_include_directories(gpiomockup PUBLIC
> > $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/tests/mockup/>)
> > +
> > +  enable_testing()
> > +  add_executable(gpiod_tests
> > +        tests/gpiod-test.c
> > +        tests/gpiod-test.h
> > +        tests/tests-chip.c
> > +        tests/tests-event.c
> > +        tests/tests-line.c
> > +        tests/tests-misc.c)
> > +  target_compile_options(gpiod_tests PRIVATE ${GLIB_CFLAGS} -Wall
> > -Wextra -include ${CMAKE_BINARY_DIR}/config.h)
>
> Those glib cflags should come with pkgconfig imported target, for GCC
> options see above.
>
> Is there any reason to include that config.h header like this over
> adding $<BUILD_INTERFACE:${CMAKE_BINARY_DIR}> to
> target_include_directories?

This is done in the autotools build. This only replicates the solution. I am not
particularly fond of doing something like that. The two relevant flags
configured
that way are only used internally and never exposed to users of the library.

Thanks for the review - I hope the next update addresses all findings.

kind regards
Andreas