Re: [PATCH] gnu: Add libicns

[Marius Bakke <mbakke@fastmail.com>]

Brendan Tildesley <brendan.tildesley@openmailbox.org> writes:

> On 2016-09-03 22:38, Marius Bakke wrote:
>> Marius Bakke <mbakke@fastmail.com> writes:
>>
>>> Additionally this package bundles "m4" even if it doesn't use it. Can
>>> you remove it with a "origin" snippet? See e.g. "xcb-util-xrm" for how
>>> that's done.
>> My mistake, it does appear to use those files, and it's not actually m4.
>> I think using the generated libtool is probably OK.
>>
>> Alternatively it can be re-generated by adding a phase that invokes
>> "autoreconf -vif". I'm not actually sure if this is desired, so just
>> leave it for now.
>>
>> Thank you,
>> Marius
>
> Ah, well that lost me a bit of time.

Sorry about that! Now it re-generates libtool and friends, so it doesn't
hurt, at least. I only have a couple of remarks, and then someone with
commit access can take a final glance.

> From b04ad5e40ac5befe30832ffbcf3b4283b4ae1d63 Mon Sep 17 00:00:00 2001
> From: Brendan Tildesley <brendan.tildesley@openmailbox.org>
> Date: Sat, 3 Sep 2016 05:53:56 +1000
> Subject: [PATCH] gnu: Add libicns.
>
> * gnu/packages/image.scm (libicns): New variable.
> ---
>  gnu/packages/image.scm | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/gnu/packages/image.scm b/gnu/packages/image.scm
> index 4fdc4ae..1b21306 100644
> --- a/gnu/packages/image.scm
> +++ b/gnu/packages/image.scm
> @@ -48,6 +48,7 @@
>    #:use-module (guix download)
>    #:use-module (guix build-system gnu)
>    #:use-module (guix build-system cmake)
> +  #:use-module (guix utils)

I don't think this is required.

>    #:use-module (srfi srfi-1))
>  
>  (define-public libpng
> @@ -147,6 +148,45 @@ maximum quality factor.")
>     (license license:gpl2+)
>     (home-page "http://www.kokkonen.net/tjko/projects.html#jpegoptim")))
>  
> +(define-public libicns
> +  (package
> +    (name "libicns")
> +    (version "0.8.1")
> +    (source (origin
> +             (method url-fetch)
> +             (uri (string-append
> +                   "mirror://sourceforge/icns/libicns-" version ".tar.gz"))
> +             (sha256 (base32
> +                      "1hjm8lwap7bjyyxsyi94fh5817xzqhk4kb5y0b7mb6675xw10prk"))))
> +    (build-system gnu-build-system)
> +    (inputs
> +     `(("libpng" ,libpng-1.2)

It built fine with the latest libpng, is there a specific reason to use
this version?

> +       ("jasper" ,jasper)))
> +    (native-inputs
> +     `(("autoconf" ,autoconf)
> +       ("automake" ,automake)
> +       ("libtool" ,libtool)))
> +    (arguments
> +     ;; no tests
> +     `(#:tests? #f

Typically we add the "no tests" remark at the end of the line, i.e.
`(#:tests? #f ; No tests.

> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-before 'configure 'autoconf

I would call this phase "autoreconf", since autoconf is another tool.

> +           (lambda _
> +             (zero? (system* "autoreconf" "-vfi")))))))
> +    (home-page "http://icns.sourceforge.net/")
> +    (synopsis "Library for handling Mac OS icns resource files")
> +    (description
> +     "Libicns is a library for the manipulation of Mac OS IconFamily resource
> +type files (ICNS).  @command{icns2png} and @command{png2icns} are provided to
> +convert between PNG and ICNS. @command{icns2png} will extract image files from
> +ICNS files under names like \"Foo_48x48x32.png\" useful for installing for use
> +with .desktop files.  Additionally, @Command{icontainer2png} is provided for
                                      ^^^
                         This probably should not be capitalized.

> +extracting icontainer icon files.")
> +    (license (list license:lgpl2.1+     ; libicns
> +                   license:lgpl2.0+     ; src/apidocs.*
> +                   license:gpl2+))))    ; icns2png, png2icns, icontainer2png

The rest of the patch looks good to me.

Thanks again!
Marius