[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, 18 Sep 2020 10:45:48 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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’.