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

[Philip McGrath <philip@philipmcgrath.com>]

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