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

Previous Next

Full log

Message #125 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, 18 Sep 2020 10:45:48 +0200

Hi,

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

> From bb5d102b6ea5e6b5c06bbf90a58927c6180e23bc Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien <at> 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’.

Previous Next

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