Re: [libgpiod][PATCH v5 1/1] bindings: python: optionally include module in sdist

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Oct 19, 2023 at 12:42 PM Phil Howard <phil@xxxxxxxxxxxxx> wrote:
>
> On Thu, 19 Oct 2023 at 10:41, Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> >
> > On Wed, Oct 18, 2023 at 10:57 PM Phil Howard <phil@xxxxxxxxxxxxx> wrote:
> > >
> > > Optionally vendor libgpiod source into sdist so that the Python module can
> > > be built from source, even with a missing or mismatched system libgpiod.
> > >
> > > Add two new environment variables "LINK_SYSTEM_LIBGPIOD" and
> > > "GPIOD_VERSION" to control what kind of package setup.py will build.
> > >
> > > In order to build an sdist or wheel package with a vendored libgpiod a
> > > version must be specified via the "GPIOD_VERSION" environment variable.
> > >
> > > This will instruct setup.py to fetch the tarball matching the requested
> > > version from git.kernel.org, unpack it, and copy the lib and include
> > > directories into the package root so they can be included in sdist or used
> > > to build a binary wheel.
> > >
> > > eg: GPIOD_VERSION=2.0.2 python3 setup.py sdist
> > >
> > > Will build a source distribution with gpiod version 2.0.2 source included.
> > >
> > > It will also save the gpiod version into "gpiod-version.txt" so that it can
> > > be passed to the build when the sdist is built by pip.
> > >
> > > Requiring an explicit version ensures that the Python bindings - which
> > > can be changed and versions independent of libgpiod -  are built against a
> > > stable libgpiod release.
> > >
> > > In order to force a package with vendored gpiod source to link the system
> > > libgpiod, the "LINK_SYSTEM_LIBGPIOD" environment variable can be used:
> > >
> > > eg: LINK_SYSTEM_LIBGPIOD=1 pip install libgpiod
> > >
> > > Signed-off-by: Phil Howard <phil@xxxxxxxxxxxxx>
> > > ---
> > >  bindings/python/MANIFEST.in |   5 ++
> > >  bindings/python/setup.py    | 131 +++++++++++++++++++++++++++++++++---
> > >  2 files changed, 125 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/bindings/python/MANIFEST.in b/bindings/python/MANIFEST.in
> > > index c7124d4..0aa9079 100644
> > > --- a/bindings/python/MANIFEST.in
> > > +++ b/bindings/python/MANIFEST.in
> > > @@ -2,6 +2,7 @@
> > >  # SPDX-FileCopyrightText: 2023 Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > >
> > >  include setup.py
> > > +include gpiod-version.txt
> > >
> > >  recursive-include gpiod *.py
> > >  recursive-include tests *.py
> > > @@ -11,3 +12,7 @@ recursive-include gpiod/ext *.h
> > >
> > >  recursive-include tests/gpiosim *.c
> > >  recursive-include tests/procname *.c
> > > +
> > > +recursive-include lib *.c
> > > +recursive-include lib *.h
> > > +recursive-include include *.h
> > > diff --git a/bindings/python/setup.py b/bindings/python/setup.py
> > > index df10e18..f0d5c1f 100644
> > > --- a/bindings/python/setup.py
> > > +++ b/bindings/python/setup.py
> > > @@ -1,24 +1,136 @@
> > >  # SPDX-License-Identifier: GPL-2.0-or-later
> > >  # SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <brgl@xxxxxxxx>
> > >
> > > -from os import environ, path
> > > -from setuptools import setup, Extension, find_packages
> > > +import tarfile
> > > +from os import getenv, path, unlink
> > > +from shutil import copytree, rmtree
> > > +from urllib.request import urlretrieve
> >
> > Doesn't it make our setup.py depend on additional packages on top of
> > the standard library?
>
> "tarfile" and "urllib" are both part of the CPython standard library [1]
>

Yocto has a separate recipe for urllib3 but also has an urllib module
in python3-core. Is this expected? What is the relationship between
the two?

> They'd be in pip otherwise, since it would necessarily need to fetch and
> unpack tarballs like the sdist. (pip has a habit of vendoring things)

