[bug#74582] [PATCH python-team 0/4] Fix edge case in pyproject-build-system

[bug#74582] [PATCH python-team 0/4] Fix edge case in pyproject-build-system

[Maxim Cournoyer]

Maxim Cournoyer (4):
  build/pyproject: Really merge directories in install phase.
  build/pyproject: Fix indentation.
  build/pyproject: Update PEP 427 reference URL in comment.
  build/pyproject: Resolve import warning.

 guix/build/pyproject-build-system.scm | 52 +++++++++++++++------------
 1 file changed, 29 insertions(+), 23 deletions(-)


base-commit: dd4b96e72c8fda4b025a75b47212e06e381e9ea1
-- 
2.46.0

[bug#74582] [PATCH python-team 1/4] build/pyproject: Really merge directories in install phase.

[Maxim Cournoyer]

Using rename-file, the destination had to be empty otherwise it would error
out.  By using copy-recursively, a directory can be copied onto a pre-existing
directory, really merging them.  This problem manifested itself attempting to
build the python-pyre package.

* guix/build/pyproject-build-system.scm (install)
<merge-directories>: Use copy-recursively instead of rename-file.

Change-Id: Iceb8609a86f29b17e5fbe6a9629339d0bc26e11f
---
 guix/build/pyproject-build-system.scm | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/guix/build/pyproject-build-system.scm b/guix/build/pyproject-build-system.scm
index c69ccc9d64..03992d915f 100644
--- a/guix/build/pyproject-build-system.scm
+++ b/guix/build/pyproject-build-system.scm
@@ -194,8 +194,13 @@ (define* (install #:key inputs outputs #:allow-other-keys)
                   (format #t "~a/~a -> ~a/~a~%"
                           source file destination file)
                   (mkdir-p destination)
-                  (rename-file (string-append source "/" file)
-                               (string-append destination "/" file))
+                  ;; Use 'copy-recursively' rather than 'rename-file' to guard
+                  ;; against the odd case where DESTINATION is a non-empty
+                  ;; directory, which may happen when using hybrid Python
+                  ;; build systems.
+                  (copy-recursively (string-append source "/" file)
+                                    (string-append destination "/" file))
+                  (delete-file-recursively (string-append source "/" file))
                   (when post-move
                     (post-move file)))
                 (scandir source
-- 
2.46.0

[bug#74582] [PATCH python-team 1/4] build/pyproject: Really merge directories in install phase.

[Lars-Dominik Braun]

Hi,

> +                  ;; Use 'copy-recursively' rather than 'rename-file' to guard
> +                  ;; against the odd case where DESTINATION is a non-empty
> +                  ;; directory, which may happen when using hybrid Python
> +                  ;; build systems.
> +                  (copy-recursively (string-append source "/" file)
> +                                    (string-append destination "/" file))
> +                  (delete-file-recursively (string-append source "/" file))

wouldn’t it be easier to remove this function entirely and move the
shebang-replacement via POST-MOVE into a separate function (perhaps
powered by FIND-FILES instead of SCANDIR)?

I believe with this patch we can also remove &cannot-extract-multiple-wheels
further down, since directories should be merged now, right?

Cheers,
Lars

[bug#74582] [PATCH python-team 1/4] build/pyproject: Really merge directories in install phase.

[Maxim Cournoyer]

Hi,

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

> Hi,
>
>> +                  ;; Use 'copy-recursively' rather than 'rename-file' to guard
>> +                  ;; against the odd case where DESTINATION is a non-empty
>> +                  ;; directory, which may happen when using hybrid Python
>> +                  ;; build systems.
>> +                  (copy-recursively (string-append source "/" file)
>> +                                    (string-append destination "/" file))
>> +                  (delete-file-recursively (string-append source "/" file))
>
> wouldn’t it be easier to remove this function entirely and move the
> shebang-replacement via POST-MOVE into a separate function (perhaps
> powered by FIND-FILES instead of SCANDIR)?

Yes, that could be nicer.  I'd like to keep it for a distinct commti
though, to keep this small and focus.

> I believe with this patch we can also remove &cannot-extract-multiple-wheels
> further down, since directories should be merged now, right?

Perhaps, though we'd want to verify that it indeed now works, and not
having seen that error once, I'm not too sure how to test it.  Do you
know of a package that could make use of this?

-- 
Thanks,
Maxim

[bug#74582] [PATCH python-team 1/4] build/pyproject: Really merge directories in install phase.

[Lars-Dominik Braun]

Hi,

> Yes, that could be nicer.  I'd like to keep it for a distinct commti
> though, to keep this small and focus.

sure, fine.

> Perhaps, though we'd want to verify that it indeed now works, and not
> having seen that error once, I'm not too sure how to test it.  Do you
> know of a package that could make use of this?

Hm, I checked and it does not seem to be possible to build more than
one wheel with a single pyproject.toml[1]. Let’s keep the code as is then.

Lars

[1] https://peps.python.org/pep-0517/#build-wheel states that
    build_wheel() should return the basename of the .whl file it
    creates. That implies it can only be one.

[bug#74582] [PATCH python-team 1/4] build/pyproject: Really merge directories in install phase.

[Maxim Cournoyer]

Hi,

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

> Hi,
>
>> Yes, that could be nicer.  I'd like to keep it for a distinct commti
>> though, to keep this small and focus.
>
> sure, fine.
>
>> Perhaps, though we'd want to verify that it indeed now works, and not
>> having seen that error once, I'm not too sure how to test it.  Do you
>> know of a package that could make use of this?
>
> Hm, I checked and it does not seem to be possible to build more than
> one wheel with a single pyproject.toml[1]. Let’s keep the code as is then.

Yes.  I'd expect perhaps some kind of mono-repo holding multiple Python
packages could perhaps end up in such a situation where it'd have
mulitple .whl installed to the same prefix.

The odd case I had encountered where merging directories was failing was
an attempt to build a scikit package (I think it was python-libcst) that
used CMake for the extensions, installed that already, then did a
regular PEP 517 build and attempted to install more things to its
prefix, which already had things placed there by scikit/cmake, IIUC.

I ended up packaging libcst via cargo instead of scikit, since it's
authored in Rust.

-- 
Thanks,
Maxim

[bug#74582] [PATCH python-team 1/4] build/pyproject: Really merge directories in install phase.

[Maxim Cournoyer]

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

[...]

> The odd case I had encountered where merging directories was failing was
> an attempt to build a scikit package (I think it was python-libcst)

Correction: it was python-pyre, see: bug#74581 for a package definition
reproducing it.

-- 
Thanks,
Maxim

[bug#74582] [PATCH python-team 2/4] build/pyproject: Fix indentation.

[Maxim Cournoyer]

* guix/build/pyproject-build-system.scm: Re-indent file with Emacs.

Change-Id: I15c89628190b81a71f799e4812c3b6a360f93bcb
---
 guix/build/pyproject-build-system.scm | 36 +++++++++++++--------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/guix/build/pyproject-build-system.scm b/guix/build/pyproject-build-system.scm
index 03992d915f..d42577b259 100644
--- a/guix/build/pyproject-build-system.scm
+++ b/guix/build/pyproject-build-system.scm
@@ -77,11 +77,11 @@ (define-condition-type &python-build-error &error python-build-error?)
 
 ;; 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?)
+  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?)
+  cannot-extract-multiple-wheels?)
 
 ;; Raised, when no wheel has been built by the build system.
 (define-condition-type &no-wheels-built &python-build-error no-wheels-built?)
@@ -93,8 +93,7 @@ (define* (build #:key outputs build-backend configure-flags #:allow-other-keys)
     "Look up the build backend in a pyproject.toml file."
     (call-with-input-file file
       (lambda (in)
-        (let loop
-          ((line (read-line in 'concat)))
+        (let loop ((line (read-line in 'concat)))
           (if (eof-object? line) #f
               (let ((m (string-match "build-backend = [\"'](.+)[\"']" line)))
                 (if m
@@ -122,18 +121,18 @@ (define* (build #:key outputs build-backend configure-flags #:allow-other-keys)
                                 auto-build-backend
                                 "setuptools.build_meta")))
     (format #t
-     "Using '~a' to build wheels, auto-detected '~a', override '~a'.~%"
-     use-build-backend auto-build-backend build-backend)
+            "Using '~a' to build wheels, auto-detected '~a', override '~a'.~%"
+            use-build-backend auto-build-backend build-backend)
     (mkdir-p wheel-dir)
     ;; Call the PEP 517 build function, which drops a .whl into wheel-dir.
     (invoke "python" "-c"
-     "import sys, importlib, json
+            "import sys, importlib, json
 config_settings = json.loads (sys.argv[3])
 builder = importlib.import_module(sys.argv[1])
 builder.build_wheel(sys.argv[2], config_settings=config_settings)"
-     use-build-backend
-     wheel-dir
-     config-settings)))
+            use-build-backend
+            wheel-dir
+            config-settings)))
 
 (define* (check #:key tests? test-backend test-flags #:allow-other-keys)
   "Run the test suite of a given Python package."
@@ -253,19 +252,20 @@ (define* (install #:key inputs outputs #:allow-other-keys)
                         (scandir wheel-dir
                                  (cut string-suffix? ".whl" <>)))))
       (cond
-        ((> (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))))
-        ((= (length wheels) 0)
-         (raise (condition (&no-wheels-built)))))
+       ((> (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))))
+       ((= (length wheels) 0)
+        (raise (condition (&no-wheels-built)))))
       (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))))
+                  (rmdir directory))
+                datadirs))))
 
 (define* (compile-bytecode #:key inputs outputs #:allow-other-keys)
   "Compile installed byte-code in site-packages."
@@ -341,7 +341,7 @@ (define* (create-entrypoints #:key inputs outputs #:allow-other-keys)
 import sys
 import ~a as mod
 sys.exit (mod.~a ())~%" interpreter module function)))
-        (chmod file-path #o755)))
+      (chmod file-path #o755)))
 
   (let* ((site-dir (site-packages inputs outputs))
          (out (assoc-ref outputs "out"))
-- 
2.46.0

[bug#74582] [PATCH python-team 3/4] build/pyproject: Update PEP 427 reference URL in comment.

[Maxim Cournoyer]

* guix/build/pyproject-build-system.scm (install): Update reference URL.

Change-Id: Icf5dcc7254c33e8e466773ee66a2fd5648d583da
---
 guix/build/pyproject-build-system.scm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/guix/build/pyproject-build-system.scm b/guix/build/pyproject-build-system.scm
index d42577b259..314839c30f 100644
--- a/guix/build/pyproject-build-system.scm
+++ b/guix/build/pyproject-build-system.scm
@@ -172,8 +172,9 @@ (define* (check #:key tests? test-backend test-flags #:allow-other-keys)
       (format #t "test suite not run~%")))
 
 (define* (install #:key inputs outputs #: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
+  "Install a wheel file according to PEP 427."
+  ;; See <https://packaging.python.org/en/latest/specifications/\
+  ;; binary-distribution-format/#binary-distribution-format>.
   (let ((site-dir (site-packages inputs outputs))
         (python (assoc-ref inputs "python"))
         (out (assoc-ref outputs "out")))
-- 
2.46.0

[bug#74582] [PATCH python-team 4/4] build/pyproject: Resolve import warning.

[Maxim Cournoyer]

* guix/build/pyproject-build-system.scm: Hide the 'delete' symbol from
the imported (guix build utils) module to avoid a naming clash warning.

Change-Id: I7db9500b20c71c89e740c18c089f33c8569c4ffd
---
 guix/build/pyproject-build-system.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/guix/build/pyproject-build-system.scm b/guix/build/pyproject-build-system.scm
index 314839c30f..beca4dc8ca 100644
--- a/guix/build/pyproject-build-system.scm
+++ b/guix/build/pyproject-build-system.scm
@@ -19,7 +19,7 @@
 
 (define-module (guix build pyproject-build-system)
   #:use-module ((guix build python-build-system) #:prefix python:)
-  #:use-module (guix build utils)
+  #:use-module ((guix build utils) #:hide (delete))
   #:use-module (guix build json)
   #:use-module (ice-9 match)
   #:use-module (ice-9 ftw)
-- 
2.46.0

[bug#74582] [PATCH python-team 4/4] build/pyproject: Resolve import warning.

[jgart via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 147 bytes --]

Thanks!

Would anyone else like to review this? I won't be able to test and review it for up to a week.

LGTM, otherwise.

all best,

jgart

[-- Attachment #2: Type: text/html, Size: 376 bytes --]