unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
From: "Ludovic Courtès" <ludo@gnu.org>
To: "JOULAUD François" <Francois.JOULAUD@radiofrance.com>
Cc: "44178@debbugs.gnu.org" <44178@debbugs.gnu.org>,
	Katherine Cox-Buday <cox.katherine.e@gmail.com>
Subject: [bug#44178] Add a Go Module Importer
Date: Tue, 02 Mar 2021 22:54:49 +0100	[thread overview]
Message-ID: <871rcxte52.fsf_-_@gnu.org> (raw)
In-Reply-To: <20210219161737.4l266imcd24gqxwn@fjo-extia-HPdeb.example.avalenn.eu> ("JOULAUD François"'s message of "Fri, 19 Feb 2021 16:21:03 +0000")

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




  reply	other threads:[~2021-03-02 21:56 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-23 14:06 [bug#44178] Add a Go Module Importer Katherine Cox-Buday
2020-10-28 10:41 ` Ludovic Courtès
2020-10-28 10:42 ` Ludovic Courtès
2020-11-10 20:26 ` Marius Bakke
     [not found]   ` <CANe01w55ZO=_9v0HcDv248UsoLUXb_9WVAgM4LqiZ4E-r1XgXg@mail.gmail.com>
2020-11-11  1:23     ` Helio Machado
2021-01-23 21:35       ` [bug#44178] [PATCH] Create importer for Go modules guix-patches--- via
2021-01-23 22:41         ` Katherine Cox-Buday
2021-01-25 21:03           ` guix-patches--- via
2021-01-27 14:38             ` Katherine Cox-Buday
2021-01-28 13:27               ` Ludovic Courtès
2021-01-29 16:43                 ` guix-patches--- via
2021-01-29 16:52                   ` [bug#44178] [PATCHv2] " guix-patches--- via
2021-01-31 16:23                   ` [bug#44178] [PATCH] " Ludovic Courtès
2021-02-19 15:51                     ` JOULAUD François via Guix-patches via
2021-02-19 16:21                       ` [bug#44178] [PATCHv3] " JOULAUD François via Guix-patches via
2021-03-02 21:54                         ` Ludovic Courtès [this message]
2021-03-04  5:40                           ` [bug#44178] [PATCH v4] Re: bug#44178: Add a Go Module Importer Maxim Cournoyer
2021-03-04 14:14                             ` JOULAUD François via Guix-patches via
2021-03-04 15:47                               ` Maxim Cournoyer
2021-03-08 13:54                           ` [bug#44178] " JOULAUD François via Guix-patches via
2021-03-10 17:12                             ` bug#44178: " Ludovic Courtès
2021-01-28  5:01             ` [bug#44178] [PATCH] Create importer for Go modules Timmy Douglas
2020-11-11 20:48   ` [bug#44178] Add a Go Module Importer Katherine Cox-Buday
2020-12-09 14:22 ` [bug#44178] dftxbs3e
2020-12-10  2:42   ` [bug#44178] dftxbs3e
2020-12-10  3:14     ` [bug#44178] dftxbs3e
2021-01-28  7:29 ` [bug#44178] [PATCH] Create importer for Go modules guix-patches--- via

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://guix.gnu.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=871rcxte52.fsf_-_@gnu.org \
    --to=ludo@gnu.org \
    --cc=44178@debbugs.gnu.org \
    --cc=Francois.JOULAUD@radiofrance.com \
    --cc=cox.katherine.e@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).