Package: guix-patches;
Reported by: Lars-Dominik Braun <lars <at> 6xq.net>
Date: Mon, 1 Mar 2021 13:45:01 UTC
Severity: normal
Tags: patch
Done: Lars-Dominik Braun <lars <at> 6xq.net>
Bug is archived. No further changes may be made.
Message #50 received at 46848 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Lars-Dominik Braun <lars <at> 6xq.net> Cc: 46848 <at> debbugs.gnu.org Subject: Re: bug#46848: [PATCHES] [core-updates] PEP 517 python-build-system Date: Sun, 23 Jan 2022 00:29:34 -0500
Hi Lars,
Here's a review of patches 1 to 6.
Lars-Dominik Braun <lars <at> 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 <at> 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 <at> 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.