Yocto doesn't use pip, it fetches the sources over http. All
dependencies must be satisfied by existing yocto recipes.

>
> If there are cases where these are missing (Yocto weirdness?) then I can
> move the imports to just-in-time so they're a soft dependency only for
> when a package is built.
>

Yocto has a different weirdness - it splits the standard library into
~65 separate packages so that you can keep the image small if you
don't need the entire thing.

> [1] - https://docs.python.org/3/library/index.html
>
> >
> > > +
> > > +from setuptools import Extension, find_packages, setup
> > >  from setuptools.command.build_ext import build_ext as orig_build_ext
> > > -from shutil import rmtree
> > > +from setuptools.command.sdist import sdist as orig_sdist
> > > +
> > > +LINK_SYSTEM_LIBGPIOD = getenv("LINK_SYSTEM_LIBGPIOD") == "1"
> > > +GPIOD_VERSION = getenv("GPIOD_VERSION")
> >
> > I'd call it LIBGPIOD_VERSION to be more explicit.
> >
> > > +GPIOD_WITH_TESTS = getenv("GPIOD_WITH_TESTS") == "1"
> > > +SRC_BASE_URL = "https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/snapshot/";
> >
> > I would prefer to use the http mirror at
> > https://mirrors.edge.kernel.org/pub/software/libs/libgpiod/ rather
> > than git snapshots. We have better control over what's in there
>
> Yessir!
>
> > and we can even verify the checksums after download if needed.
>
> We can pull the checksum from sha256sums.asc and verify with hashlib
> but we can't verify the gpg signature without a dependency I think.
>
> >
> > > +SRC_FILENAME = "libgpiod-{version}.tar.gz"
> >
> > Maybe put it in /tmp?
>
> Sure "tempfile" is also in the stl, so might as well use it!
>

Sounds good!

Bart

