Re: [PATCH 1/4] gnu: node: Add http-parser.

[Alex Kost]

Jelle Licht (2016-08-27 14:23 +0300) wrote:

> * gnu/packages/node.scm (http-parser): New variable.
> * gnu/packages/node.scm (define-module): Import gnu packages tls with
>   tls: prefix

These are 2 independent things, so I think it should be 2 separate
patches, but actually why is it needed to prefix tls?

> ---
>  gnu/packages/node.scm | 39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
>
>
> diff --git a/gnu/packages/node.scm b/gnu/packages/node.scm
> index 2b27774..545aaaa 100644
> --- a/gnu/packages/node.scm
> +++ b/gnu/packages/node.scm
> @@ -32,7 +32,42 @@
>    #:use-module (gnu packages linux)
>    #:use-module (gnu packages perl)
>    #:use-module (gnu packages python)
> -  #:use-module (gnu packages tls))
> +  #:use-module ((gnu packages tls) #:prefix tls:)
> +  #:use-module (gnu packages valgrind))
> +
> +(define-public http-parser
> +  (package
> +    (name "http-parser")
> +    (version "2.7.1")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append 
> "https://github.com/nodejs/http-parser/archive/v";
> +                                  version ".tar.gz"))

Here the following line is needed:
                 (file-name (string-append name "-" version ".tar.gz"))

Without it the source tarball will have "...-v2.7.1.tar.gz" name, which
is not very understandable.

> +              (sha256 (base32 
> "1cw6nf8xy4jhib1w0jd2y0gpqjbdasg8b7pkl2k2vpp54k9rlh3h"))))
> +    (build-system gnu-build-system)
> +    (arguments
> +     '(#:make-flags (list "CC=gcc" (string-append "DESTDIR=" (assoc-ref 
> %outputs "out")))

Does it work if it will be PREFIX instead of DESTDIR?  I forgot the
exact reason, but we try to avoid using DESTDIR directly, and prefer to
set PREFIX instead.

Also please don't make such long lines :-)

> +       #:test-target "test-valgrind"
> +       #:phases
> +       (modify-phases %standard-phases
> +         (delete 'configure)
> +         (add-before 'build 'patch-makefile
> +           (lambda* (#:key inputs outputs #:allow-other-keys)

Just (lambda _ ...)  Keywords are not needed here

> +              (substitute* '("Makefile")
> +                (("/usr/local") ""))))

Please add #t in the end of this phase: succeeded phases should return
non-false value, and the return value of 'substitute*' is not specified.

> +         (replace 'build
> +           (lambda* (#:key make-flags #:allow-other-keys)
> +             (zero? (apply system* "make" "library" make-flags))))
> +         )))

Please move these lonely parentheses to the previous line.

> +    (inputs '())

This line is not needed as the empty list is the default value.

> +    (native-inputs `(("valgrind" ,valgrind)))
> +    (home-page "https://github.com/nodejs/http-parser";)
> +    (synopsis "HTTP request/response parser for C")
> +    (description "HTTP parser is a parser for HTTP messages written in C.  It
> +parses both requests and responses.  The parser is designed to be used in
> +performance HTTP applications.  It does not make any syscalls nor 
> allocations,
> +it does not buffer data, it can be interrupted at anytime.")
> +    (license expat)))
>  
>  (define-public node
>    (package
> @@ -118,7 +153,7 @@
>         ("which" ,which)))
>      (inputs
>       `(("libuv" ,libuv)
> -       ("openssl" ,openssl)
> +       ("openssl" ,tls:openssl)
>         ("zlib" ,zlib)))
>      (synopsis "Evented I/O for V8 JavaScript")
>      (description "Node.js is a platform built on Chrome's JavaScript runtime

-- 
Alex