Re: [libgpiod] [PATCH] install-tests to bindir test

[Bartosz Golaszewski <brgl@xxxxxxxx>]

wt., 26 lut 2019 o 19:19 Anders Roxell <anders.roxell@xxxxxxxxxx> napisał(a):
>
> On Tue, 26 Feb 2019 at 16:57, Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> >
> > wt., 26 lut 2019 o 16:37 Anders Roxell <anders.roxell@xxxxxxxxxx> napisał(a):
> > >
> > > When building the tests it assumes that the build artifacts is located
> > > in the srcdir.
> > > Rework so we install tests into the bindir, if that is done we look for
> > > the binaries in bin dir and not in the src dir.
> > >
> > > Signed-off-by: Anders Roxell <anders.roxell@xxxxxxxxxx>
> > > ---
> >
> > Hi Anders,
> >
> > I guess this is for kernel.ci, right?
>
> Yes.
>
> > Just an FYI - once v5.1 is
> > released, the tests will stop working as we're changing the
> > gpio-mockup debugfs interface.
>
> aha, thanks for the heads up.
>
> > I'll try to have a new release ready at
> > that time, but I thought I'd let you know.
>
> Thank you, can we add a switch so it will work with both the new and
> old interfaces some how?
>

I'd prefer not to keep legacy code in the repo. The new interface
completely changes the approach (for instance: we'll be able to verify
the results of actions over debugfs, which we now cannot). Updating
the kernel in lockstep with libgpiod will work though. Maybe we should
wait with adding the tests until v5.1 is out?

> >
> > >  configure.ac       | 15 +++++++++++++++
> > >  libgpiod.pc.in     |  1 +
> > >  tests/Makefile.am  | 11 ++++++++++-
> > >  tests/gpiod-test.c | 10 +++++-----
> > >  4 files changed, 31 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/configure.ac b/configure.ac
> > > index 49bedf4d2410..b93feab6ebcc 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -113,6 +113,21 @@ then
> > >         AC_CHECK_HEADERS([sys/signalfd.h], [], [HEADER_NOT_FOUND_TOOLS([sys/signalfd.h])])
> > >  fi
> > >
> > > +AC_ARG_ENABLE([install-tests],
> > > +       [AC_HELP_STRING([--enable-install-tests],
> > > +               [enable install tests [default=no]])],
> > > +       [
> > > +               if test "x$enableval" = xyes
> > > +               then
> > > +                       with_install_tests=true
> > > +                       with_tests=true
> > > +               else
> > > +                       with_install_tests=false
> > > +               fi
> > > +       ],
> >
> > Can you put it on a single line as is done in commit 04f566d5bde8a in
> > current master?
>
> of course I'll fix it.
>
> >
> > > +       [with_install_tests=false])
> > > +AM_CONDITIONAL([WITH_INSTALL_TESTS], [test "x$with_install_tests" = xtrue])
> > > +
> > >  AC_ARG_ENABLE([tests],
> > >         [AC_HELP_STRING([--enable-tests],
> > >                 [enable libgpiod tests [default=no]])],
> > > diff --git a/libgpiod.pc.in b/libgpiod.pc.in
> > > index 48ff11392ae4..96d1111324e5 100644
> > > --- a/libgpiod.pc.in
> > > +++ b/libgpiod.pc.in
> > > @@ -1,5 +1,6 @@
> > >  prefix=@prefix@
> > >  exec_prefix=@exec_prefix@
> > > +bindir=@bindir@
> > >  libdir=@libdir@
> > >  includedir=@includedir@
> > >
> > > diff --git a/tests/Makefile.am b/tests/Makefile.am
> > > index a9319a725f0d..e4b14274081f 100644
> > > --- a/tests/Makefile.am
> > > +++ b/tests/Makefile.am
> > > @@ -9,10 +9,19 @@
> > >  AM_CFLAGS = -I$(top_srcdir)/include/ -include $(top_builddir)/config.h
> > >  AM_CFLAGS += -Wall -Wextra -g $(KMOD_CFLAGS) $(UDEV_CFLAGS)
> > >  AM_LDFLAGS = -pthread
> > > -LDADD = ../src/lib/libgpiod.la $(KMOD_LIBS) $(UDEV_LIBS)
> > > +LDADD = $(top_builddir)/src/lib/libgpiod.la $(KMOD_LIBS) $(UDEV_LIBS)
> > >
> > >  check_PROGRAMS = gpiod-test
> > >
> > > +if WITH_INSTALL_TESTS
> > > +bin_PROGRAMS = $(check_PROGRAMS)
> > > +TOOLS_PATH=@prefix@/bin
> >
> > Let's stick to the used namespace - please call it GPIOD_TOOLS_PATH.
>
> Yes.
>
> >
> > > +else
> > > +noinst_PROGRAMS = $(check_PROGRAMS)
> > > +TOOLS_PATH="./../../src/tools"
> > > +endif
> > > +AM_CFLAGS += -DTOOLS_PATH=$(TOOLS_PATH)/
> > > +
> > >  gpiod_test_SOURCES =   gpiod-test.c \
> > >                         gpiod-test.h \
> > >                         tests-chip.c \
> > > diff --git a/tests/gpiod-test.c b/tests/gpiod-test.c
> > > index 92d6b7875fef..fda9d9d50fb5 100644
> > > --- a/tests/gpiod-test.c
> > > +++ b/tests/gpiod-test.c
> > > @@ -30,6 +30,9 @@
> > >  #define NORETURN       __attribute__((noreturn))
> > >  #define MALLOC         __attribute__((malloc))
> > >
> > > +#define xstr(s) str(s)
> > > +#define str(s) #s
> >
> > I'm not sure what xstr() is here for and also I actually prefer
> > stringification without macro wrappers - it's more readable.
>
> The macro's are there to redo the GPIOD_TOOLS_PATH to a
> string, its either that or we have to define it as a string from start
> via the shell, unsure how to do that, also will that be more readable ?
>

Oh I'm seeing why you did this like this:
https://gcc.gnu.org/onlinedocs/gcc-3.4.4/cpp/Stringification.html

Nevermind my comment, let's leave it like it is.

> Cheers,
> Anders
>
> >
> > > +
> > >  static const unsigned int min_kern_major = 4;
> > >  static const unsigned int min_kern_minor = 16;
> > >  static const unsigned int min_kern_release = 0;
> > > @@ -449,12 +452,9 @@ static void gpiotool_proc_dup_fds(int in_fd, int out_fd, int err_fd)
> > >
> > >  static char *gpiotool_proc_get_path(const char *tool)
> > >  {
> > > -       char *path, *progpath, *progdir;
> > > +       char *path;
> > >
> > > -       progpath = xstrdup(program_invocation_name);
> > > -       progdir = dirname(progpath);
> > > -       path = xappend(NULL, "%s/../../src/tools/%s", progdir, tool);
> > > -       free(progpath);
> > > +       path = xappend(NULL, "%s%s", xstr(TOOLS_PATH), tool);
> > >
> > >         return path;
> > >  }
> > > --
> > > 2.20.1
> > >
> >
> > Best regards,
> > Bartosz Golaszewski