[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 0/1] Go importer
From: |
Ludovic Courtès |
Subject: |
Re: [PATCH 0/1] Go importer |
Date: |
Thu, 26 Jul 2018 16:12:59 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Hello!
Pierre-Antoine Rouby <address@hidden> skribis:
>> From: "Leo Famulari" <address@hidden>
>> 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 <address@hidden>
> 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.
> address@hidden gopkg
> address@hidden gopkg
> address@hidden Golang
> address@hidden 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’.