GNU bug report logs - #38408
[PATCH 0/3] (WIP) Semantic version aware recusive importer for crates

Previous Next

Full log

Message #233 received at 38408 <at> debbugs.gnu.org (full text, mbox):

From: Martin Becze <mjbecze <at> riseup.net>
To: Ludovic Courtès <ludo <at> gnu.org>
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 15:00:01 -0400

[Message part 1 (text/plain, inline)]

Ok Ludo, so I think I have worked through everything that you mentioned 
and attached is a new patch set.

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

I think it would, 'crate-version-dependencies' only deal with looking up 
the dependencies. If I made it local 'crate->guix-package' it aslo 
wouldn't work because we to cache across multiple calls to 
'crate->guix-package'.

>>     (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.)

Understood. If its any trouble it is easy to drop this commit. Though on 
slow networks it can help quite a bit.

Let me know if anything else needs changed!

Thanks,
Martni

[v13-0006-guix-self-Adds-guile-semver-as-a-depenedency.patch (text/x-patch, attachment)]

[v13-0005-import-crate-Parametrized-importing-of-dev-depen.patch (text/x-patch, attachment)]

[v13-0004-import-utils-Trim-patch-version-from-names.patch (text/x-patch, attachment)]

[v13-0003-import-crate-Memorize-crate-guix-package.patch (text/x-patch, attachment)]

[v13-0002-import-crate-Use-guile-semver-to-resolve-module-.patch (text/x-patch, attachment)]

[v13-0001-import-utils-recursive-import-accepts-an-optiona.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.