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 <address@hidden> skribis:
From 494f7c874781f64b702e31841c95c95c68fb28fc Mon Sep 17 00:00:00 2001
From: Martin Becze <address@hidden>
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 <address@hidden>
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 <address@hidden>
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 <address@hidden>
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 <address@hidden>
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 <address@hidden>
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 <address@hidden>
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 <address@hidden>
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’.