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

[Ricardo Wurmus]

> 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.

>    (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.

> +                 (begin
> +                   (parse-requires.txt (first requires.txt-files)))

No need for “begin” here.

> +                 (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.

--
Ricardo