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

  • Open
  • quality assurance status badge
Details
3 participants
  • jgart
  • Lars-Dominik Braun
  • Maxim Cournoyer
Owner
unassigned
Submitted by
Maxim Cournoyer
Severity
normal
M
M
Maxim Cournoyer wrote on 28 Nov 09:05 +0100
(address . guix-patches@gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
cover.1732781055.git.maxim.cournoyer@gmail.com
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
M
M
Maxim Cournoyer wrote on 28 Nov 13:16 +0100
[PATCH python-team 1/4] build/pyproject: Really merge directories in install phase.
(address . 74582@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
35ca6b4d6fb04d511694f38e52e766134b8565dc.1732781055.git.maxim.cournoyer@gmail.com
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(-)

Toggle diff (22 lines)
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
M
M
Maxim Cournoyer wrote on 28 Nov 13:16 +0100
[PATCH python-team 3/4] build/pyproject: Update PEP 427 reference URL in comment.
(address . 74582@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
47f64576e3feaf679e1486a86e58417cb87f7d87.1732781055.git.maxim.cournoyer@gmail.com
* 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(-)

Toggle diff (18 lines)
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
M
M
Maxim Cournoyer wrote on 28 Nov 13:16 +0100
[PATCH python-team 2/4] build/pyproject: Fix indentation.
(address . 74582@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
90f63776c70ab18318067ceaab407f72b5093876.1732781055.git.maxim.cournoyer@gmail.com
* 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(-)

Toggle diff (92 lines)
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
M
M
Maxim Cournoyer wrote on 28 Nov 13:16 +0100
[PATCH python-team 4/4] build/pyproject: Resolve import warning.
(address . 74582@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
aebd13c4b0d7daed770c6e79a1b112f25e023a5d.1732781055.git.maxim.cournoyer@gmail.com
* 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(-)

Toggle diff (15 lines)
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
J
32738d465626faf1db388e920830030a7c743fe5@dismail.de
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: file
L
L
Lars-Dominik Braun wrote on 29 Nov 08:23 +0100
Re: [bug#74582] [PATCH python-team 1/4] build/pyproject: Really merge directories in install phase.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
Z0lsD5JDyxEAGlEr@noor.fritz.box
Hi,

Toggle quote (8 lines)
> + ;; 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
M
M
Maxim Cournoyer wrote on 14 Dec 16:12 +0100
(name . Lars-Dominik Braun)(address . lars@6xq.net)
87v7vm1eo5.fsf@gmail.com
Hi,

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

Toggle quote (14 lines)
> 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.

Toggle quote (3 lines)
> 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
L
L
Lars-Dominik Braun wrote on 15 Dec 17:25 +0100
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
Z18C-R2WaWqy9vSQ@noor.fritz.box
Hi,

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

sure, fine.

Toggle quote (4 lines)
> 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

build_wheel() should return the basename of the .whl file it
creates. That implies it can only be one.
M
M
Maxim Cournoyer wrote on 16 Dec 08:09 +0100
(name . Lars-Dominik Braun)(address . lars@6xq.net)
87ikrkumsn.fsf@gmail.com
Hi,

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

Toggle quote (14 lines)
> 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
M
M
Maxim Cournoyer wrote 7 days ago
(name . Lars-Dominik Braun)(address . lars@6xq.net)
87ttb1nrel.fsf@gmail.com
Hello,

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

[...]

Toggle quote (3 lines)
> 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
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 74582@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 74582
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch