[bug#44178] Add a Go Module Importer

["Ludovic Courtès" <ludo@gnu.org>]

Hi,

JOULAUD François <Francois.JOULAUD@radiofrance.com> skribis:

> This patch add a `guix import go` command.
>
> It was tested with several big repositories and mostly works. Several
> features are lacking (see TODO in source code) but we will do the
> improvments step-by-step in future patches.
>
> * doc/guix.texi: doc about go importer and guile-lib dependency
> * gnu/packages/package-management.scm: added guile-lib dependency
> * guix/self.scm: add guile-lib dependency
> * guix/build-system/go.scm: go-version->git-ref function
> * guix/import/go.scm: Created Go importer
> * guix/scripts/import/go.scm: Subcommand for Go importer
> * guix/scripts/import.scm: Declare subcommand guix import go
> * tests/import-go.scm: Tests for parse-go.mod procedure

Nitpick: please mention the sections (for documentation) or variables
changed (see
<https://guix.gnu.org/manual/en/html_node/Submitting-Patches.html>).

Some comments below, mostly stylistic as I’m not familiar with the
actual file formats etc. that the importer implements.

> +@item
> +@uref{https://www.nongnu.org/guile-lib/doc/ref/htmlprag/, guile-lib} for
> +the @code{crate} importer (@pxref{Invoking guix import}).

s/crate/go/
s/guile-lib/Guile-Lib/

> +         (re (string-concatenate
> +              (list
> +               "(v?[0-9]\\.[0-9]\\.[0-9])" ; "v" prefix can be omitted in version prefix
> +               "(-|-pre\\.0\\.|-0\\.)"     ; separator
> +               "([0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9])-" ; timestamp
> +               "([0-9A-Fa-f][0-9A-Fa-f][0-9A-Fa-f][0-9A-Fa-f][0-9A-Fa-f][0-9A-Fa-f][0-9A-Fa-f][0-9A-Fa-f][0-9A-Fa-f][0-9A-Fa-f][0-9A-Fa-f][0-9A-Fa-f])"))) ; commit hash

You can use ‘string-append’ instead of (string-concatenate (list …)).
Use [[:xdigit:]] instead of [0-9A-Fa-f] for clarity and
locale-independence.

Also, you can arrange to use ‘make-regexp’ so that the regexp is
compiled once for all, and then just ‘regexp-exec’:

  (define %go-version-rx (make-regexp …))

  (define (go-version->git-ref version)
    (… (regexp-exec %go-version-rx …) …))

It’s not critical though.

> +  (define (replace-directive results line)
> +    "Extract replaced modules and new requirements from replace directive
> +    in LINE and add to RESULTS."
> +    ;; ReplaceSpec = ModulePath [ Version ] "=>" FilePath newline
> +    ;;             | ModulePath [ Version ] "=>" ModulePath Version newline .
> +    (let* ((requirements (car results))
> +           (replaced (cdr results))

Please use ‘match’ instead of car/cdr (throughout):

  https://guix.gnu.org/manual/en/html_node/Coding-Style.html

> +           (re (string-concatenate
> +                '("([^[:blank:]]+)([[:blank:]]+([^[:blank:]]+))?"
> +                  "[[:blank:]]+" "=>" "[[:blank:]]+"
> +                  "([^[:blank:]]+)([[:blank:]]+([^[:blank:]]+))?")))
> +           (match (string-match re line))

As above, you should arrange to pre-compile the regexp.

> +           (module-path (match:substring match 1))
> +           (version (match:substring match 3))
> +           (new-module-path (match:substring match 4))
> +           (new-version (match:substring match 6))
> +           (new-replaced (acons module-path version replaced))

s/acons/alist-cons/ for consistency with the rest of the code.

> +           (re (string-concatenate
> +                '("^[[:blank:]]*"
> +                  "([^[:blank:]]+)[[:blank:]]+([^[:blank:]]+)"
> +                  "([[:blank:]]+//.*)?")))
> +           (match (string-match re line))

Same as above.

> +  ;; TODO: handle module path with VCS qualifier as described in
> +  ;; https://golang.org/ref/mod#vcs-find and
> +  ;; https://golang.org/cmd/go/#hdr-Remote_import_paths
> +  (define-record-type <vcs>
> +    (make-vcs url-prefix root-regex type)
> +    vcs?
> +    (url-prefix vcs-url-prefix)
> +    (root-regex vcs-root-regex)
> +    (type vcs-type))

You could rename ‘make-vcs’ above to ‘%make-vcs’ and do:

  (define (make-vcs prefix regexp type)
    (%make-vcs prefix (make-regex regexp) type))

so that again you can rely on pre-compiled regexps.

> +  (let* ((known-vcs
> +          (list
> +           (make-vcs
> +            "github.com"
> +            "^(github\\.com/[A-Za-z0-9_.\\-]+/[A-Za-z0-9_.\\-]+)(/[A-Za-z0-9_.\\-]+)*$"
> +            'git)
> +           (make-vcs
> +            "bitbucket.org"
> +            "^(bitbucket\\.org/([A-Za-z0-9_.\\-]+/[A-Za-z0-9_.\\-]+))(/[A-Za-z0-9_.\\-]+)*$"
> +            'unknown)
> +           (make-vcs
> +            "hub.jazz.net/git/"
> +            "^(hub\\.jazz\\.net/git/[a-z0-9]+/[A-Za-z0-9_.\\-]+)(/[A-Za-z0-9_.\\-]+)*$"
> +            'git)
> +           (make-vcs
> +            "git.apache.org"
> +            "^(git\\.apache\\.org/[a-z0-9_.\\-]+\\.git)(/[A-Za-z0-9_.\\-]+)*$"
> +            'git)
> +           (make-vcs
> +            "git.openstack.org"
> +            "^(git\\.openstack\\.org/[A-Za-z0-9_.\\-]+/[A-Za-z0-9_.\\-]+)(\\.git)?(/[A-Za-z0-9_.\\-]+)*$"
> +            'git)))
> +         (vcs (find (lambda (vcs) (string-prefix? (vcs-url-prefix vcs) module-path))
> +                    known-vcs)))

Keep ‘known-vcs’ in a global variable so it doesn’t have to be
recomputed every time.

> +        `(package
> +           (name ,guix-name)
> +           ;; Elide the "v" prefix Go uses
> +           (version ,(string-trim latest-version #\v))
> +           (source
> +            ,(vcs->origin vcs-type vcs-repo-url latest-version temp))
> +           (build-system go-build-system)
> +           (arguments
> +            '(#:import-path ,root-module-path))
> +           ,@(maybe-inputs (map go-module->guix-package-name dependencies))
> +           ;; TODO(katco): It would be nice to make an effort to fetch this
> +           ;; from known forges, e.g. GitHub
> +           (home-page ,(format #f "https://~a" root-module-path))
> +           (synopsis "A Go package")
                         ^
‘guix lint’ wouldn’t like it. :-)  Maybe "Write synopsis here" instead?

> +           (description ,(format #f "~a is a Go package." guix-name))
> +           (license #f))

Is there no info about the license?

> +++ b/guix/self.scm
> @@ -814,6 +814,9 @@ itself."
>    (define guile-ssh
>      (specification->package "guile-ssh"))
>  
> +  (define guile-lib
> +    (specification->package "guile-lib"))
> +
>    (define guile-git
>      (specification->package "guile-git"))
>  
> @@ -842,7 +845,7 @@ itself."
>      (append-map transitive-package-dependencies
>                  (list guile-gcrypt gnutls guile-git guile-avahi
>                        guile-json guile-semver guile-ssh guile-sqlite3
> -                      guile-zlib guile-lzlib guile-zstd)))
> +                      guile-lib guile-zlib guile-lzlib guile-zstd)))

New dependency; it’s a bit of a commitment, but hopefully Guile-Lib is
stable enough and works on all the supported architectures.

Please add guix/scripts/import/go.scm to ‘po/guix/POTFILES.in’ so it can
be translated.

> +++ b/tests/import-go.scm

Looks nice!  It should be called ‘tests/go.scm’ for consistency, with:

  (test-begin "go")
  …
  (test-end "go")

Would it be an option to also have an end-to-end test (checking the
resulting ‘package’ sexp)?  That’d be nice, but perhaps we can add it
afterwards if you prefer.

Let’s see how much of the comments above you can address for a v4, and
then we can get that in and improve it from there if needed!

Thanks again,
Ludo’.