Re: [PATCH] gnu: Add lua-lpeg

[Marius Bakke <mbakke@fastmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4347 bytes --]

José Miguel Sánchez García <jmi2k@openmailbox.org> writes:

> Add lua-lpeg (I hope it's finally correct!)

Cool! Nice to see some Lua love :)

> -- 
> José Miguel Sánchez García
> From 4ebd6648b2dcf1e02d39532173781b91850e4f41 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Jos=C3=A9=20Miguel=20S=C3=A1nchez=20Garc=C3=ADa?=
>  <jmi2k@openmailbox.org>
> Date: Mon, 5 Dec 2016 15:15:33 +0100
> Subject: [PATCH] gnu: Add lua-lpeg
>
> ---
>  gnu/packages/lua-lpeg.scm | 52 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 gnu/packages/lua-lpeg.scm

I think this can go in 'lua.scm', similar to what we do for perl, ruby,
python etc libraries. It's not a lot there currently, so this will help
tremendously :-)

> diff --git a/gnu/packages/lua-lpeg.scm b/gnu/packages/lua-lpeg.scm
> new file mode 100644
> index 0000000..50042bd
> --- /dev/null
> +++ b/gnu/packages/lua-lpeg.scm
> @@ -0,0 +1,51 @@
> +;;; GNU Guix --- Functional package management for GNU
> +;;; Copyright © 2016 José Miguel Sánchez García <jmi2k@openmailbox.org>
> +;;;
> +;;; This file is part of GNU Guix.
> +;;;
> +;;; GNU Guix is free software; you can redistribute it and/or modify it
> +;;; under the terms of the GNU General Public License as published by
> +;;; the Free Software Foundation; either version 3 of the License, or (at
> +;;; your option) any later version.
> +;;;
> +;;; GNU Guix is distributed in the hope that it will be useful, but
> +;;; WITHOUT ANY WARRANTY; without even the implied warranty of
> +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;;; GNU General Public License for more details.
> +;;;
> +;;; 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 (gnu packages lua-lpeg)
> +  #:use-module (guix packages)
> +  #:use-module (guix download)
> +  #:use-module (guix build-system gnu)
> +  #:use-module (guix licenses)
> +  #:use-module (gnu packages lua))
> +
> +(define-public lua-lpeg
> +  (package
> +    (name "lua-lpeg")
> +    (version "1.0.0")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append "http://www.inf.puc-rio.br/~roberto/lpeg/lpeg-"
> +                                  version ".tar.gz"))
> +              (sha256
> +                (base32 "13mz18s359wlkwm9d9iqlyyrrwjc6iqfpa99ai0icam2b3khl68h"))))
> +    (build-system gnu-build-system)
> +    (arguments '(

The cosmetic improvement from the previous email applies here too.

> +      #:make-flags '("LUADIR=/usr/include")

What is LUADIR in this context? Note that Guix does not use a '/usr'
partition, so we typically refer directly to the 'include' dir of each
package directly. This usually works "out of the box" however. Since
there is no '/usr/include' in the build chroot, it can probably be
removed.

> +      #:phases (modify-phases %standard-phases
> +        (delete 'configure)
> +        (replace 'install (lambda _
> +          (let* ((out (assoc-ref %outputs "out")))

Minor nit-pick: Using 'let' here (without the star) is sufficient. The
star is only needed if any of the declared variables uses other
variables declared by the same 'let' statement.

> +            (install-file "lpeg.so" (string-append out "/usr/lib/lua/5.3"))
> +            (install-file "re.lua" (string-append out "/usr/share/lua/5.3"))))))

This looks correct, but '/usr' should be removed. Then they will end up
in the correct profile directories so things Just Work. Could you add a
comment stating why using the normal 'install' phase does not work?

> +      #:tests? #f))

Please also add a comment stating why tests are disabled (if there are
none, or if they need some packages we don't yet have in Guix, etc).

> +    (inputs `(("lua", lua)))
> +    (synopsis "Pattern-matching library for Lua")
> +    (description
> +      "LPeg is a pattern-matching library for Lua, based on Parsing Expression
> +Grammars (PEGs).")
> +    (home-page "http://www.inf.puc-rio.br/~roberto/lpeg")
> +    (license expat)))

Sorry for the nit-picking! With the mentioned improvements I think this
is good to go. Can you send an updated patch? :-)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]