On Wed, 09 Nov 2016 16:44:41 +0100
ludo@gnu.org (Ludovic Courtès) wrote:

> Hello Julien,
> 
> Seems like the original reviewer (hi Marius! ;-)) missed this
> revision, so here are a few comments.
> 
> Julien Lepiller <julien@lepiller.eu> skribis:
> 
> > From 94c512aa3c9710b65b6fce0cd108744a7c308c63 Mon Sep 17 00:00:00
> > 2001 From: Julien Lepiller <julien@lepiller.eu>
> > Date: Sun, 30 Oct 2016 15:05:51 +0100
> > Subject: [PATCH] gnu: Add php
> >
> > * gnu/packages/php.scm: New file.
> > * gnu/packages/php.scm (php): New variable.  
> 
> Only the first line is needed.
> 
> > +              (snippet
> > +                '(with-directory-excursion "ext"
> > +                   (for-each delete-file-recursively
> > +                             `("pcre/pcrelib"
> > +                               "sqlite3/libsqlite"
> > +                               "gd/libgd"
> > +                               "mbstring/oniguruma"
> > +                               "xmlrpc/libxmlrpc"
> > +                               "zip/lib"))))))
> > +                               ;; couldn't unbundle these
> > libraries:
> > +                               ;"bcmath/libbcmath" ;; this is bc.
> > +                               ;"fileinfo/libmagic"
> > +                               ;"mbstring/libmbfl"
> > +                               ;"date/lib"  
> 
> Is it hard to unbundle ‘bc’ and ‘file’ (libmagic)?
> 
> > +    (build-system gnu-build-system)
> > +    (arguments
> > +       '(
> > +        #:configure-flags  
> 
> Please adjust the indentation (check what other files do.)
> 
> > +          (list (string-append "--with-libxml-dir="
> > +                               (assoc-ref %build-inputs
> > "libxml2"))  
> 
> I suggest this trick to make it a bit more concise:
> 
>   #:configure-flags
>   (let-syntax ((with (syntax-rules ()
>                        ((_ option input)
>                         (string-append option "="
>                                        (assoc-ref %build-inputs
> input)))))) (list (with "--with-libxml-dir" "libxml2")
>           (with "--with-readline" "readline")
>           …))
> 
> > +        ; A lot of tests fail and failure is not considered fatal.
> > +        #:tests? #f))  
> 
> In what sense is it not considered fatal?  :-)
> 
> It would be nice to have some understanding of why the test fails.  If
> it turns out to be difficult to fix, we can at least document the
> problem in the comment, with enough detail.  Could you try to
> investigate a bit?
I investigated a bit, but couldn't find any reason. logs say that tests
print nothing where they should print the result, so they fail. When I
run them later from outside the build environment (with the php
executable that was built), it works perfectly fine though.

> 
> > +    (license license:gpl2)));(list
> > +             ; (license:non-copyleft "file://LICENSE"); the php
> > license
> > +             ; license:lgpl2.1;bcmath and libmbfl
> > +             ; license:bsd-2;libmagic
> > +             ; license:expat)));date/lib  
> 
> So PHP itself is GPLv2-only?
> 
> Apart from that, I think we’re almost done.
> 
> Could you send an updated patch to address those issues?  Then we can
> happily apply it.
here is the updated patch. I let the tests be done because it doesn't
harm, but it does no good either, so feel free to disable them if you
prefer.

> 
> Thank you for working on it!
> 
> Ludo’.