[bug#42338] [PATCH 03/34] guix: Add composer-build-system.

[Ludovic Courtès]

Hi,

Julien Lepiller <julien@lepiller.eu> skribis:

> Le Fri, 18 Sep 2020 10:45:48 +0200,
> Ludovic Courtès <ludo@gnu.org> a écrit :
>
>> Hi,
>> 
>> Julien Lepiller <julien@lepiller.eu> skribis:
>> 
>> > +    (let* ((package-data (read-package-data #:filename
>> > composer-file))
>> > +           (scripts (match (assoc-ref package-data "scripts")
>> > +                      (('@ script ...) script)
>> > +                      (#f '())))
>> > +           (test-script
>> > +             (assoc-ref scripts test-target))
>> > +           (dependencies (filter (lambda (dep) (string-contains
>> > dep "/"))
>> > +                                 (map car
>> > +                                      (match (assoc-ref
>> > package-data "require")
>> > +                                        (('@ dependency ...)
>> > dependency)
>> > +                                        (#f '())))))
>> > +           (dependencies-dev
>> > +             (filter (lambda (dep) (string-contains dep "/"))
>> > +                     (map car
>> > +                          (match (assoc-ref package-data
>> > "require-dev")
>> > +                            (('@ dependency ...) dependency)
>> > +                            (#f '())))))
>> > +           (name (assoc-ref package-data "name")))  
>> 
>> This is also a case for ‘define-json-mapping’.  I suppose we could use
>> Guile-JSON instead of (guix build json), no?
>> 
>> I think this code and similar occurrences would be less intimidating
>> if we used ‘define-json-mapping’; it would make the data structures
>> clearer, unlike here where one has to keep in mind what the list/tree
>> looks like so they can map car/cdr around.
>
> I think we already tried that with the node build system, but we had to
> revert, because we were importing guile-json from the host side. I
> don't remember the details though, so if you think it's OK now, I'll
> gladly make the code look nicer :)

Yes please. :-)  I think code full of alists/dictionaries would be hard
to read and to maintain since mistakes could end up being silently
ignored or lead to a wrong-type-#f error far down the road.

Also please remember to avoid car/cdr:

  https://guix.gnu.org/manual/en/html_node/Data-Types-and-Pattern-Matching.html

As for Guile-JSON: perhaps you can post a draft that we can play with to
see if there’s anything wrong, but off the top of my head I don’t see
why it wouldn’t work.

>> > +      (match test-script
>> > +        ((? string? command)
>> > +         (unless (equal? (system command) 0)
>> > +           (throw 'failed-command command)))
>> > +        (('@ (? string? command) ...)
>> > +         (for-each
>> > +           (lambda (c)
>> > +             (unless (equal? (system c) 0)
>> > +               (throw 'failed-command c)))
>> > +           command))  
>> 
>> Use (zero? x) instead of (equal? 0 x).
>> 
>> Also, why not use ‘invoke’?  I this because these commands are really
>> shell commands and expect things like glob patterns and tilde
>> expansion? If these are not shell commands, I recommend ‘invoke’,
>> which will report failures more nicely.
>
> Here I have a single string that contains shell commands, so I don't
> think I can use invoke.

‘system’ passes the string to “sh -c”, which means the string is subject
to shelly things: glob expansion, semicolon interpretation, string
quotation, etc.

If those strings are meant to be shell-interpreted, then passing them to
‘system’ is the right thing.  Otherwise, it should be avoided IMO.

Thanks,
Ludo’.