From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:470:142:3::10]:56955) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jGkPU-0007uR-ET for guix-patches@gnu.org; Tue, 24 Mar 2020 10:20:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1jGkPS-0001ir-FW for guix-patches@gnu.org; Tue, 24 Mar 2020 10:20:04 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:49437) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1jGkPS-0001i3-B3 for guix-patches@gnu.org; Tue, 24 Mar 2020 10:20:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jGkPS-0005Gb-5o for guix-patches@gnu.org; Tue, 24 Mar 2020 10:20:02 -0400 Subject: [bug#38408] [PATCH v9 3/8] Added Guile-Semver as a dependency to guix Resent-Message-ID: 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> <87mu86njj3.fsf@gnu.org> From: Martin Becze Message-ID: <34c9958a-2453-601d-574f-9c9d59214b17@riseup.net> Date: Tue, 24 Mar 2020 10:19:12 -0400 MIME-Version: 1.0 In-Reply-To: <87mu86njj3.fsf@gnu.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: 38408@debbugs.gnu.org, Efraim Flashner , jsoo1@asu.edu, Leo Famulari 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 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 ‘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 records of VERSION, a >> ." >> @@ -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 >> 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 >> 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 " > ^ ^ > 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 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’. >