[bug#46848,PATCHES,core-updates] PEP 517 python-build-system

Hi everyone,

the attached patches switch python-build-system to a PEP 517-based build
system using python-pypa-build.

As discussed previously Python is currently in the process of opening up
for build systems other than setuptools using the API specified in PEP
517. python-pypa-build is a simple tool for building packages using that
new API. It supports setup.py-based builds as a fallback, if no
pyproject.toml is present.

One downside is that this tool is not self-contained and has a few
dependencies. Thus first I bootstrap setuptools using itself (possible
because it bundles all of its own dependencies), then build
python-pypa-build’s dependencies using setuptools (which is fortunately
still possible) and then combine everything into a
python-toolchain(-for-build), which is then used by the build-process.

I can successfully build packages like python-pypa-build and
python-pytest and python-pep517-bootstrap. The latter is using flit as
its build backend. But other packages currently fail because I removed
some arguments.

Cheers,
Lars

Hi Lars,

Here's a review of patches 1 to 6.

Lars-Dominik Braun <lars@6xq.net> writes:

> the attached patches switch python-build-system to a PEP 517-based build
> system using python-pypa-build.

Neat!  Thank you for working on this!

> One downside is that this tool is not self-contained and has a few
> dependencies. Thus first I bootstrap setuptools using itself (possible
> because it bundles all of its own dependencies), then build
> python-pypa-build’s dependencies using setuptools (which is fortunately
> still possible) and then combine everything into a
> python-toolchain(-for-build), which is then used by the build-process.
>
> I can successfully build packages like python-pypa-build and
> python-pytest and python-pep517-bootstrap. The latter is using flit as
> its build backend. But other packages currently fail because I removed
> some arguments.
> Lars
>
>
>
>
>>From 61313d8ddba30772e2587e3e16ca30d1565d3c7e Mon Sep 17 00:00:00 2001
> From: Lars-Dominik Braun <lars@6xq.net>
> Date: Sun, 28 Feb 2021 13:05:51 +0100
> Subject: [PATCH 04/12] gnu: python-setuptools: Bootstrap using itself
>
> * gnu/packages/python-xyz.scm (python-setuptools) [arguments]: Add phase
> setting GUIX_PYTHONPATH to source directory.
> ---
>  gnu/packages/python-xyz.scm | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/gnu/packages/python-xyz.scm b/gnu/packages/python-xyz.scm
> index f8afa13f33..79d01f700a 100644
> --- a/gnu/packages/python-xyz.scm
> +++ b/gnu/packages/python-xyz.scm
> @@ -1144,7 +1144,18 @@ other machines, such as over the network.")
>      ;; FIXME: Tests require pytest, which itself relies on setuptools.
>      ;; One could bootstrap with an internal untested setuptools.
>      (arguments
> -     `(#:tests? #f))
> +     `(#:tests? #f
> +       #:python ,python-wrapper
> +       #:phases (modify-phases %standard-phases
> +                  ;; Use this setuptools’ sources to bootstrap themselves.
> +                  (add-before 'build 'set-PYTHONPATH
                                             ^ nitpick: GUIX_PYTHONPATH

> +                    (lambda _
> +                      (format #t "current working dir ~s~%" (getcwd))
> +                      (setenv "GUIX_PYTHONPATH"
> +                              (string-append ".:" (getenv "GUIX_PYTHONPATH")))
> +                      #t)))))
> +    ;; Not required when not building a wheel
> +    ;(propagated-inputs `(("python-wheel" ,python-wheel)))
>      (home-page "https://pypi.org/project/setuptools/")
>      (synopsis
>       "Library designed to facilitate packaging Python projects")
> -- 
> 2.26.2
>
>
>>From 7a99aaa40e65fde58ee2e78ad7d3e0ccd6d169ae Mon Sep 17 00:00:00 2001
> From: Lars-Dominik Braun <lars@6xq.net>
> Date: Sun, 28 Feb 2021 13:08:58 +0100
> Subject: [PATCH 05/12] python-build: Switch to PEP 517-based build
>
> * gnu/packages/python-commencement.scm: New file, containing
> python-toolchain.
> * gnu/local.mk: Add it.
> * gnu/packages/python.scm (python): Disable installing bundled
> pip/setuptools.
> * guix/build/python-build-system.scm: Rewrite using python-pypa-build.
> * guix/build-system/python.scm (default-python): Switch to
> python-toolchain
> (lower): Remove unused parameter.
>
> XXX: rationale

Forgotten XXX comment (perhaps just drop it).

> ---
>  gnu/local.mk                         |   1 +
>  gnu/packages/python-commencement.scm | 175 +++++++++++++++++++
>  gnu/packages/python.scm              |   2 +-
>  guix/build-system/python.scm         |   8 +-
>  guix/build/python-build-system.scm   | 249 ++++++++++++++++++---------
>  5 files changed, 342 insertions(+), 93 deletions(-)
>  create mode 100644 gnu/packages/python-commencement.scm

[...]

> +                   (use-modules (ice-9 match)
> +                                (srfi srfi-1)
> +                                (srfi srfi-26)
> +                                (guix build union))
> +
> +                   (let ((out (assoc-ref %outputs "out")))
> +                     (union-build out (filter-map (match-lambda
> +                                                ((_ . directory) directory))
> +                                              %build-inputs))
> +                     #t))))

Note that returning #t after phases and snippets is obsolete; you can
safely take them all out now.

> +    (inputs
> +     `(("python" ,python-wrapper)
> +       ("python-setuptools" ,python-setuptools)
> +       ("python-pip" ,python-pip))) ; XXX Maybe virtualenv/venv too? It kind of
> +                                    ; defeats the purpose of guix, but is used
> +                                    ; alot in local development.

I think it's OK to have people explicitly add virtualenv if they want
it, rather than grow the set of minimal packages here.  I'd simply
remove the above XXX comment.

> +    (native-search-paths
> +     (package-native-search-paths python))
> +    (search-paths
> +     (package-search-paths python))
> +    (license (package-license python)) ; XXX
> +    (synopsis "Python toolchain")
> +    (description
> +     "Python toolchain including Python itself, setuptools and pip.  Use this
> +package if you need a fully-fledged Python toolchain instead of just the
> +interpreter.")
> +    (home-page (package-home-page python))))

Following my above comment, perhaps s/fully-fledged/minimal/.

[...]

> diff --git a/gnu/packages/python.scm b/gnu/packages/python.scm
> index 8e8f46467b..ca5ce667ef 100644
> --- a/gnu/packages/python.scm
> +++ b/gnu/packages/python.scm
> @@ -182,7 +182,7 @@
>         (list "--enable-shared"          ;allow embedding
>               "--with-system-expat"      ;for XML support
>               "--with-system-ffi"        ;build ctypes
> -             "--with-ensurepip=install" ;install pip and setuptools
> +             "--with-ensurepip=no"      ;do not install pip and setuptools
>               "--enable-unicode=ucs4"
>  
>               ;; Prevent the installed _sysconfigdata.py from retaining a reference
> diff --git a/guix/build-system/python.scm b/guix/build-system/python.scm
> index 2bb6fa87ca..998ea9323d 100644
> --- a/guix/build-system/python.scm
> +++ b/guix/build-system/python.scm
> @@ -65,8 +65,8 @@ extension, such as '.tar.gz'."
>  (define (default-python)
>    "Return the default Python package."
>    ;; Lazily resolve the binding to avoid a circular dependency.
> -  (let ((python (resolve-interface '(gnu packages python))))
> -    (module-ref python 'python-wrapper)))
> +  (let ((python (resolve-interface '(gnu packages python-commencement))))
> +    (module-ref python 'python-toolchain-for-build)))
>  
>  (define (default-python2)
>    "Return the default Python 2 package."
> @@ -172,8 +172,6 @@ pre-defined variants."
>  (define* (python-build store name inputs
>                         #:key
>                         (tests? #t)
> -                       (test-target "test")
> -                       (use-setuptools? #t)
>                         (configure-flags ''())
>                         (phases '(@ (guix build python-build-system)
>                                     %standard-phases))
> @@ -199,9 +197,7 @@ provides a 'setup.py' file as its build system."
>                                    source))
>                       #:configure-flags ,configure-flags
>                       #:system ,system
> -                     #:test-target ,test-target
>                       #:tests? ,tests?
> -                     #:use-setuptools? ,use-setuptools?
>                       #:phases ,phases
>                       #:outputs %outputs
>                       #:search-paths ',(map search-path-specification->sexp
> diff --git a/guix/build/python-build-system.scm b/guix/build/python-build-system.scm
> index 8ade1d5911..a5731511a9 100644
> --- a/guix/build/python-build-system.scm
> +++ b/guix/build/python-build-system.scm
> @@ -34,6 +34,7 @@
>    #:use-module (ice-9 format)
>    #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-26)
> +  #:use-module (srfi srfi-35)
>    #:export (%standard-phases
>              add-installed-pythonpath
>              site-packages
> @@ -108,30 +109,17 @@
>  ;; "--single-version-externally-managed" is set, thus the .egg-info directory
>  ;; and the scripts defined in entry-points will always be created.
>  
> +;; Base error type.
> +(define-condition-type &python-build-error &error
> +  python-build-error?)
>  
> -(define setuptools-shim
> -  ;; Run setup.py with "setuptools" being imported, which will patch
> -  ;; "distutils". This is needed for packages using "distutils" instead of
> -  ;; "setuptools" since the former does not understand the
> -  ;; "--single-version-externally-managed" flag.
> -  ;; Python code taken from pip 9.0.1 pip/utils/setuptools_build.py
> -  (string-append
> -   "import setuptools, tokenize;__file__='setup.py';"
> -   "f=getattr(tokenize, 'open', open)(__file__);"
> -   "code=f.read().replace('\\r\\n', '\\n');"
> -   "f.close();"
> -   "exec(compile(code, __file__, 'exec'))"))
> -
> -(define (call-setuppy command params use-setuptools?)
> -  (if (file-exists? "setup.py")
> -      (begin
> -         (format #t "running \"python setup.py\" with command ~s and parameters ~s~%"
> -                command params)
> -         (if use-setuptools?
> -             (apply invoke "python" "-c" setuptools-shim
> -                    command params)
> -             (apply invoke "python" "./setup.py" command params)))
> -      (error "no setup.py found")))
> +;; Raised when 'check cannot find a valid test system in the inputs.
> +(define-condition-type &test-system-not-found &python-build-error
> +  test-system-not-found?)
> +
> +;; Raised when multiple wheels are created by 'build.
> +(define-condition-type &cannot-extract-multiple-wheels &python-build-error
> +  cannot-extract-multiple-wheels?)
>  
>  (define* (sanity-check #:key tests? inputs outputs #:allow-other-keys)
>    "Ensure packages depending on this package via setuptools work properly,
> @@ -142,23 +130,51 @@ without errors."
>      (with-directory-excursion "/tmp"
>        (invoke "python" sanity-check.py (site-packages inputs outputs)))))
>  
> -(define* (build #:key use-setuptools? #:allow-other-keys)
> +(define* (build #:key outputs #:allow-other-keys)
>    "Build a given Python package."
> -  (call-setuppy "build" '() use-setuptools?)
> +
> +  (define pyproject-build (which "pyproject-build"))
> +
> +  (define (build-pep517)
> +    ;; XXX: should probably use a different path, outside of source directory,
> +    ;; maybe secondary output “wheel”?
> +    (mkdir-p "dist")
> +    (invoke pyproject-build "--outdir" "dist" "--no-isolation" "--wheel" "."))
> +
> +      ;; XXX Would be nice, if we could use bdist_wheel here to remove extra
> +      ;; code path in 'install, but that depends on python-wheel.
> +  (define (build-setuptools)
> +    (invoke "python" "setup.py" "build"))
> +
> +  (if pyproject-build
> +    (build-pep517)
> +    (build-setuptools))
>    #t)
>  
> -(define* (check #:key tests? test-target use-setuptools? #:allow-other-keys)
> +(define* (check #:key inputs outputs tests? #:allow-other-keys)
>    "Run the test suite of a given Python package."
>    (if tests?
> -      ;; Running `setup.py test` creates an additional .egg-info directory in
> -      ;; build/lib in some cases, e.g. if the source is in a sub-directory
> -      ;; (given with `package_dir`). This will by copied to the output, too,
> -      ;; so we need to remove.
> -      (let ((before (find-files "build" "\\.egg-info$" #:directories? #t)))
> -        (call-setuppy test-target '() use-setuptools?)
> -        (let* ((after (find-files "build" "\\.egg-info$" #:directories? #t))
> -               (inter (lset-difference string=? after before)))
> -          (for-each delete-file-recursively inter)))
> +    ;; Unfortunately with PEP 517 there is no common method to specify test
> +    ;; systems. Guess test system based on inputs instead.
> +    (let ((pytest (which "pytest"))
> +            (have-setup-py (file-exists? "setup.py")))
             ^ indentation
             
> +        ;; Prefer pytest
> +        ;; XXX: support nose

You can remove this; nose is stale/deprecated.

> +        (cond
> +          (pytest
> +            (begin
> +              (format #t "using pytest~%")
> +              (invoke pytest "-vv"))) ; XXX: support skipping tests based on name/extra arguments?

We could have a #:test-command argument to specify an arbitrary command
as a list of strings, such as used by the emacs-build-system; that'd
allow us to avoid overriding the phase just to add a '-k "not
this-test"' or similar.

> +          ;; But fall back to setup.py, which should work for most
> +          ;; packages. XXX: would be nice not to depend on setup.py here? fails
> +          ;; more often than not to find any tests at all. Maybe we can run
> +          ;; `python -m unittest`?
> +          (have-setup-py
> +            (begin
> +              (format #t "using setup.py~%")
> +                (invoke "python" "setup.py" "test" "-v")))

As Marius noted, falling back to 'python setup.py test' is not
desirable; it's scheduled to be removed already.

> +          ;; The developer should explicitly disable tests in this case.
> +          (#t (raise (condition (&test-system-not-found))))))
>        (format #t "test suite not run~%"))
>    #t)
>  
> @@ -195,31 +211,109 @@ running checks after installing the package."
>                                  "/bin:"
>                                  (getenv "PATH"))))
>  
> -(define* (install #:key inputs outputs (configure-flags '()) use-setuptools?
> -                  #:allow-other-keys)
> -  "Install a given Python package."
> -  (let* ((out (python-output outputs))
> -         (python (assoc-ref inputs "python"))
> -         (major-minor (map string->number
> -                           (take (string-split (python-version python) #\.) 2)))
> -         (<3.7? (match major-minor
> -                   ((major minor)
> -                    (or (< major 3) (and (= major 3) (< minor 7))))))
> -         (params (append (list (string-append "--prefix=" out)
> -                               "--no-compile")
> -                         (if use-setuptools?
> -                             ;; distutils does not accept these flags
> -                             (list "--single-version-externally-managed"
> -                                   "--root=/")
> -                             '())
> -                         configure-flags)))
> -    (call-setuppy "install" params use-setuptools?)
> -    ;; Rather than produce potentially non-reproducible .pyc files on Pythons
> -    ;; older than 3.7, whose 'compileall' module lacks the
> -    ;; '--invalidation-mode' option, do not generate any.
> -    (unless <3.7?
> -      (invoke "python" "-m" "compileall" "--invalidation-mode=unchecked-hash"
> -              out))))
> +(define* (install #:key inputs outputs (configure-flags '()) #:allow-other-keys)
> +  "Install a wheel file according to PEP 427"
> +  ;; See https://www.python.org/dev/peps/pep-0427/#installing-a-wheel-distribution-1-0-py32-none-any-whl
> +  (let* ((site-dir (site-packages inputs outputs))
> +         (out (assoc-ref outputs "out")))
> +    (define (extract file)
> +      "Extract wheel (ZIP file) into site-packages directory"
> +      ;; Use Python’s zipfile to avoid extra dependency
                                               
> +      (invoke "python" "-m" "zipfile" "-e" file site-dir))
> +
> +    (define python-hashbang
> +      (string-append "#!" (assoc-ref inputs "python") "/bin/python"))
> +
> +    (define (move-data source destination)
> +      (mkdir-p (dirname destination))
> +      (rename-file source destination))
> +
> +    (define (move-script source destination)
> +      "Move executable script file from .data/scripts to out/bin and replace
> +temporary hashbang"
> +	  (move-data source destination)
> +      ;; ZIP does not save/restore permissions, make executable
> +      ;; XXX: might not be a file, but directory with subdirectories
> +      (chmod destination #o755)
> +      (substitute* destination (("#!python") python-hashbang)))

It seems the directory case should be handled?  Otherwise the
substitute* call would error out upon encountering it.

> +    ;; Python’s distutils.command.install defines this mapping from source to
> +    ;; destination mapping.
> +    (define install-schemes
> +      `(("scripts" "bin" ,move-script)
> +        ;; XXX: Why does Python not use share/ here?
> +        ("data" "share" ,move-data)))
> +
> +    (define (expand-data-directory directory)
> +      "Move files from all .data subdirectories to their respective
> +destinations."
> +      (for-each
> +        (match-lambda ((source destination function)
> +          (let ((source-path (string-append directory "/" source))
> +                (destination-path (string-append out "/" destination)))
> +            (when (file-exists? source-path)
> +              (begin
> +                ;; This assumes only files exist in the scripts/ directory.
> +                (for-each
> +                  (lambda (file)
> +                    (apply
> +                      function
> +                      (list
> +                        (string-append source-path "/" file)
> +                        (string-append destination-path "/" file))))
> +                  (scandir source-path (negate (cut member <> '("." "..")))))
> +                (rmdir source-path))))))
> +        install-schemes))
> +    
> +  (define pyproject-build (which "pyproject-build"))
> +
> +  (define (list-directories base predicate)
> +    ;; Cannot use find-files here, because it’s recursive.
> +    (scandir
> +      base
> +      (lambda (name)
> +        (let ((stat (lstat (string-append base "/" name))))
> +        (and
> +          (not (member name '("." "..")))
> +          (eq? (stat:type stat) 'directory)
> +          (predicate name stat))))))
> +
> +  (define (install-pep517)
> +    "Install a wheel generated by a PEP 517-compatible builder."
> +    (let ((wheels (find-files "dist" "\\.whl$"))) ; XXX: do not recurse

If we do not want to recurse, we should use scandir?

> +      (when (> (length wheels) 1) ; This code does not support multiple wheels
> +                                  ; yet, because their outputs would have to be
> +                                  ; merged properly.
> +        (raise (condition (&cannot-extract-multiple-wheels))))
> +      (for-each extract wheels))
> +    (let ((datadirs (map
> +					  (cut string-append site-dir "/" <>)
> +					  (list-directories site-dir (file-name-predicate "\\.data$")))))
> +      (for-each (lambda (directory)
> +                  (expand-data-directory directory)
> +                  (rmdir directory))
> +                datadirs)))
> +
> +    (define (install-setuptools)
> +      "Install using setuptools."
> +      (let ((out (assoc-ref outputs "out")))
> +        (invoke "python" "setup.py"
> +				"install"
> +				"--prefix" out
> +				"--single-version-externally-managed"
> +				"--root=/")))
> +
> +    (if pyproject-build
> +      (install-pep517)
> +      (install-setuptools))
> +    #t))

So, IIUC, this complicated install phase is because we no longer take
'pip' for granted and is only later available, built from this very
build system, right?  Otherwise installing a wheel with pip would be
trivial (c.f. python-isort).

> +
> +(define* (compile-bytecode #:key inputs outputs (configure-flags '()) #:allow-other-keys)
> +  "Compile installed byte-code in site-packages."
> +  (let ((site-dir (site-packages inputs outputs)))
> +    (invoke "python" "-m" "compileall" site-dir)
> +    ;; XXX: We could compile with -O and -OO too here, at the cost of more space.
> +    #t))

I think you can drop that comment; the default sounds reasonable:

  -o OPT_LEVELS         Optimization levels to run compilation with. Default is -1 which uses the
                        optimization level of the Python interpreter itself (see -O).

If we ever want to change we could globally change it for our Python.

I think we keep using "--invalidation-mode=unchecked-hash" though, for
performance [0]:

    Unchecked hash-based .pyc files are a useful performance optimization
    for environments where a system external to Python (e.g., the build
    system) is responsible for keeping .pyc files up-to-date.

[0] https://docs.python.org/3.7/whatsnew/3.7.html#pep-552-hash-based-pyc-files


[...]

It looks rather good to me, with the above comments!  I'll be looking
forward reviewing the rest of this series shortly.

Thank you!

Maxim

Hi Maxim,

> Here's a review of patches 1 to 6.

thanks for the review. Unfortunately this is not the most recent
proposal and I have no way to retract the previous patches. I pushed v3
to the wip-python-pep517 branch, because of the sheer number of patches
and so the CI could build it (since it requires a rebuild of the entire
rust bootstrap chain).

> > +        ;; Prefer pytest
> > +        ;; XXX: support nose
> 
> You can remove this; nose is stale/deprecated.

So it’s preferred to replace 'check in cases where python-nose is
still in use?

> 
> > +        (cond
> > +          (pytest
> > +            (begin
> > +              (format #t "using pytest~%")
> > +              (invoke pytest "-vv"))) ; XXX: support skipping tests based on name/extra arguments?
> 
> We could have a #:test-command argument to specify an arbitrary command
> as a list of strings, such as used by the emacs-build-system; that'd
> allow us to avoid overriding the phase just to add a '-k "not
> this-test"' or similar.

I added #:test-flags in my v3 proposal.

> > +          ;; But fall back to setup.py, which should work for most
> > +          ;; packages. XXX: would be nice not to depend on setup.py here? fails
> > +          ;; more often than not to find any tests at all. Maybe we can run
> > +          ;; `python -m unittest`?
> > +          (have-setup-py
> > +            (begin
> > +              (format #t "using setup.py~%")
> > +                (invoke "python" "setup.py" "test" "-v")))
> 
> As Marius noted, falling back to 'python setup.py test' is not
> desirable; it's scheduled to be removed already.

Sure, but using `python -m unittest` instead requires some investigation.

> > +    (define (move-script source destination)
> > +      "Move executable script file from .data/scripts to out/bin and replace
> > +temporary hashbang"
> > +	  (move-data source destination)
> > +      ;; ZIP does not save/restore permissions, make executable
> > +      ;; XXX: might not be a file, but directory with subdirectories
> > +      (chmod destination #o755)
> > +      (substitute* destination (("#!python") python-hashbang)))
> 
> It seems the directory case should be handled?  Otherwise the
> substitute* call would error out upon encountering it.

I have not seen anyone using subdirectories in bin/ yet. Is that
supported anywhere?

> So, IIUC, this complicated install phase is because we no longer take
> 'pip' for granted and is only later available, built from this very
> build system, right?  Otherwise installing a wheel with pip would be
> trivial (c.f. python-isort).

If we want to bootstrap these two packages easily (and possibly start
unvendoring their vendored dependencies later), they cannot be part of
this build system and thus we need to implement building/installing
ourselves. I tried using pypa-build in an earlier version, but the
bootstrap chain is unmaintainable.

There also is a project called installer[1], but it does not have a
CLI yet.

Cheers,
Lars

[1] https://github.com/pradyunsg/installer