[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#46848] [PATCHES] [core-updates] PEP 517 python-build-system
|
From: |
Maxim Cournoyer |
|
Subject: |
[bug#46848] [PATCHES] [core-updates] PEP 517 python-build-system |
|
Date: |
Sun, 23 Jan 2022 00:29:34 -0500 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
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