GNU bug report logs - #42338
[PATCH] Add composer build system (PHP)

Previous Next

Full log

Message #137 received at 42338 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 42338 <at> debbugs.gnu.org
Subject: Re: [bug#42338] [PATCH 03/34] guix: Add composer-build-system.
Date: Fri, 25 Sep 2020 12:33:56 +0200

Hi,

Julien Lepiller <julien <at> lepiller.eu> skribis:

> Le Fri, 18 Sep 2020 10:45:48 +0200,
> Ludovic Courtès <ludo <at> gnu.org> a écrit :
>
>> Hi,
>> 
>> Julien Lepiller <julien <at> 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’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.