[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#42338] [PATCH 03/34] guix: Add composer-build-system.
From: |
Ludovic Courtès |
Subject: |
[bug#42338] [PATCH 03/34] guix: Add composer-build-system. |
Date: |
Fri, 25 Sep 2020 12:33:56 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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’.