[bug#55030] [PATCH 00/30] gnu: elm: Update to 0.19.1. Add build system & importer.

[Philip McGrath]

Hi,

On 5/1/22 16:35, Ludovic Courtès wrote:
> Philip McGrath <philip@philipmcgrath.com> skribis:
>
> > * gnu/packages/patches/elm-offline-package-registry.scm: New file.
> > * gnu/local.mk (dist_patch_DATA): Add it.
> > * gnu/packages/elm.scm (elm): Use it.
> > * guix/build-system/elm.scm, guix/build/elm-build-system.scm,
> > guix/import/elm.scm, guix/scripts/import/elm.scm: New files.
> > * guix/scripts/import.scm (importers): Add "elm".
>
> I think the custom would be to add the importer in a separate commit; if
> you can do that, that’s great.

I certainly can split it that way. I did it like this because I actually 
wrote the importer before the build system.

> Could you add an entry for the importer under “Invoking guix import”,
> and one for the build system under “Build Systems” in guix.texi?  You
> can follow existing entries as a template.

I will give it a try! I haven't written any Texinfo before.

> It would be nice to have tests for the importer.  One way to do that is
> like ‘tests/cpan.scm’, which spawns an HTTP server that mimics the real
> registry.

I'll take a look at that.

> > +;; COMMENTARY:
>
> Nitpick: You can make that literally “;;; Commentary:”.   That’s what
> (ice-9 documentation) expects.
>
> > +;; CODE:
>
> Likewise: “;;; Code:”.

Will do.

> > +(define elm-package-registry
> > +  ;; It is much nicer to fetch this small (< 40 KB gzipped)
> > +  ;; file once than to do many HTTP requests.
> > +  (mlambda ()
> > +    "Fetch the Elm package registry, represented as a vhash mapping package
> > +names to lists of available versions, sorted from latest to oldest."
> > +    (let ((url "https://package.elm-lang.org/all-packages";))
> > +      (cond
> > +       ((json-fetch url)
> > +        => (lambda (alist)
> > +             (fold (lambda (entry vh)
> > +                     (match entry
> > +                       ((name . vec)
> > +                        (vhash-cons name
> > +                                    (sort (vector->list vec) version>?)
> > +                                    vh))))
> > +                   vlist-null
> > +                   alist)))
> > +       (else
> > +        (raise (formatted-message
> > +                (G_ "error downloading Elm package registry from ~a")
> > +                url)))))))
> > +
> > +(define (make-elm-package-sexp name version)
> > +  "Return two values: the `package' s-expression for the Elm package with the
> > +given NAME and VERSION, and a list of Elm packages it depends on."
> > +  (define-values (checkout _commit _relation)
> > +    ;; Elm requires that packages use this very specific format
> > +    (update-cached-checkout (string-append "https://github.com/"; name)
> > +                            #:ref `(tag . ,version)))
> > +  (define info
> > +    (call-with-input-file (string-append checkout "/elm.json")
> > +      json->scm))
> > +  (define (get-deps key)
> > +    (cond
> > +     ((assoc-ref info key)
> > +      => (cut map car <>))
> > +     (else
> > +      '())))
>
> The way the importer fiddles with alists isn’t pretty IMO.  :-)
>
> How about using ‘define-json-mapping’ (also from Guile-JSON) to “map”
> JSON data structures to records?  See how pypi.scm and others do it.
> The resulting code should be clearer.

I had tried that first, but there were some problems: IIRC, there might 
have been an issue with potentially-absent fields defaulting to 
*unspecified*, some alist manipulation was needed anyway for fields that 
use JSON objects as key--value maps, and, with a view toward being able 
to process `{"type":"application"}` files some day, there didn't seem to 
be enough ability to adapt parsing based on the value for the key. I 
found this code less confusing. But I can try again if it seems important!

> Also, instead of or in addition to memoizing ‘elm-package-registry’,
> would it make sense to use ‘http-fetch/cached’ to fetch that file?

I'll take a look!

> Nitpick: Guile has multiple-value truncation, so you can write:
>
>    (define checkout
>      (update-cached-checkout …))

I saw that some places in Guix relied on that already, but I also saw 
that `info guile values` says that:

> The effect of passing no
> value or more than one value to continuations that were not created
> by ‘call-with-values’ is unspecified.

... so I wasn't sure what to do.

-Philip