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

[Martin Becze <mjbecze@riseup.net>]

Thanks for the feedback Ludo! I will try to get this done today.

On 3/24/20 6:18 AM, Ludovic Courtès wrote:
> 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’.
>