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

[Philip McGrath]

Hi,

Here is v2!

A few notes before the patches:

On 5/1/22 18:17, Philip McGrath wrote:
> Hi,
> 
> On 5/1/22 16:37, Ludovic Courtès wrote:
>> Philip McGrath <philip@philipmcgrath.com> skribis:
>>
>>> +  (package
>>> +    (name "elm-virtual-dom")
>>
>> [...]
>>
>>> +    (properties '((upstream-name . "elm/virtual-dom")))))
>>
>> Could/should the importer infer the upstream name from the Guix name by
>> default?
>>
>> That way, we’d only need to specify that property where the automatic
>> Guix->upstream name mapping wouldn’t work.
> 
> It could, but the heuristics seemed a bit brittle. To pick a few examples:
> 
>   1. elm-virtual-dom -> "elm/virtual-dom"
>   2. elm-explorations-markdown -> "elm-explorations/markdown"
>   2. elm-terezka-intervals -> "terezka/intervals"
> 
> We could add a special case for the "elm-explorations/*" namespace, but 
> at least one of the others would need an explicit property. I *think* 
> most of the packages in the "elm/*" namespace are single-element (e.g. 
> "elm/html"), so maybe we could require the property for e.g. 
> "elm/virtual-dom" and "elm/project-metadata-utils" ...
> 

I think I've come up with an approach to inferring upstream names that handles
most cases (everything in this series but "elm/virtual-dom" and
"elm/project-metadata-utils") while also not adding so much magic that the
behavior would be unpredictable.

I've put the functions going in both directions in `(guix build-system elm)`
rather than `(guix import elm)` (which seemed somewhat more common) because
`elm-package-origin` needs `elm->package-name`, and it seemed better overall
to define the Elm-to-Guix and Guix-to-Elm conversions in the same place.

On 5/1/22 18:03, Philip McGrath wrote:
> Hi,
> 
> On 5/1/22 16:35, Ludovic Courtès wrote:
>> 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.
>

I've added documentation there, and also a section on naming conventions in
contributing.texi. I'm not sure all of my Texinfo is idiomatic (well, I
didn't find equivalents for all of the Scribble cross-reference functions I'm
used to), but it does build and work ok.

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

I've added a number of tests. They seem to be producing more "PASS" lines than
other tests, though. It seems like nested `test-group`s may be handled
differently than I'm used to from RackUnit and (less so) Racket's `srfi/64`
implementation, but I'm not sure what the idiomatic way to structure them
would be, if this isn't it.

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

Ironically, I couldn't find any documentation for `(ice-9 documentation)`
other than the comments in the source file, but I hope I've done this
properly now.

>>
>> 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!
>

Most of the problems I'd run into were because I'd gotten the wrong idea from
the grammar of 'define-json-mapping`, and `define-json-mapping` didn't check
its implicit requirements: I've reported the details and potential
improvements at <https://github.com/aconchillo/guile-json/issues/79>.

The remaining, unescapable alist manipulation is for cases when JSON objects
are used as key--value maps, rather than records with some finite set of
potential keys.

>> 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!
>

Using `http-fetch/cached` without duplicating `json-fetch` required a few
additional patches:

 - [PATCH v2 06/34] http-client: Accept '#:headers' in 'http-fetched/cached'.
 - [PATCH v2 07/34] http-client: 'http-fetch/cached' converts strings to URIs.
 - [PATCH v2 08/34] import: json: Accept '#:http-fetch' in 'json-fetch'.

They seemed small enough, and IIUC they don't affect any build-side code or
trigger rebuilds, but OTOH the actuall JSON being fetched is quite small, so
it wouldn't be a problem to drop them if there are any problems.

One other difference is that `elm-virtual-dom` had an upstream release since
the first version of this patch series, so that package is now at 1.0.3.

 -Philip

Philip McGrath (34):
  gnu: elm-compiler: Update to 0.19.1.
  gnu: elm: Rename package to match the command.
  guix: Add elm-build-system.
  gnu: Add elm-core and elm-json.
  build-system/elm: Add implicit Elm inputs.
  http-client: Accept '#:headers' in 'http-fetched/cached'.
  http-client: 'http-fetch/cached' converts strings to URIs.
  import: json: Accept '#:http-fetch' in 'json-fetch'.
  import: Add Elm importer.
  gnu: Add elm-virtual-dom.
  gnu: Add elm-html.
  gnu: Add elm-svg.
  gnu: Add elm-time.
  gnu: Add elm-url.
  gnu: Add elm-browser.
  gnu: Add elm-bytes.
  gnu: Add elm-file.
  gnu: Add elm-http.
  gnu: Add elm-parser.
  gnu: Add elm-project-metadata-utils.
  gnu: Add elm-explorations-markdown.
  gnu: elm: Support 'elm reactor'.
  gnu: Add elm-todomvc.
  gnu: Add elm-debois-elm-dom.
  gnu: Add elm-random.
  gnu: Add elm-explorations-test.
  gnu: Add elm-danhandrea-elm-date-format.
  gnu: Add elm-danhandrea-elm-time-extra.
  gnu: Add elm-justinmimbs-date.
  gnu: Add elm-justinmimbs-time-extra.
  gnu: Add elm-myrho-elm-round.
  gnu: Add elm-ryannhg-date-format.
  gnu: Add elm-terezka-intervals.
  gnu: Add elm-terezka-elm-charts.

 Makefile.am                                   |   5 +
 doc/contributing.texi                         |  82 ++
 doc/guix.texi                                 |  80 ++
 gnu/local.mk                                  |   4 +-
 gnu/packages/elm.scm                          | 749 +++++++++++++++++-
 .../elm-compiler-disable-reactor.patch        |  71 --
 .../patches/elm-compiler-fix-map-key.patch    |  38 -
 .../elm-offline-package-registry.patch        |  71 ++
 .../patches/elm-reactor-static-files.patch    | 251 ++++++
 guix/build-system/elm.scm                     | 206 +++++
 guix/build/elm-build-system.scm               | 380 +++++++++
 guix/http-client.scm                          |  24 +-
 guix/import/elm.scm                           | 210 +++++
 guix/import/json.scm                          |   9 +-
 guix/scripts/import.scm                       |   3 +-
 guix/scripts/import/elm.scm                   | 107 +++
 tests/elm.scm                                 | 268 +++++++
 17 files changed, 2414 insertions(+), 144 deletions(-)
 delete mode 100644 gnu/packages/patches/elm-compiler-disable-reactor.patch
 delete mode 100644 gnu/packages/patches/elm-compiler-fix-map-key.patch
 create mode 100644 gnu/packages/patches/elm-offline-package-registry.patch
 create mode 100644 gnu/packages/patches/elm-reactor-static-files.patch
 create mode 100644 guix/build-system/elm.scm
 create mode 100644 guix/build/elm-build-system.scm
 create mode 100644 guix/import/elm.scm
 create mode 100644 guix/scripts/import/elm.scm
 create mode 100644 tests/elm.scm


base-commit: 665dd8211cb5c7556f0fb83944d33215accf957a
-- 
2.32.0