Re: [PATCH] bindings: python: fix out-of-source build

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Jan 16, 2023 at 10:07 PM Bartosz Golaszewski
<bartosz.golaszewski@xxxxxxxxxx> wrote:
>
> On Mon, 16 Jan 2023 at 19:15, <jf@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > From: Joerg Faschingbauer <jf@xxxxxxxxxxxxxxxxxxx>
> >
> > Makefile.am delegates the build of the python extension to its
> > setup.py file, which references the extension .c files relative to the
> > source dir. This makes it impossible to build in a directory that is
> > different from the source directory (for example, for PC and ARM but
> > from the same source).
> >
>
> Thanks for sparing me the surprise of finding that out when writing
> the bitbake recipe. Great catch!
>
> A couple nits below:
>
> > * Modify Makefile.am to pass automake's $(srcdir) into setup.py via
> >   GPIOD_SRCDIR environment variable.
> > * Modify setup.py to pick up .c files relative from that directory.
>
> Can you leave your Sign-off here?
>
> > ---
> >  bindings/python/Makefile.am |  6 ++++--
> >  bindings/python/setup.py    | 23 ++++++++++++++---------
> >  2 files changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/bindings/python/Makefile.am b/bindings/python/Makefile.am
> > index 3212a8f..6c2f99b 100644
> > --- a/bindings/python/Makefile.am
> > +++ b/bindings/python/Makefile.am
> > @@ -12,13 +12,15 @@ endif
> >  all-local:
> >         GPIOD_VERSION_STRING=$(VERSION_STR) \
> >         GPIOD_WITH_TESTS=$(BUILD_TESTS) \
> > -       $(PYTHON) setup.py build_ext --inplace \
> > +       GPIOD_SRCDIR=$(srcdir) \
> > +       $(PYTHON) $(srcdir)/setup.py build_ext --inplace \
> >                 --include-dirs=$(top_srcdir)/include/:$(top_srcdir)/tests/gpiosim/ \
> >                 --library-dirs=$(top_builddir)/lib/.libs/:$(top_srcdir)/tests/gpiosim/.libs/
> >
> >  install-exec-local:
> >         GPIOD_WITH_TESTS= \
> > -       $(PYTHON) setup.py install --prefix=$(prefix)
> > +       GPIOD_SRCDIR=$(srcdir) \
> > +       $(PYTHON) $(srcdir)/setup.py install --prefix=$(prefix)
> >
> >  SUBDIRS = gpiod
> >
> > diff --git a/bindings/python/setup.py b/bindings/python/setup.py
> > index a951069..3ab01e1 100644
> > --- a/bindings/python/setup.py
> > +++ b/bindings/python/setup.py
> > @@ -2,17 +2,22 @@
> >  # SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <brgl@xxxxxxxx>
> >
> >  from os import environ
> > +import os.path
>
> Maybe `from os import environ, path`?
>
> >  from setuptools import setup, Extension, find_packages
> >
> > +srcdir = environ.get('GPIOD_SRCDIR', '.')
> > +def src(path):
> > +    return os.path.join(srcdir, path)
> > +
> >  gpiod_ext = Extension(
> >      "gpiod._ext",
> >      sources=[
> > -        "gpiod/ext/chip.c",
> > -        "gpiod/ext/common.c",
> > -        "gpiod/ext/line-config.c",
> > -        "gpiod/ext/line-settings.c",
> > -        "gpiod/ext/module.c",
> > -        "gpiod/ext/request.c",
> > +        src("gpiod/ext/chip.c"),
> > +        src("gpiod/ext/common.c"),
> > +        src("gpiod/ext/line-config.c"),
> > +        src("gpiod/ext/line-settings.c"),
> > +        src("gpiod/ext/module.c"),
> > +        src("gpiod/ext/request.c"),
> >      ],
> >      define_macros=[("_GNU_SOURCE", "1")],
> >      libraries=["gpiod"],
> > @@ -21,7 +26,7 @@ gpiod_ext = Extension(
> >
> >  gpiosim_ext = Extension(
> >      "tests.gpiosim._ext",
> > -    sources=["tests/gpiosim/ext.c"],
> > +    sources=[src("tests/gpiosim/ext.c")],
> >      define_macros=[("_GNU_SOURCE", "1")],
> >      libraries=["gpiosim"],
> >      extra_compile_args=["-Wall", "-Wextra"],
> > @@ -29,7 +34,7 @@ gpiosim_ext = Extension(
> >
> >  procname_ext = Extension(
> >      "tests.procname._ext",
> > -    sources=["tests/procname/ext.c"],
> > +    sources=[src("tests/procname/ext.c")],
> >      define_macros=[("_GNU_SOURCE", "1")],
> >      extra_compile_args=["-Wall", "-Wextra"],
> >  )
> > @@ -39,7 +44,7 @@ if "GPIOD_WITH_TESTS" in environ and environ["GPIOD_WITH_TESTS"] == "1":
> >      extensions.append(gpiosim_ext)
> >      extensions.append(procname_ext)
> >
> > -with open("gpiod/version.py", "r") as fd:
> > +with open(src("gpiod/version.py"), "r") as fd:
> >      exec(fd.read())
> >
> >  setup(
> > --
> > 2.38.1
> >
>
> Looks good otherwise!
>
> Bart

Now when I'm thinking about it: why not simply use the __file__
variable, run it through dirname() and derive the right directory like
that?

Bart