Re: [PATCH] Add fyba to GNU Guix

[Ricardo Wurmus]

Hi Dennis,

> This patch adds Fyba, a library required by GDAL to process Norwegian
> statistical datums, to GNU Guix.

Thanks for your patch!

> From the package description:
>
> FYBA is the source code release of the FYBA library, distributed by
> the National Mapping Authority of Norway (Kartverket) to read and
> write files in the National geodata standard format SOSI.

This is a much better description than the one you chose to use for the
package below.


> From 5212e686078f73211182a4fe52d81529faccefba Mon Sep 17 00:00:00 2001
> From: Dennis Mungai <address@hidden>
> Date: Sun, 20 Mar 2016 04:48:27 +0300
> Subject: [PATCH] Ported the fyba package to GNU Guix

Please see my comments on commit messages earlier.

> +(define-module (gn packages fyba)

Is it really necessary to create a new module?  Could this package be
added to an existing module instead?  Also, our modules are called “gnu
packages ...” not “gn packages ...”.

> +(define-public fyba
> +  (package
> +   (name "fyba")
> +   (version "4.1.1")
> +   (source (origin
> +             (method url-fetch)
> +             (uri (string-append 
> "https://github.com/kartverket/fyba/archive/";
> +                                 version ".tar.gz"))
> +             (file-name (string-append name "-" version ".tar.gz"))
> +             (sha256
> +              (base32
> +               "0ya1agi78d386skq353dk400fl11q6whfqmv31qrkn4g5vamixlr"))))

OK.  Have you checked that there is no other release tarball?

> +    (inputs `(("zip" ,zip)
> +             ("autoconf" ,autoconf)
> +             ("automake" ,automake)
> +             ("libtool" ,libtool)
> +             ("libgcrypt" ,libgcrypt)))                                      
>         

Most packages place the “inputs” field after “arguments”.  I think its
better to do the same.

Most of these, however, look like they should be “native-inputs”.

Are the autotools really required?  Or is there a bootstrapped release
tarball somewhere that we could use instead?

> +    (build-system gnu-build-system)
> +     (arguments
> +     '(#:phases (modify-phases %standard-phases
> +                    (add-after 'unpack `bootstrap
> +                      (lambda _
> +                        (zero? (system* "autoreconf" "-vfi")))))))    
> +    (home-page "http://labs.kartverket.no/sos/";)
> +    (synopsis "source code release of the FYBA library")

This is not a helpful synopsis.  It should tell me succintly what this
library does, not that this is a release.

> +    (description "OpenFYBA is the source code release of the FYBA library.")

Is it OpenFYBA or just FYBA?  What is the FYBA library?  Your
explanation in the email is a lot better than this.

> +    (license (list gpl2))))

No “list” required when you only have one license.  Is this really just
“GPLv2 only” or “GPLv2 or later”?

Could you please resend this patch after addressing these comments and
running “guix lint fyba”?

Thanks again!

~~ Ricardo