From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:470:142:3::10]:51539) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jGgeJ-00051M-7n for guix-patches@gnu.org; Tue, 24 Mar 2020 06:19:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1jGgeD-0004Ex-Ra for guix-patches@gnu.org; Tue, 24 Mar 2020 06:19:07 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:47941) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1jGgeD-0004Ei-NB for guix-patches@gnu.org; Tue, 24 Mar 2020 06:19:01 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jGgeD-0002lw-JY for guix-patches@gnu.org; Tue, 24 Mar 2020 06:19:01 -0400 Subject: [bug#38408] [PATCH v9 3/8] Added Guile-Semver as a dependency to guix Resent-Message-ID: From: Ludovic =?UTF-8?Q?Court=C3=A8s?= References: <20200217100345.GI1968@E5400> <87wo8l702y.fsf@gnu.org> <874kvld2bl.fsf@gnu.org> <87sgj4nwjw.fsf@gnu.org> <8453e3ce-bc5a-3bb3-1bfb-deaca7ca11d3@riseup.net> <6386cf3e-1156-8d09-a264-cacbb4935f03@riseup.net> <47ed3e36-0d9f-cd6f-a64b-e3f9c9d71537@riseup.net> <20200322201018.GB16716@jasmine.lan> <45972138-5bf8-821a-178a-c04e6446e076@riseup.net> Date: Tue, 24 Mar 2020 11:18:08 +0100 In-Reply-To: (Martin Becze's message of "Mon, 23 Mar 2020 12:28:04 -0400") Message-ID: <87mu86njj3.fsf@gnu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Martin Becze Cc: 38408@debbugs.gnu.org, Efraim Flashner , jsoo1@asu.edu, Leo Famulari Hi Martin & all, I apologize for taking so long and dropping the ball. Partly that=E2=80=99s because this is non-trivial, thanks for working on it, Martin! Some quick comments: Martin Becze skribis: > From 494f7c874781f64b702e31841c95c95c68fb28fc Mon Sep 17 00:00:00 2001 > From: Martin Becze > 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 > 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 > 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 > 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 > 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 =E2=80=98crate-recursive-import=E2=80= =99. 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 records of VERSION, a > ." > @@ -216,7 +219,7 @@ latest version of CRATE-NAME." > (eq? (crate-dependency-kind dependency) 'normal))) >=20=20 > (define crate > - (lookup-crate crate-name)) > + (mem-lookup-crate crate-name)) I=E2=80=99d suggest calling =E2=80=98mem-lookup-crate=E2=80=99 =E2=80=98loo= kup-crate*=E2=80=99 for instance. Can we also make its definition local to =E2=80=98crate-version-dependencie= s=E2=80=99 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)))) >=20=20 > +(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 =E2=80=98mem-crate->guix-package=E2=80=99 local to =E2=80=98cra= te-recursive-import=E2=80=99 and call it =E2=80=98crate->guix-package*=E2=80=99 for instance. (Memoization should always be used as a last resort: it=E2=80=99s a neat ha= ck, but it=E2=80=99s a hack. :-) In particular, it has the problem that its c= ache cannot be easily invalidated. That said, I realize that other importers do this already, so that=E2=80=99s OK.) > From 81056961d065e197fe8f1f2096c858776debf485 Mon Sep 17 00:00:00 2001 > From: Martin Becze > 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 d= ependencies. Applied. > From 98129432b4d746fd2a12a005ebe2d36e8ee0f600 Mon Sep 17 00:00:00 2001 > From: Martin Becze > 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 =E2=80=A6))) inputs) > +(define* (crate->guix-package crate-name #:key version #:allow-other-key= s) Please avoid #:allow-other-keys. In general, it=E2=80=99s best to know exa= ctly 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 " ^ ^ Typos. For inner procedures, please write a comment instead of a docstring. > From 356bf29011097367a6e95dd45e71050db8bfa8e4 Mon Sep 17 00:00:00 2001 > From: Martin Becze > Date: Tue, 4 Feb 2020 07:18:18 -0500 > Subject: [PATCH v12 1/8] import: utils: 'recursive-import' accepts an opt= ional > 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 REP= O 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 =E2=80=98version=E2=80=99 (a #:ve= rsion 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=E2=80=99.