From debbugs-submit-bounces@debbugs.gnu.org Sun Jan 23 00:29:51 2022 Received: (at 46848) by debbugs.gnu.org; 23 Jan 2022 05:29:51 +0000 Received: from localhost ([127.0.0.1]:38406 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1nBVRg-0004gU-Fm for submit@debbugs.gnu.org; Sun, 23 Jan 2022 00:29:51 -0500 Received: from mail-qv1-f47.google.com ([209.85.219.47]:38411) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1nBVRe-0004gD-43 for 46848@debbugs.gnu.org; Sun, 23 Jan 2022 00:29:43 -0500 Received: by mail-qv1-f47.google.com with SMTP id kl12so16103543qvb.5 for <46848@debbugs.gnu.org>; Sat, 22 Jan 2022 21:29:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version:content-transfer-encoding; bh=G3LXihQLy5xEL4n12cXs3iSYOkLRnIvip6p/JlZvfSk=; b=TRNT/lbJ0qDYYbAVTLadeub8TZxt4keeLt9BkcTJY3WasWFyWue3xkS+nxlN34s7NF HGk7SI1xP0US+rztVFZusNAvqsw7R441TwI8xikdxNNYl213Sth3/g9yZoK8becZOyYp Qp2oMQS7I1/BF0GSfd+sMvtBMbJXn4tQbE6Vdx6ROZvSNnoOox5r4nomg60icfgP09Jq fBg8jufhRWezW0H3NNZVGi1CZRfH7Q23FWgwiY5YFJN3jGKvKjGLTke87mjN7SGNRDd+ TUjA3f3vINzsxtL1qIS585PgrVfYMwcR9zVF6PsO6gKD8ANWRdwePK4oER2UIYb/Ok00 c09w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=G3LXihQLy5xEL4n12cXs3iSYOkLRnIvip6p/JlZvfSk=; b=kS8xFM+pER9TiT4A+haz3qsH3XkHBoA5RUHmbM37JsBoRgXCh1zGjOYVo5Iz/hrc9E cKegJ2c4S75R+otQ24px5RrVsbeFNSgSGyO8jtNCcG1CG9l/2BAAj7Z0tZ6gAE8cvis2 q6vL4XY4fhA7NVNCxIPIMb5gEoVmzhwM4yFou/tMNyJO3rRskxlEIWG6WL9epez6IgXd /C7BdRdnRgf/K3c6kWONxZUkdEfnc7pEkXl/Klt4hMp+qIuW+YwiYoPSn3JiHtKHX7Um hqW0y8QbNyaDluIqDYRU3k8XgtIp3J9D4rgxRxsblrWMb3md3AZnaCNBfuWHJBoMehku PTnw== X-Gm-Message-State: AOAM530JWV6rbQa/K3jchD3rmv8sQxQxlZcU9vBU7s72ZpCYxXBHEj0n RKgimFkR89/gusS4fAMwFADQSGspjYQ= X-Google-Smtp-Source: ABdhPJxSRvONPB86BEnfFDfiTAWIvBbG9SrsI5IXSvvwnZM+Qa6d84W17niOcM16HV4BmXNZg3wezw== X-Received: by 2002:a05:6214:1ccb:: with SMTP id g11mr10112553qvd.97.1642915776274; Sat, 22 Jan 2022 21:29:36 -0800 (PST) Received: from hurd (dsl-205-236-230-254.b2b2c.ca. [205.236.230.254]) by smtp.gmail.com with ESMTPSA id t20sm5570067qta.41.2022.01.22.21.29.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 22 Jan 2022 21:29:35 -0800 (PST) From: Maxim Cournoyer To: Lars-Dominik Braun Subject: Re: bug#46848: [PATCHES] [core-updates] PEP 517 python-build-system References: Date: Sun, 23 Jan 2022 00:29:34 -0500 In-Reply-To: (Lars-Dominik Braun's message of "Mon, 1 Mar 2021 14:43:22 +0100") Message-ID: <87ee4z6mv5.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 46848 Cc: 46848@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) Hi Lars, Here's a review of patches 1 to 6. Lars-Dominik Braun 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=E2=80=99s dependencies using setuptools (which is fortu= nately > 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 > 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=E2=80=99 sources to bootstrap t= hemselves. > + (add-before 'build 'set-PYTHONPATH ^ nitpick: GUIX_PYTHONPATH > + (lambda _ > + (format #t "current working dir ~s~%" (getcwd)) > + (setenv "GUIX_PYTHONPATH" > + (string-append ".:" (getenv "GUIX_PYTHONPA= TH"))) > + #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") > --=20 > 2.26.2 > > >>From 7a99aaa40e65fde58ee2e78ad7d3e0ccd6d169ae Mon Sep 17 00:00:00 2001 > From: Lars-Dominik Braun > 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) directo= ry)) > + %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 i= s 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=3Dinstall" ;install pip and setuptools > + "--with-ensurepip=3Dno" ;do not install pip and setupt= ools > "--enable-unicode=3Ducs4" >=20=20 > ;; 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))) >=20=20 > (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->se= xp > 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 dire= ctory > ;; and the scripts defined in entry-points will always be created. >=20=20 > +;; Base error type. > +(define-condition-type &python-build-error &error > + python-build-error?) >=20=20 > -(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__=3D'setup.py';" > - "f=3Dgetattr(tokenize, 'open', open)(__file__);" > - "code=3Df.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 par= ameters ~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-err= or > + cannot-extract-multiple-wheels?) >=20=20 > (define* (sanity-check #:key tests? inputs outputs #:allow-other-keys) > "Ensure packages depending on this package via setuptools work properl= y, > @@ -142,23 +130,51 @@ without errors." > (with-directory-excursion "/tmp" > (invoke "python" sanity-check.py (site-packages inputs outputs))))) >=20=20 > -(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 dire= ctory, > + ;; maybe secondary output =E2=80=9Cwheel=E2=80=9D? > + (mkdir-p "dist") > + (invoke pyproject-build "--outdir" "dist" "--no-isolation" "--wheel"= ".")) > + > + ;; XXX Would be nice, if we could use bdist_wheel here to remove e= xtra > + ;; 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) >=20=20 > -(define* (check #:key tests? test-target use-setuptools? #:allow-other-k= eys) > +(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 directo= ry in > - ;; build/lib in some cases, e.g. if the source is in a sub-directo= ry > - ;; (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=3D? after before))) > - (for-each delete-file-recursively inter))) > + ;; Unfortunately with PEP 517 there is no common method to specify t= est > + ;; systems. Guess test system based on inputs instead. > + (let ((pytest (which "pytest")) > + (have-setup-py (file-exists? "setup.py"))) ^ indentation =20=20=20=20=20=20=20=20=20=20=20=20=20 > + ;; 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 base= d 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) >=20=20 > @@ -195,31 +211,109 @@ running checks after installing the package." > "/bin:" > (getenv "PATH")))) >=20=20 > -(define* (install #:key inputs outputs (configure-flags '()) use-setupto= ols? > - #: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 (=3D major 3) (< minor 7)))))) > - (params (append (list (string-append "--prefix=3D" out) > - "--no-compile") > - (if use-setuptools? > - ;; distutils does not accept these flags > - (list "--single-version-externally-managed" > - "--root=3D/") > - '()) > - configure-flags))) > - (call-setuppy "install" params use-setuptools?) > - ;; Rather than produce potentially non-reproducible .pyc files on Py= thons > - ;; 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=3Dunchecke= d-hash" > - out)))) > +(define* (install #:key inputs outputs (configure-flags '()) #:allow-oth= er-keys) > + "Install a wheel file according to PEP 427" > + ;; See https://www.python.org/dev/peps/pep-0427/#installing-a-wheel-di= stribution-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=E2=80=99s zipfile to avoid extra dependency =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 > + (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 rep= lace > +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=E2=80=99s distutils.command.install defines this mapping f= rom 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/ directo= ry. > + (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)) > +=20=20=20=20 > + (define pyproject-build (which "pyproject-build")) > + > + (define (list-directories base predicate) > + ;; Cannot use find-files here, because it=E2=80=99s 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 hav= e 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=3D/"))) > + > + (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 mo= re space. > + #t)) I think you can drop that comment; the default sounds reasonable: -o OPT_LEVELS Optimization levels to run compilation with. Defaul= t 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=3Dunchecked-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-fi= les [...] 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