[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Add php
From: |
Julien Lepiller |
Subject: |
Re: [PATCH] Add php |
Date: |
Fri, 11 Nov 2016 17:31:23 +0100 |
On Wed, 09 Nov 2016 16:44:41 +0100
address@hidden (Ludovic Courtès) wrote:
> Hello Julien,
>
> Seems like the original reviewer (hi Marius! ;-)) missed this
> revision, so here are a few comments.
>
> Julien Lepiller <address@hidden> skribis:
>
> > From 94c512aa3c9710b65b6fce0cd108744a7c308c63 Mon Sep 17 00:00:00
> > 2001 From: Julien Lepiller <address@hidden>
> > 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’.
0001-gnu-Add-php.patch
Description: Text Data
- Re: [PATCH] Add php, Leo Famulari, 2016/11/01
- Re: [PATCH] Add php, Julien Lepiller, 2016/11/02
- Re: [PATCH] Add php, Ludovic Courtès, 2016/11/09
- Re: [PATCH] Add php,
Julien Lepiller <=
- Re: [PATCH] Add php, Marius Bakke, 2016/11/14
- Re: [PATCH] Add php, Marius Bakke, 2016/11/14
- Re: [PATCH] Add php, Ludovic Courtès, 2016/11/14
- Re: [PATCH] Add php, Marius Bakke, 2016/11/14
- Re: [PATCH] Add php, Ludovic Courtès, 2016/11/14
- Re: [PATCH] Add php, Marius Bakke, 2016/11/16
- Re: [PATCH] Add php, Hartmut Goebel, 2016/11/17
- Re: [PATCH] Add php, Julien Lepiller, 2016/11/17
- Re: [PATCH] Add php, tyreunom, 2016/11/17
- Re: [PATCH] Add php, Ludovic Courtès, 2016/11/17