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

["Ludovic Courtès" <ludo@gnu.org>]

Hi,

Julien Lepiller <julien@lepiller.eu> skribis:

> From bb5d102b6ea5e6b5c06bbf90a58927c6180e23bc Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Tue, 29 Oct 2019 20:58:51 +0100
> Subject: [PATCH 03/34] guix: Add composer-build-system.
>
> * guix/build-system/composer.scm: New file.
> * guix/build/composer-build-system.scm: New file.
> * gnu/packages/aux-files/findclass.php: New file.
> * Makefile.am: Add them.
> * doc/guix.texi (Build Systems): Document it.

[...]

> --- /dev/null
> +++ b/gnu/packages/aux-files/findclass.php

I can’t believe we’ll have PHP in our code base.  :-)

> +;;; You should have received a copy of the GNU General Public License
> +;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
> +(define-module (guix build-system composer)

Missing newline.

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

> +      (for-each
> +        (lambda (input)

Like for ‘map’, please indent on the same line:

  (for-each (lambda (input)

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

> +(define (find-php-dep inputs dependency)
> +  (let loop ((inputs (map cdr inputs)))
> +    (if (null? inputs)
> +        (throw 'unsatisfied-dependency "Unsatisfied dependency: required " dependency)
> +        (let ((autoload (string-append (car inputs) "/share/web/" dependency "/vendor/autoload_conf.php")))
> +          (if (file-exists? autoload)
> +              autoload
> +              (loop (cdr inputs)))))))

Please use ‘match’ instead of car/cdr.

> +(define* (create-autoload vendor composer-file inputs #:key dev-dependencies?)
> +  (with-output-to-file (string-append vendor "/autoload.php")
> +    (lambda _
> +      (display "<?php
> +// autoload.php @generated by Guix
> +$map = $psr4map = $classmap = array();
> +")
> +      (format #t "require_once '~a/autoload_conf.php'~%" vendor)
> +      (format #t "require_once '~a/share/web/composer/ClassLoader.php'~%"
> +              (assoc-ref inputs "composer-classloader"))
> +      (display "$loader = new \\Composer\\Autoload\\ClassLoader();
> +foreach ($map as $namespace => $path) {
> +  $loader->set($namespace, $path);
> +}
> +foreach ($psr4map as $namespace => $path) {
> +  $loader->setPsr4($namespace, $path);
> +}
> +$loader->addClassMap($classmap);
> +$loader->register();
> +")))

Please add a docstring explaining what’s happening here.  Also, perhaps
use ‘string-append’ instead of ‘format’ so we don’t end up generating
things like:

  require_once '#f/autoload_conf.php'

:-)

In short, I think we must pay attention to the style to facilitate
maintainability.

Could you send an updated patch?

Thanks!

Ludo’.