bug#24450: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case.

[Maxim Cournoyer]

Hello!

Ricardo Wurmus <address@hidden> writes:

> Patch number 6:
>
>> From fb0547ef225103c0f8355a7eccc41e0d028f6563 Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <address@hidden>
>> Date: Thu, 28 Mar 2019 00:26:03 -0400
>> Subject: [PATCH 6/9] import: pypi: Parse wheel METADATA instead of
>>  metadata.json.
>
>> With newer Wheel releases, there is no more metadata.json file; the METADATA
>> file should be used instead (see: https://github.com/pypa/wheel/issues/195).
>
>> This change updates our PyPI importer so that it uses the later.
>
> Typo: should be “latter” instead of “later”.

Fixed.

>> * guix/import/pypi.scm (define-module): Remove unnecessary modules and export
>>   the PARSE-WHEEL-METADATA method.
>
> Please remove the indentation here.  Also, please don’t use “method”
> (because it’s not); use “procedure” instead.

Done. Thanks for fixing my terminology :-).

>> (parse-wheel-metadata): Add method.
>
> Same here.

Done.

>> +  (define (requires-dist-header? line)
>> +    ;; Return #t if the given LINE is a Requires-Dist header.
>> +    (regexp-match? (string-match "^Requires-Dist: " line)))
>> +
>> +  (define (requires-dist-value line)
>> +    (string-drop line (string-length "Requires-Dist: ")))
>> +
>> +  (define (extra? line)
>> +    ;; Return #t if the given LINE is an "extra" requirement.
>> +    (regexp-match? (string-match "extra == " line)))
>
> The use of “regexp-match?” here isn’t strictly necessary as the return
> value is true-ish anyway.

Done.

>> +  (call-with-input-file metadata
>> +    (lambda (port)
>> +      (let loop ((requirements '()))
>> +        (let ((line (read-line port)))
>> +          ;; Stop at the first 'Provides-Extra' section: the non-optional
>> +          ;; requirements appear before the optional ones.
>> +          (if (eof-object? line)
>> +              (reverse (delete-duplicates requirements))
>> +              (cond
>> +               ((and (requires-dist-header? line) (not (extra? line)))
>> +                (loop (cons (specification->requirement-name
>> +                             (requires-dist-value line))
>> +                            requirements)))
>> +               (else
>> +                (loop requirements)))))))))
>> +
>
> As before you can simplify the nested let and merge “if” and "cond“.

Oh, I get it now, I think:

--8<---------------cut here---------------start------------->8---
 
   (call-with-input-file metadata
     (lambda (port)
       (let loop ((requirements '()))
-        (let ((line (read-line port)))
-          ;; Stop at the first 'Provides-Extra' section: the non-optional
-          ;; requirements appear before the optional ones.
-          (if (eof-object? line)
-              (reverse (delete-duplicates requirements))
-              (cond
-               ((and (requires-dist-header? line) (not (extra? line)))
-                (loop (cons (specification->requirement-name
-                             (requires-dist-value line))
-                            requirements)))
-               (else
-                (loop requirements)))))))))
+        (match (read-line port)
+          (line
+           ;; Stop at the first 'Provides-Extra' section: the non-optional
+           ;; requirements appear before the optional ones.
+           (cond
+            ((eof-object? line)
+             (reverse (delete-duplicates requirements)))
+            ((and (requires-dist-header? line) (not (extra? line)))
+             (loop (cons (specification->requirement-name
+                          (requires-dist-value line))
+                         requirements)))
+            (else
+             (loop requirements)))))))))
 
 (define (guess-requirements source-url wheel-url archive)
   "Given SOURCE-URL, WHEEL-URL and a ARCHIVE of the package, return a list
--8<---------------cut here---------------end--------------->8---

>>    (define (read-wheel-metadata wheel-archive)
>>      ;; Given WHEEL-ARCHIVE, a ZIP Python wheel archive, return the package's
>> -    ;; requirements.
>> +    ;; requirements, or #f if the metadata file contained therein couldn't 
>> be
>> +    ;; extracted.
>>      (let* ((dirname (wheel-url->extracted-directory wheel-url))
>> -           (json-file (string-append dirname "/metadata.json")))
>> -      (and (zero? (system* "unzip" "-q" wheel-archive json-file))
>> -           (dynamic-wind
>> -             (const #t)
>> -             (lambda ()
>> -               (call-with-input-file json-file
>> -                 (lambda (port)
>> -                   (let* ((metadata (json->scm port))
>> -                          (run_requires (hash-ref metadata "run_requires"))
>> -                          (requirements (if run_requires
>> -                                            (hash-ref (list-ref 
>> run_requires 0)
>> -                                                       "requires")
>> -                                            '())))
>> -                     (map specification->requirement-name requirements)))))
>> -             (lambda ()
>> -               (delete-file json-file)
>> -               (rmdir dirname))))))
>> +           (metadata (string-append dirname "/METADATA")))
>> +      (call-with-temporary-directory
>> +       (lambda (dir)
>> +         (if (zero? (system* "unzip" "-q" wheel-archive "-d" dir metadata))
>> +             (parse-wheel-metadata (string-append dir "/" metadata))
>> +             (begin
>> +               (warning
>> +                (G_ "Failed to extract file: ~a from wheel.~%") metadata)
>> +               #f))))))
>
> The old approach took care of removing the unpacked archive no matter
> what happened.  The new code doesn’t do that.

The temporary directory where the archive is unpacked should be cleared
when leaving upon leaving its scope; the docstring of
"call-with-temporary-directory" says:

    "Call PROC with a name of a temporary directory; close the directory and
    delete it when leaving the dynamic extent of this call."

>> --- a/tests/pypi.scm
>> +++ b/tests/pypi.scm
>
> Thanks for the tests!

:-)

Maxim