Re: [PATCH] gnu: Add libicns

[Marius Bakke <mbakke@fastmail.com>]

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

> From 64ca4155a754a93b26a1002185950f49469197f3 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

Thanks! Nit-pick: we end commit messages and descriptions with a ".",
but this can be fixed by the committer.
  
> +(define-public libicns
> +  (package
> +    (name "libicns")
> +    (version "0.8.1")
> +    (source (origin
> +             (method url-fetch)
> +             (uri (string-append
> +                   "https://sourceforge.net/projects/icns/files/libicns-"

Could you change this to the mirror://sourceforge system?

> +                   version ".tar.gz/download"))
> +             (file-name (string-append "libicns-" version ".tar.gz"))

Changing file-name is only necessary if it doesn't already use a
"package-version.ext" style name, which this appears to do. `guix lint`
will tell whether this is needed.

> +             (sha256 (base32
> +                      "1hjm8lwap7bjyyxsyi94fh5817xzqhk4kb5y0b7mb6675xw10prk"))))
> +    (build-system gnu-build-system)
> +    (arguments
> +     ;; no tests
> +     '(#:tests? #f))
> +    (inputs `(("libpng" ,libpng)
> +              ("jasper" ,jasper)))

We typically add the inputs on a new line, but I have no strong opinions
on this.

> +    (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).  icns2png and png2icns are provided to convert between PNG

When describing command-line tools, it looks prettier in generated
documentation by using `@command(icns2png)`, but this is not always the
case or necessary.

> +and ICNS. icns2png will extract image files from ICNS files under names like
> +\"Foo_48x48x32.png\" useful for installing for use with .desktop files.")
> +    (license (list license:lgpl2.1+  ; libicns
> +                   license:gpl2+)))) ; icns2png, png2icns, icontainer2png

A couple of files are "lgpl2+" as well.

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.

The rest looks fine to me. Can you send an updated patch?

Thanks!
Marius