[bug#38408] [PATCH v9 3/8] Added Guile-Semver as a dependency to guix

[Martin Becze]

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
Description: Text Data

v13-0005-import-crate-Parametrized-importing-of-dev-depen.patch
Description: Text Data

v13-0004-import-utils-Trim-patch-version-from-names.patch
Description: Text Data

v13-0003-import-crate-Memorize-crate-guix-package.patch
Description: Text Data

v13-0002-import-crate-Use-guile-semver-to-resolve-module-.patch
Description: Text Data

v13-0001-import-utils-recursive-import-accepts-an-optiona.patch
Description: Text Data