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

[Maxim Cournoyer]

Hello Ricardo!

Ricardo Wurmus <address@hidden> writes:

>> From cfde6e09f8f8c692fe252d76ed27e8c50a9e5377 Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <address@hidden>
>> Date: Sat, 30 Mar 2019 23:13:26 -0400
>> Subject: [PATCH 8/9] import: pypi: Scan source archive to find requires.txt
>>  file.
>
>> * guix/import/pypi.scm (use-modules): Use invoke from (guix build utils).
>> (guess-requirements)[archive-root-directory]: Remove procedure.
>
> Oh, I guess I reviewed this procedure in vain :(
>
> Please modify the commits so that added procedures are not removed in
> later commits.  This is easier on the reviewer and makes for a clearer
> commit history.

Indeed; I'll be more careful about this is the future; sorry!

I've squashed this commit along with the one enabling more archive types
support, as this is where the modified (and later removed) procedure
originated.

>>    (define (guess-requirements-from-source)
>>      ;; Return the package's requirements by guessing them from the source.
>> -    (let ((dirname (archive-root-directory source-url))
>> -          (extension (file-extension source-url)))
>> -      (if (string? dirname)
>> -          (call-with-temporary-directory
>> -           (lambda (dir)
>> -             (let* ((pypi-name (string-take dirname (string-rindex dirname 
>> #\-)))
>> -                    (requires.txt (string-append dirname "/" pypi-name
>> -                                                 ".egg-info" 
>> "/requires.txt"))
>> -                    (exit-code
>> -                     (parameterize ((current-error-port (%make-void-port 
>> "rw+"))
>> -                                    (current-output-port (%make-void-port 
>> "rw+")))
>> -                       (if (string=? "zip" extension)
>> -                           (system* "unzip" archive "-d" dir requires.txt)
>> -                           (system* "tar" "xf" archive "-C" dir 
>> requires.txt)))))
>> -               (if (zero? exit-code)
>> -                   (parse-requires.txt (string-append dir "/" requires.txt))
>> -                   (begin
>> -                     (warning
>> -                      (G_ "Failed to extract file: ~a from source.~%")
>> -                      requires.txt)
>> -                     (list '() '()))))))
>> +    (if (compressed-file? source-url)
>> +        (call-with-temporary-directory
>> +         (lambda (dir)
>> +           (parameterize ((current-error-port (%make-void-port "rw+"))
>> +                          (current-output-port (%make-void-port "rw+")))
>> +             (if (string=? "zip" (file-extension source-url))
>> +                 (invoke "unzip" archive "-d" dir)
>> +                 (invoke "tar" "xf" archive "-C" dir)))
>> +           (let ((requires.txt-files
>> +                  (find-files dir (lambda (abs-file-name _)
>> +                                (string-match "\\.egg-info/requires.txt$"
>> +                                                  abs-file-name)))))
>> +             (if (> (length requires.txt-files) 0)
>
> Let’s work on the empty list directly.  Here “match” would be better.

Done, like this:

--8<---------------cut here---------------start------------->8---
-             (if (> (length requires.txt-files) 0)
-                 (parse-requires.txt (first requires.txt-files))
-                 (begin (warning (G_ "Cannot guess requirements from source 
archive:\
+             (match requires.txt-files
+               (()
+                (warning (G_ "Cannot guess requirements from source archive:\
  no requires.txt file found.~%"))
-                        '())))))
+                '())
+               (else (parse-requires.txt (first requires.txt-files)))))))
--8<---------------cut here---------------end--------------->8---

>> +                 (begin
>> +                   (parse-requires.txt (first requires.txt-files)))
>
> No need for “begin” here.

Fixed.

>> +                 (begin (warning (G_ "Cannot guess requirements from source 
>> archive:\
>> + no requires.txt file found.~%"))
>> +                        (list '() '()))))))
>
> I know that this is from an earlier commit, but I don’t like the look of
> “(list '() '())” at all :)
>
>> +        (begin
>> +          (warning (G_ "Unsupported archive format; \
>> +cannot determine package dependencies from source archive: ~a~%")
>> +                   (basename source-url))
>>            (list '() '()))))
>
> Same here.  Certainly there’s a better return value.

This might look ugly, but I can't think of a better return value, since
using anything else would mean having to introduce extra logic in the
callers, while it is now a correct value that needs no special case.

Maxim