> >
> > Bart
> >
> > > +
> > > +# __version__
> > > +with open("gpiod/version.py", "r") as fd:
> > > +    exec(fd.read())
> > > +
> > > +
> > > +def fetch_tarball(func):
> > > +    # If no GPIOD_VERSION is specified in env, just run the task
> > > +    if GPIOD_VERSION is None:
> > > +        return func
> > > +
> > > +    # If GPIOD_VERSION is specified, fetch the requested version tarball
> > > +    # and create gpiod-version.txt so the sdist package knows what version
> > > +    # it's building.
> > > +    def wrapper(self):
> > > +        TARBALL = SRC_FILENAME.format(version=GPIOD_VERSION)
> > > +
> > > +        print(f"fetching: {SRC_BASE_URL + TARBALL}")
> > > +
> > > +        filename, headers = urlretrieve(SRC_BASE_URL + TARBALL, TARBALL)
> > > +
> > > +        if not tarfile.is_tarfile(filename):
> > > +            print(f"error: refusing to build sdist (invalid tarball {TARBALL})")
> > > +            return
> > > +
> > > +        # Unpack the downloaded tarball
> > > +        print(f"unpacking: {filename}")
> > > +        file = tarfile.open(filename)
> > > +        file.extractall(".")
> > > +        file.close()
> > > +        unlink(filename)
> > > +
> > > +        # Copy the include and lib directories we need to build libgpiod
> > > +        copytree(f"libgpiod-{GPIOD_VERSION}/include/", "./include")
> > > +        copytree(f"libgpiod-{GPIOD_VERSION}/lib/", "./lib")
> > > +        rmtree(f"libgpiod-{GPIOD_VERSION}")
> > > +
> > > +        # Save the gpiod version for sdist
> > > +        open("gpiod-version.txt", "w").write(GPIOD_VERSION)
> > > +
> > > +        func(self)
> > > +
> > > +        rmtree("./lib", ignore_errors=True)
> > > +        rmtree("./include", ignore_errors=True)
> > > +        unlink("gpiod-version.txt")
> > > +
> > > +    return wrapper
> > >
> > >
> > >  class build_ext(orig_build_ext):
> > >      """
> > > -    setuptools install all C extentions even if they're excluded in setup().
> > > -    As a workaround - remove the tests directory right after all extensions
> > > -    were built (and possibly copied to the source directory if inplace is set).
> > > +    Wrap build_ext to amend the module sources and settings to build
> > > +    the bindings and gpiod into a combined module when a version is
> > > +    specified and LINK_SYSTEM_LIBGPIOD=1 is not present in env.
> > > +
> > > +    run is wrapped with @fetch_tarball in order to fetch the sources
> > > +    needed to build binary wheels when GPIOD_VERSION is specified, eg:
> > > +
> > > +    GPIOD_VERSION="2.0.2" python3 -m build .
> > >      """
> > >
> > > +    @fetch_tarball
> > >      def run(self):
> > > +        # Try to get the gpiod version from the .txt file included in sdist
> > > +        try:
> > > +            gpiod_version = open("gpiod-version.txt", "r").read()
> > > +        except OSError:
> > > +            gpiod_version = GPIOD_VERSION
> > > +
> > > +        if gpiod_version and not LINK_SYSTEM_LIBGPIOD:
> > > +            # When building the extension from an sdist with a vendored
> > > +            # amend gpiod._ext sources and settings accordingly.
> > > +            gpiod_ext = self.ext_map["gpiod._ext"]
> > > +            gpiod_ext.sources += [
> > > +                "lib/chip.c",
> > > +                "lib/chip-info.c",
> > > +                "lib/edge-event.c",
> > > +                "lib/info-event.c",
> > > +                "lib/internal.c",
> > > +                "lib/line-config.c",
> > > +                "lib/line-info.c",
> > > +                "lib/line-request.c",
> > > +                "lib/line-settings.c",
> > > +                "lib/misc.c",
> > > +                "lib/request-config.c",
> > > +            ]
> > > +            gpiod_ext.libraries = []
> > > +            gpiod_ext.include_dirs = ["include", "lib", "gpiod/ext"]
> > > +            gpiod_ext.extra_compile_args.append(
> > > +                f'-DGPIOD_VERSION_STR="{gpiod_version}"',
> > > +            )
> > > +
> > >          super().run()
> > > +
> > > +        # We don't ever want the module tests directory in our package
> > > +        # since this might include gpiosim._ext or procname._ext from a
> > > +        # previous dirty build tree.
> > >          rmtree(path.join(self.build_lib, "tests"), ignore_errors=True)
> > >
> > >
> > > +class sdist(orig_sdist):
> > > +    """
> > > +    Wrap sdist in order to fetch the libgpiod source files for vendoring
> > > +    into a source distribution.
> > > +
> > > +    run is wrapped with @fetch_tarball in order to fetch the sources
> > > +    needed to build binary wheels when GPIOD_VERSION is specified, eg:
> > > +
> > > +    GPIOD_VERSION="2.0.2" python3 -m build . --sdist
> > > +    """
> > > +
> > > +    @fetch_tarball
> > > +    def run(self):
> > > +        super().run()
> > > +
> > > +
> > >  gpiod_ext = Extension(
> > >      "gpiod._ext",
> > >      sources=[
> > > @@ -50,19 +162,16 @@ procname_ext = Extension(
> > >  )
> > >
> > >  extensions = [gpiod_ext]
> > > -if environ.get("GPIOD_WITH_TESTS") == "1":
> > > +if GPIOD_WITH_TESTS:
> > >      extensions.append(gpiosim_ext)
> > >      extensions.append(procname_ext)
> > >
> > > -with open("gpiod/version.py", "r") as fd:
> > > -    exec(fd.read())
> > > -
> > >  setup(
> > >      name="libgpiod",
> > >      packages=find_packages(exclude=["tests", "tests.*"]),
> > >      python_requires=">=3.9.0",
> > >      ext_modules=extensions,
> > > -    cmdclass={"build_ext": build_ext},
> > > +    cmdclass={"build_ext": build_ext, "sdist": sdist},
> > >      version=__version__,
> > >      author="Bartosz Golaszewski",
> > >      author_email="brgl@xxxxxxxx",
> > > --
> > > 2.34.1
> > >