All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andreas Pokorny <andreas.pokorny@gmail.com>
To: linux-gpio@vger.kernel.org
Subject: Re: [libgpiod PATCH] Add cmake support
Date: Wed, 15 Sep 2021 16:43:07 +0200	[thread overview]
Message-ID: <CAFKLa4LUWq9Z7MmNt701z3BNFiyfcUTwS3fsU97Keya0YGzEsA@mail.gmail.com> (raw)
In-Reply-To: <YUHbb1awZcmih5PC@ada.ifak-system.com>

Hi,

Am Mi., 15. Sept. 2021 um 13:39 Uhr schrieb Alexander Dahl <ada@thorsis.com>:
>
> 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

      reply	other threads:[~2021-09-15 14:43 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-15 10:59 [libgpiod PATCH] Add cmake support Andreas Pokorny
2021-09-15 11:39 ` Alexander Dahl
2021-09-15 14:43   ` Andreas Pokorny [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFKLa4LUWq9Z7MmNt701z3BNFiyfcUTwS3fsU97Keya0YGzEsA@mail.gmail.com \
    --to=andreas.pokorny@gmail.com \
    --cc=linux-gpio@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.