Subject: Re: bug#42338: [PATCH] Add composer build system (PHP)

From: Maxim Cournoyer <[email-masked]>
Date: Sat, 16 Oct 2021 00:15:07 -0400

Hey Julien,

That's a pretty interesting series you have there.

Ludovic Courtès <[email-masked]> writes:

> Hi Julien,
>
> Julien Lepiller <[email-masked]> skribis:
>
>> From 70b9cb2bb389f3e5f9dcc75a44d7d60c28f997bc Mon Sep 17 00:00:00 2001
>> From: Julien Lepiller <[email-masked]>
>> Date: Tue, 29 Oct 2019 08:07:38 +0100
>> Subject: [PATCH 01/34] guix: import: Add composer importer.
>>
>> * guix/import/composer.scm: New file.
>> * guix/scripts/import/composer.scm: New file.
>> * guix/tests/composer.scm: New file.
>> * Makefile.am: Add them.
>> * guix/scripts/import.scm: Add composer importer.
>> * doc/guix.texi (Invoking guix import): Mention it.
>
> [...]
>
>> +@cindex PHP
>> +Import metadat from the @uref{https://getcomposer.org/, Composer} package
>                 ^
> Typo.
>
>> +(define* (composer-fetch name #:optional version)
>> +  "Return an alist representation of the Composer metadata for the package NAME,
>> +or #f on failure."
>> +  (let ((package (json-fetch
>> +                   (string-append (%composer-base-url) "/p/" name ".json"))))
>> +    (if package
>> +        (let* ((packages (assoc-ref package "packages"))
>> +               (package (or (assoc-ref packages name) package))
>> +               (versions (filter
>> +                           (lambda (version)
>> +                             (and (not (string-contains version "dev"))
>> +                                  (not (string-contains version "beta"))))
>> +                           (map car package)))
>> +               (version (or (if (null? version) #f version)
>> +                            (latest-version versions))))
>> +          (assoc-ref package version))
>> +        #f)))
>
> I think this should directly return a <composer-package> since the all
> the callers pass the alist through ‘json->composer-package’.  The idea
> is that alists should be converted to records as soon as they enter the
> process.
>
> Also it’s weird that ‘package’ above has a “packages” (plural) entry.
> Perhaps we’re missing another JSON mapping?
>
> [...]
>
>> +++ b/guix/scripts/import/composer.scm
>> @@ -0,0 +1,107 @@
>> +;;; GNU Guix --- Functional package management for GNU
>> +;;; Copyright © 2015 David Thompson <[email-masked]>
>> +;;; Copyright © 2018 Oleg Pykhalov <[email-masked]>
>
> You can preserve these two lines if you think it’s relevant, but I’d
> suggest adding one for yourself.
>
> OK to push with changes along these lines.

Seems you were almost ready to roll.  Consider this a very gentle ping
:-).

Maxim