Re: [PATCH] import: pypi: detect requirements from requirements.txt.

[David Thompson]

Awesome! :)

How about this for the commit log:

"import: pypi: Detect inputs."

?

Cyril Roelandt <address@hidden> writes:

> * guix/import/pypi.scm (compute-inputs, guess-requirements): New functions.

"New procedures."

> ---
>  guix/import/pypi.scm | 146 
> ++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 115 insertions(+), 31 deletions(-)
>
> diff --git a/guix/import/pypi.scm b/guix/import/pypi.scm
> index 8567cad..9ac3342 100644
> --- a/guix/import/pypi.scm
> +++ b/guix/import/pypi.scm
> @@ -21,6 +21,7 @@
>    #:use-module (ice-9 match)
>    #:use-module (ice-9 pretty-print)
>    #:use-module (ice-9 regex)
> +  #:use-module ((ice-9 rdelim) #:select (read-line))
>    #:use-module (srfi srfi-1)
>    #:use-module (rnrs bytevectors)
>    #:use-module (json)
> @@ -77,42 +78,125 @@ or #f on failure."
>  with dashes."
>    (string-join (string-split (string-downcase str) #\_) "-"))
>  
> -(define (guix-hash-url url)
> -  "Download the resource at URL and return the hash in nix-base32 format."
> -  (call-with-temporary-output-file
> -   (lambda (temp port)
> -     (and (url-fetch url temp)
> -          (bytevector->nix-base32-string
> -           (call-with-input-file temp port-sha256))))))
> +(define (guix-hash-url filename)
> +  "Return the hash of FILENAME in nix-base32 format."
> +  (bytevector->nix-base32-string
> +   (call-with-input-file filename port-sha256)))
> +
> +(define (guix-name name)
> +  (if (string-prefix? "python-" name)
> +      (snake-case name)
> +      (string-append "python-" (snake-case name))))
> +
> +(define (maybe-inputs guix-name inputs)
> +  (match inputs
> +    (()
> +     '())
> +    ((inputs ...)
> +     (list (list guix-name
> +                 (list 'quasiquote inputs))))))

How about:

    `((,guix-name ,(list 'quasiquote inputs)))

Or:

    `((,guix-name (,'quasiquote ,inputs)))

> +
> +(define (guess-requirements source-url tarball)
> +  "Given SOURCE-URL and a TARBALL of the package, return a list of the 
> required
> +packages specified in the requirements.txt file."
> +
> +  (define (tarball-directory url)
> +    "Given the URL of the package's tarball, return the name of the directory
> +that will be created upon decompressing it."
> +    ;; TODO: Support more archive formats.
> +    (let ((basename (substring url (+ 1 (string-rindex url #\/)))))
> +    (cond
> +     ((string-suffix? ".tar.gz" basename)
> +      (string-drop-right basename 7))
> +     ((string-suffix? ".tar.bz2" basename)
> +      (string-drop-right basename 8))
> +     (else #f))))
> +
> +  (define (read-requirements requirements-file)
> +    "Given REQUIREMENTS-FILE, a Python requirements.txt file, return a list 
> of
> +name/variable pairs describing the requirements."
> +    (call-with-input-file requirements-file
> +      (lambda (port)
> +        (let loop ((result '()))
> +          (let ((line (read-line port)))
> +            (if (eof-object? line)
> +                result
> +                (loop (if (or (string-null? line)
> +                              (eq? (string-ref line 0) #\#))

For readability, consider extracting `(eq? (string-ref line 0) #\#)` to
an inner procedure named 'comment?'

> +                          result
> +                          (let ((input (guix-name (clean-requirement line))))
> +                            ;; Argparse has been part of Python since 2.7.
> +                            (if (string=? input "python-argparse")
> +                                result
> +                                (cons input result)))))))))))

There's a lot of nested 'if' forms here.  I think it could be
restructured into a more readable 'cond' by moving these conditionals
outside of the call to 'loop' and filtering out "python-argpase" as a
final step rather than doing it while you are still reading the file.

> +
> +  (define (clean-requirement s)
> +    "Given a requirement LINE, as can be found in a Python requirements.txt
> +file, remove everything other than the actual name of the required package, 
> and
> +return it."
> +    (string-take s
> +     (or (string-index s (lambda (c)
> +                         (member c '(#\< #\> #\= #\#))))
> +         (string-length s))))
> +
> +  (let* ((dirname (tarball-directory source-url))
> +         (req-file (if (string? dirname)
> +                       (string-append dirname "/requirements.txt")
> +                       #f)))

    (and (string? dirname) (string-append dirname "/requirements.txt"))
                       
> +    (if (and (string? req-file)

If 'dirname' was a string, then 'req-file' is certainly a string, so I
think you can just test dirname above and never go down this code path
if it isn't a string.

> +             ;; TODO: support more formats.
> +             (zero? (system* "tar" "xf" tarball req-file)))
> +        (dynamic-wind
> +          (const #t)
> +          (lambda ()
> +            (read-requirements req-file))
> +          (lambda ()
> +            (delete-file req-file)
> +            (rmdir dirname)))
> +        '())))
> +
> +(define (compute-inputs source-url tarball)
> +  "Given the SOURCE-URL of an already downloaded TARBALL, return a list of
> +name/variable pairs describing the required inputs of this package."
> +  (sort
> +    (map (lambda (input)
> +           (list input (list 'unquote (string->symbol input))))
> +         (append '("python-setuptools")
> +                 (guess-requirements source-url tarball)))
> +    (lambda args
> +      (match args
> +        (((a _ ...) (b _ ...))
> +         (string-ci<? a b))))))
>  
>  (define (make-pypi-sexp name version source-url home-page synopsis
>                          description license)
>    "Return the `package' s-expression for a python package with the given 
> NAME,
>  VERSION, SOURCE-URL, HOME-PAGE, SYNOPSIS, DESCRIPTION, and LICENSE."
> -  `(package
> -     (name ,(if (string-prefix? "python-" name)
> -                (snake-case name)
> -                (string-append "python-" (snake-case name))))
> -     (version ,version)
> -     (source (origin
> -               (method url-fetch)
> -               (uri (string-append ,@(factorize-uri source-url version)))
> -               (sha256
> -                (base32
> -                 ,(guix-hash-url source-url)))))
> -     (build-system python-build-system)
> -     (inputs
> -      `(("python-setuptools" ,python-setuptools)))
> -     (home-page ,home-page)
> -     (synopsis ,synopsis)
> -     (description ,description)
> -     (license ,(assoc-ref `((,lgpl2.0 . lgpl2.0)
> -                            (,gpl3 . gpl3)
> -                            (,bsd-3 . bsd-3)
> -                            (,expat . expat)
> -                            (,public-domain . public-domain)
> -                            (,asl2.0 . asl2.0))
> -                          license))))
> +  (call-with-temporary-output-file
> +   (lambda (temp port)
> +     (and (url-fetch source-url temp)
> +          `(package
> +             (name ,(guix-name name))
> +             (version ,version)
> +             (source (origin
> +                       (method url-fetch)
> +                       (uri (string-append ,@(factorize-uri source-url 
> version)))
> +                       (sha256
> +                        (base32
> +                         ,(guix-hash-url temp)))))
> +             (build-system python-build-system)
> +             ,@(maybe-inputs 'inputs
> +                (compute-inputs source-url temp))

Fix identation:    |------------^

> +             (home-page ,home-page)
> +             (synopsis ,synopsis)
> +             (description ,description)
> +             (license ,(assoc-ref `((,lgpl2.0 . lgpl2.0)
> +                                    (,gpl3 . gpl3)
> +                                    (,bsd-3 . bsd-3)
> +                                    (,expat . expat)
> +                                    (,public-domain . public-domain)
> +                                    (,asl2.0 . asl2.0))
> +                                  license)))))))
>  
>  (define (pypi->guix-package package-name)
>    "Fetch the metadata for PACKAGE-NAME from pypi.python.org, and return the
> -- 
> 1.8.4.rc3
>
>

Hope the above wasn't overwhelming.  Could you send an updated patch?
Thanks for working on this!

-- 
David Thompson
Web Developer - Free Software Foundation - http://fsf.org
GPG Key: 0FF1D807
Support the FSF: https://fsf.org/donate