Re: [PATCH 0/1] Go importer

[ludovic.courtes@inria.fr (Ludovic Courtès)]

Hello!

Pierre-Antoine Rouby <pierre-antoine.rouby@inria.fr> skribis:

>> From: "Leo Famulari" <leo@famulari.name>
>> On Thu, Jul 19, 2018 at 08:56:03AM +0200, Pierre-Antoine Rouby wrote:
>>> I trying to import 'gitlab-runner' (https://gitlab.com/gitlab-org/gitlab-runner)
>>> with tag 'v10.6.0'.
>> 
>> Okay, thanks. I can reproduce the error:
>
> I have fix this issue (patch attached), let me know if that work for you.

Seems to me that this importer is almost ready.  Some comments:

> From 2f97b62beeacc58935a86d08a1635c082d078189 Mon Sep 17 00:00:00 2001
> From: Rouby Pierre-Antoine <pierre-antoine.rouby@inria.fr>
> Date: Wed, 25 Jul 2018 10:34:44 +0200
> Subject: [PATCH] import: Add gopkg importer.
>
> * guix/import/gopkg.scm: New file.
> * guix/scripts/import/gopkg.scm: New file.
> * guix/scripts/import.scm: Add 'gopkg'.
> * Makefile.am: Add 'gopkg' importer in modules list.

Please mention the guix.texi changes as well.

It would be good to have unit tests in tests/go.scm for the Toml parser
and/or for the whole importer, like we do in tests/gem.scm, for
instance.

> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -167,7 +167,7 @@ MODULES =					\
>    guix/build/rpath.scm				\
>    guix/build/cvs.scm				\
>    guix/build/svn.scm				\
> -  guix/build/syscalls.scm                       \
> +  guix/build/syscalls.scm			\

Please remove whitespace changes.

> +@item gopkg
> +@cindex gopkg
> +@cindex Golang
> +@cindex Go
> +Import metadata from the @uref{https://gopkg.in/, gopkg} package
> +versioning service used by some Go software.

A few words on limitations (if any ;-)), or mentioning that it’s
recursive by default (right?), and an example would be nice.

> +(define (vcs-file? file stat)

With “TODO: Factorize” please.  :-)

> +(define (file->hash-base32 file)
> +  "Return hash of FILE in nix base32 sha256 format.  If FILE is a directory,
> +exclude vcs files."
> +  (let-values (((port get-hash) (open-sha256-port)))
> +    (write-file file port #:select? (negate vcs-file?))

Shouldn’t it be #:select? vcs-file? ?

> +(define (git->hash url commit file)
> +  "Clone git repository and return FILE hash in nix base32 sha256 format."
> +  (if (not (file-exists? (string-append file "/.git")))
> +      (git-fetch url commit file #:recursive? #f))
> +  (file->hash-base32 file))
> +
> +(define (git-ref->commit path tag)
> +  "Return commit number coresponding to git TAG.  Return \"XXX\" if tag is not
> +found."
> +  (define (loop port)
> +    (let ((line (read-line port)))
> +      (cond
> +       ((eof-object? line)              ; EOF
> +        (begin
> +          (close-port port)
> +          "XXX"))
> +       ((string-match tag line)         ; Match tag
> +        (let ((commit (car (string-split (transform-string line #\tab " ")
> +                                         #\ ))))
> +          commit))
> +       (else                            ; Else
> +        (loop port)))))
> +
> +  (let ((file (if (file-exists? (string-append path "/.git/packed-refs"))
> +                  (string-append path "/.git/packed-refs")
> +                  (string-append path "/.git/FETCH_HEAD"))))
> +    (loop (open-input-file file))))
> +
> +(define* (git-fetch url commit directory
> +                    #:key (git-command "git") recursive?)
> +  "Fetch COMMIT from URL into DIRECTORY.  COMMIT must be a valid Git commit
> +identifier.  When RECURSIVE? is true, all the sub-modules of URL are fetched,
> +recursively.  Return #t on success, #f otherwise."
> +  (mkdir-p directory)
> +  
> +  (with-directory-excursion directory
> +    (invoke git-command "init")
> +    (invoke git-command "remote" "add" "origin" url)
> +    (if (zero? (system* git-command "fetch" "--depth" "1" "origin" commit))
> +        (invoke git-command "checkout" "FETCH_HEAD")
> +        (begin
> +          (invoke git-command "fetch" "origin")
> +          (if (not (zero? (system* git-command "checkout" commit)))
> +              (let ((commit-hash (git-ref->commit directory commit)))
> +                (invoke git-command "checkout" "master")
> +                (if (not (equal? "XXX" commit-hash)) ;HACK else stay on master
> +                    (zero? (system* git-command "checkout" commit-hash))))
> +              #t)))))

I would highly recommend Guile-Git for all this (or (guix git)) since
it’s already a hard dependency, but we can leave that for later.

> +(define (cut-url url)
> +  "Return URL without protocol prefix and git file extension."
> +  (string-replace-substring
> +   (cond
> +    ((string-match "http://"  url)
> +     (string-replace-substring url "http://" ""))
> +    ((string-match "https://" url)
> +     (string-replace-substring url "https://" ""))
> +    ((string-match "git://"   url)
> +     (string-replace-substring url "git://" ""))
> +    (else
> +     url))
> +   ".git" ""))

Use (uri-path (string->uri url)) to get the “path” part of the URL.  And
then perhaps:

  (if (string-suffix? ".git" path)
      (string-drop-right path 4)
      path)

Because ‘string-replace-substring’ would replace “.git” in the middle of
the URL as well.

> +(define (url->dn url)
> +  "Return the web site DN form url 'gnu.org/software/guix' --> 'gnu.org'"
> +  (car (string-split url #\/)))

(uri-host (string->uri url))

> +(define (url->git-url url)
> +  (string-append "https://" url ".git"))

(uri->string uri)

To me this suggests the code should manipulate URI objects internally
(info "(guile) URIs") rather than URLs as strings.

> +(define (comment? line)
> +  "Return #t if LINE start with comment delimiter, else return #f."
> +  (eq? (string-ref (string-trim line) 0) #\#))

(string-ref X 0) fails if X is the empty string.  So rather:

  (string-prefix? "#" (string-trim line))

> +(define (empty-line? line)
> +  "Return #t if LINE is empty, else #f."
> +  (string-null? (string-trim line)))

Should use ‘string-trim-both’, not ‘string-trim’.

> +(define (attribute? line attribute)
> +  "Return #t if LINE contain ATTRIBUTE."
> +  (equal? (string-trim-right
> +           (string-trim
> +            (car (string-split line #\=)))) attribute))

No car please.  :-)

  (match (string-split (string-trim-both line) #\=)
    ((key value)
     (string=? key attribute))
    (_
     #f))

> +(define (attribute-by-name line name)
> +  "Return attribute value corresponding to NAME."
> +  (let* ((line-no-attribut-name (string-replace-substring
> +                                 line
> +                                 (string-append name " = ") ""))
> +         (value-no-double-quote (string-replace-substring
> +                                 line-no-attribut-name
> +                                 "\"" "")))
> +    (string-trim value-no-double-quote)))

Use ‘match’ along the same lines as above (in fact it’s probably enough
to keep ‘attribute-by-name’ and get rid of ‘attribute?’).

> +(define (make-go-sexp->package packages dependencies
> +                               name url version revision
> +                               commit str-license home-page
> +                               git-url is-dep? hash)
> +  "Create Guix sexp package for Go software NAME. Return new package sexp."
> +  (define (package-inputs)
> +    (if (not is-dep?)
> +        `((native-inputs ,(list 'quasiquote dependencies)))
> +        '()))
> +
> +  (values
> +   `(define-public ,(string->symbol name)
> +      (let ((commit ,commit)
> +            (revision ,revision))
> +        (package
> +          (name ,name)
> +          (version (git-version ,version revision commit))
> +          (source (origin
> +                    (method git-fetch)
> +                    (uri (git-reference
> +                          (url ,git-url)
> +                          (commit commit)))
> +                    (file-name (git-file-name name version))
> +                    (sha256
> +                     (base32
> +                      ,hash))))
> +          (build-system go-build-system)
> +          (arguments
> +           '(#:import-path ,url))
> +          ,@(package-inputs)
> +          (home-page ,home-page)
> +          (synopsis "XXX")
> +          (description "XXX")
> +          (license #f))))))

No need for ‘values’ here.

> +(define (create-package->packages+dependencies packages dependencies
> +                                               url version directory
> +                                               revision commit
> +                                               constraint? is-dep?)
> +  "Return packages and dependencies with new package sexp corresponding to
> +URL."

Use keyword arguments here and try to explain the important parameters
in the docstring.

> +(define (parse-dependencies->packages+dependencies port constraint?
> +                                                   packages dependencies)

The name (same with the procedures above) looks weird.  I would call it
either ‘parse-XYZ’ or ‘XYZ->ABC’.

The rest LGTM!

Pierre-Antoine I know you’ll be leaving Inria and Guix-HPC shortly, so
if you don’t get around to it, hopefully one of us will do this final
pass of polishing.

Thank you!

Ludo’.