Re: [PATCH v2] gnu: Add cbatticon.

[Ricardo Wurmus <rekado@elephly.net>]

ng0 <ng0@we.make.ritual.n0.is> writes:

> From ac578d27529cc2a5f39f66054b5991e44e65f0b9 Mon Sep 17 00:00:00 2001
> From: ng0 <ng0@we.make.ritual.n0.is>
> Date: Tue, 9 Aug 2016 16:47:37 +0000
> Subject: [PATCH] gnu: Add cbatticon.

> * gnu/packages/admin.scm (cbatticon): New variable.
> ---
>  gnu/packages/admin.scm | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)

> diff --git a/gnu/packages/admin.scm b/gnu/packages/admin.scm
> index eada796..4025300 100644
> --- a/gnu/packages/admin.scm
> +++ b/gnu/packages/admin.scm
> @@ -12,6 +12,7 @@
>  ;;; Copyright © 2016 Efraim Flashner <efraim@flashner.co.il>
>  ;;; Copyright © 2016 Peter Feigl <peter.feigl@nexoid.at>
>  ;;; Copyright © 2016 John J. Foerch <jjfoerch@earthlink.net>
> +;;; Coypright © 2016 ng0 <ng0@we.make.ritual.n0.is>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -70,7 +71,9 @@
>    #:use-module (gnu packages xorg)
>    #:use-module (gnu packages python)
>    #:use-module (gnu packages man)
> -  #:use-module (gnu packages autotools))
> +  #:use-module (gnu packages autotools)
> +  #:use-module (gnu packages gnome)
> +  #:use-module (gnu packages gtk))
 
>  (define-public aide
>    (package
> @@ -1698,3 +1701,44 @@ a new command using the matched rule, and runs it.")
>  display your disk usage in whatever format you prefer.  It is designed to be
>  highly portable.  Great for heterogenous networks.")
>      (license license:zlib)))
> +
> +(define-public cbatticon
> +  (package
> +    (name "cbatticon")
> +    (version "1.6.4")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append "https://github.com/valr/";
> +                                  name "/archive/" version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "023fvsa4q7rl98rqgwrb1shyzaybdkkbyz5sywd0s5p7ixkksxqx"))
> +              (file-name (string-append name "-" version ".tar.gz"))))
> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:tests? #f ; no make check

Nit-pick: I’d write “no "check" target” or “no tests”.

> +       #:make-flags
> +       (list (string-append "PREFIX=" (assoc-ref %outputs "out"))
> +             "CC=gcc")
> +       #:phases
> +       (modify-phases %standard-phases
> +         (delete 'configure) ; no configure script
> +         (add-before 'build 'patch-paths-in-Makefile
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (lambda _

Why is this a lambda inside of another lambda?  This means that the
substitution really doesn’t happen at build time.  This build phase only
returns a function and then moves on.

> +               (substitute* "Makefile"
> +                 (("msgfmt") (which "msgfmt"))
> +                 (("RM = rm -f")
> +                  (string-append "RM = " (which "rm") " -f")))))))))

These substitutions don’t seem necessary to me.  (Considering that this
doesn’t get executed due to the nested lambda, maybe this is really not
needed.)

> +    (propagated-inputs
> +     `(("libnotify" ,libnotify)))

Why is this propagated?  This shouldn’t be needed.  Propagation is best
avoided.

> +    (inputs
> +     `(("gtk+" ,gtk+)
> +       ("gnu-gettext" ,gnu-gettext)))
> +    (native-inputs
> +     `(("pkg-config" ,pkg-config)))
> +    (synopsis "A lightweigth battery icon for the system tray")

Please don’t begin the synopsis with an article (“A”).

s/lightweigth/lightweight/

> +    (description "cbatticon is a lightweight battery icon that displays
> +the status of your battery.")

How about adding “in the system tray” to the end of the sentence?

> +    (home-page "https://github.com/valr/cbatticon")
> +    (license license:gpl2+)))

Okay.  Could you please send an updated patch?  Thanks!

~~ Ricardo