[bug#38408] [PATCH v9 3/8] Added Guile-Semver as a dependency to guix

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

Hi Martin & all,

I apologize for taking so long and dropping the ball.  Partly that’s
because this is non-trivial, thanks for working on it, Martin!

Some quick comments:

Martin Becze <mjbecze@riseup.net> skribis:

> From 494f7c874781f64b702e31841c95c95c68fb28fc Mon Sep 17 00:00:00 2001
> From: Martin Becze <mjbecze@riseup.net>
> Date: Fri, 21 Feb 2020 10:41:44 -0500
> Subject: [PATCH v12 8/8] guix: self: Adds guile-semver as a depenedency.
>
> * guix/self.scm (compiled-guix) Added guile-semver as a depenedency.

Good.

> From 492db2aed32bb68e50eb43660d5ec3811fdb9a80 Mon Sep 17 00:00:00 2001
> From: Martin Becze <mjbecze@riseup.net>
> Date: Sun, 23 Feb 2020 04:27:42 -0500
> Subject: [PATCH v12 7/8] gnu: Add guile3.0-semver.
>
> * gnu/packages/guile-xyz.scm (guile3.0-semver): New variable.

Applied.

> From 2561fbf64e7ea47a0436b3751cbeea0032f8a77b Mon Sep 17 00:00:00 2001
> From: Martin Becze <mjbecze@riseup.net>
> Date: Mon, 3 Feb 2020 16:19:49 -0500
> Subject: [PATCH v12 6/8] import: crate: Parametrized importing of dev
>  dependencies.
>
> This changes the behavoir of the recusive crate importer so that it will
> include importing of development dependencies for the top level package
> but will not inculded the development dependencies for any other imported
> package.
>
> * guix/import/crate.scm (make-crate-sexp): Add the key BUILD?.
>   (crate->guix-package): Add the key INCLUDE-DEV-DEPS?.
>   (crate-recursive-import): Likewise.
> * guix/scripts/import/crate.scm (guix-import-crate): Likewise.
> * tests/crate.scm (cargo-recursive-import): Likewise.

LGTM.

> From cb69a7c4844c68f89b783a1026751ab945fcab5d Mon Sep 17 00:00:00 2001
> From: Martin Becze <mjbecze@riseup.net>
> Date: Thu, 30 Jan 2020 11:19:13 -0500
> Subject: [PATCH v12 5/8] import: utils: Trim patch version from names.
>
> This remove the the patch version from input names. For example
> 'rust-my-crate-1.1.2' now becomes 'rust-my-crate-1.1'
>
> * guix/import/utils.scm (package->definition): Trim patch version from
>   generated package names.
> * tests/crate.scm: (cargo>guix-package): Likewise.
>   (cargo-recursive-import): Likewise.

LGTM.

> From 3f2dbc2a47a2e5e46871fbdeabe951f55d26b557 Mon Sep 17 00:00:00 2001
> From: Martin Becze <mjbecze@riseup.net>
> Date: Thu, 30 Jan 2020 11:17:00 -0500
> Subject: [PATCH v12 4/8] import: crate: Memorize crate->guix-package.
>
> This adds memorization to procedures that involve network lookups.
> 'mem-lookup-crate; is used on every dependency of a package to find
> it's versions. 'mem-crate->guix-package; is needed becuase
> 'topological-sort' depduplicates after dependencies have been turned
> into packages.
>
> * guix/import/crate.scm (mem-crate->guix-package): New procedure.
>   (mem-lookup-crate): New procedure.

This should also mention changes to ‘crate-recursive-import’.

Regarding identifiers, please avoid abbreviations (info "(guix)
Formatting Code").

> +(define mem-lookup-crate (memoize lookup-crate))
> +
>  (define (crate-version-dependencies version)
>    "Return the list of <crate-dependency> records of VERSION, a
>  <crate-version>."
> @@ -216,7 +219,7 @@ latest version of CRATE-NAME."
>          (eq? (crate-dependency-kind dependency) 'normal)))
>  
>    (define crate
> -    (lookup-crate crate-name))
> +    (mem-lookup-crate crate-name))

I’d suggest calling ‘mem-lookup-crate’ ‘lookup-crate*’ for instance.
Can we also make its definition local to ‘crate-version-dependencies’ or
would that defeat your caching goals?

>    (define version-number
>      (or version
> @@ -238,7 +241,7 @@ latest version of CRATE-NAME."
>       containing pairs of (name version)"
>      (sort (map (lambda (dep)
>                   (let* ((name (crate-dependency-id dep))
> -                        (crate (lookup-crate name))
> +                        (crate (mem-lookup-crate name))
>                          (req (crate-dependency-requirement dep))
>                          (ver (find-version crate req)))
>                     (list name
> @@ -265,9 +268,11 @@ latest version of CRATE-NAME."
>                                              string->license))
>            cargo-inputs))))
>  
> +(define mem-crate->guix-package (memoize crate->guix-package))
> +
>  (define* (crate-recursive-import crate-name #:key version)
>    (recursive-import crate-name
> -                    #:repo->guix-package crate->guix-package
> +                    #:repo->guix-package mem-crate->guix-package

Please make ‘mem-crate->guix-package’ local to ‘crate-recursive-import’
and call it ‘crate->guix-package*’ for instance.

(Memoization should always be used as a last resort: it’s a neat hack,
but it’s a hack.  :-)  In particular, it has the problem that its cache
cannot be easily invalidated.  That said, I realize that other importers
do this already, so that’s OK.)

> From 81056961d065e197fe8f1f2096c858776debf485 Mon Sep 17 00:00:00 2001
> From: Martin Becze <mjbecze@riseup.net>
> Date: Thu, 30 Jan 2020 10:52:28 -0500
> Subject: [PATCH v12 3/8] import: crate: Deduplicate dependencies.
>
> * guix/import/crate.scm (crate-version-dependencies): Deduplicate crate dependencies.

Applied.

> From 98129432b4d746fd2a12a005ebe2d36e8ee0f600 Mon Sep 17 00:00:00 2001
> From: Martin Becze <mjbecze@riseup.net>
> Date: Tue, 4 Feb 2020 03:50:48 -0500
> Subject: [PATCH v12 2/8] import: crate: Use guile-semver to resovle module
                                                                  ^^
Typo.  :-)


> *  guix/import/crate.scm (make-crate-sexp): Added '#:skip-build?' to build
>    system args. Pass a VERSION argument to 'cargo-inputs'. Move
>    'package-definition' from scripts/import/crate.scm to here.
>    (crate->guix-package): Use guile-semver to resolve the correct module versions.
>    (crate-name->package-name): Reuse the procedure 'guix-name' instead of
>    duplicating its logic.
>    (module) Added guile-semver as a soft dependency.
> *  guix/import/utils.scm (package-names->package-inputs): Implemented
>    handling of (name version) pairs.
> *  guix/scripts/import/crate.scm (guix-import-crate): Move
>    'package-definition' from here to guix/import/crate.scm.
> *  tests/crate.scm: (recursuve-import) Added version data to the test.

[...]

> +  (define (format-inputs inputs)
> +    (map
> +     (match-lambda
> +       ((name version) (list (crate-name->package-name name)
> +                             (version-major+minor version))))
> +     inputs))

Nitpick: please format as:

  (map (match-lambda
         ((name version)
          (list …)))
       inputs)

> +(define* (crate->guix-package crate-name #:key version #:allow-other-keys)

Please avoid #:allow-other-keys.  In general, it’s best to know exactly
what parameters a procedure expects and to benefit from compile-time
warnings when we make a mistake; thus, #:allow-other-keys should only be
used as a last resort.

> +  (define (find-version crate range)
> +    "finds the a vesion of a crate that fulfils the semver <range>"
                      ^                         ^
Typos.
For inner procedures, please write a comment instead of a docstring.

> From 356bf29011097367a6e95dd45e71050db8bfa8e4 Mon Sep 17 00:00:00 2001
> From: Martin Becze <mjbecze@riseup.net>
> Date: Tue, 4 Feb 2020 07:18:18 -0500
> Subject: [PATCH v12 1/8] import: utils: 'recursive-import' accepts an optional
>  version parameter.
>
> This adds a key VERSION to 'recursive-import' and move the paramter REPO to a
> key. This also changes all the things that rely on 'recursive-import'
>
> * guix/import/utils.scm (recursive-import): Add the VERSION key. Make REPO a
>  key.
> (package->definition): Added optional 'append-version?'.
> * guix/import/cran.scm (cran->guix-package): Change the REPO parameter to a key.
> (cran-recursive-import): Likewise.
> * guix/import/elpa.scm (elpa->guix-pakcage): Likewise.
> (elpa-recursive-import): Likewise.
> * guix/import/gem.scm (gem->guix-package): Likewise.
> (recursive-import): Likewise.
> * guix/import/opam.scm (opam-recurive-import): Likewise.
> * guix/import/pypi.scm (pypi-recursive-import): Likewise.
> * guix/import/stackage.scm (stackage-recursive-import): Likewise.
> * guix/scripts/import/cran.scm: (guix-import-cran) Likewise.
> * guix/scripts/import/elpa.scm: (guix-import-elpa) Likewise.
> * tests/elpa.scm: (eval-test-with-elpa) Likewise.
> * tests/import-utils.scm Likewise.

[...]

>  (define cran->guix-package
>    (memoize
> -   (lambda* (package-name #:optional (repo 'cran))
> +   (lambda* (package-name #:key (repo 'cran) #:allow-other-keys)

I would change #:allow-other-keys to just ‘version’ (a #:version
parameter) in all the importers.

It does mean that #:version would be ignored by those importers, so
perhaps we can add a TODO comment, but eventually someone might
implement it.

If you want you can resubmit patches #1 and #2 to begin with.

Thank you!

Ludo’.