Package: guix-patches;
Reported by: Martin Becze <mjbecze <at> riseup.net>
Date: Thu, 28 Nov 2019 00:14:01 UTC
Severity: normal
Tags: patch
Fixed in version 44560
Done: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Bug is archived. No further changes may be made.
Message #227 received at 38408 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Martin Becze <mjbecze <at> riseup.net> Cc: 38408 <at> debbugs.gnu.org, Efraim Flashner <efraim <at> flashner.co.il>, jsoo1 <at> asu.edu, Leo Famulari <leo <at> famulari.name> Subject: Re: [bug#38408] [PATCH v9 3/8] Added Guile-Semver as a dependency to guix Date: Tue, 24 Mar 2020 11:18:08 +0100
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 <at> riseup.net> skribis:
> From 494f7c874781f64b702e31841c95c95c68fb28fc Mon Sep 17 00:00:00 2001
> From: Martin Becze <mjbecze <at> 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 <at> 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 <at> 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 <at> 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 <at> 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 <at> 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 <at> 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 <at> 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’.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.