[PATCH] gnu: Add figlet.

[PATCH] gnu: Add figlet.

[Steve Sprang]

[-- Attachment #1.1: Type: text/plain, Size: 53 bytes --]

My first Guix package! Feedback appreciated.

-Steve

[-- Attachment #1.2: Type: text/html, Size: 99 bytes --]

[-- Attachment #2: add_figlet.patch --]
[-- Type: text/x-patch, Size: 2935 bytes --]

From 1abd103363bb12e7b8aa5f5ad1329c94b0af48e8 Mon Sep 17 00:00:00 2001
From: Steve Sprang <scs@stevesprang.com>
Date: Sat, 15 Aug 2015 20:08:38 -0700
Subject: [PATCH] gnu: Add figlet.

* gnu/packages/figlet.scm: New file.
* gnu-system.am (GNU_SYSTEM_MODULES): Add it.
---
 gnu-system.am           |  1 +
 gnu/packages/figlet.scm | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)
 create mode 100644 gnu/packages/figlet.scm

diff --git a/gnu-system.am b/gnu-system.am
index 9f46f7b..bd053b7 100644
--- a/gnu-system.am
+++ b/gnu-system.am
@@ -96,6 +96,7 @@ GNU_SYSTEM_MODULES =				\
   gnu/packages/enlightenment.scm		\
   gnu/packages/fcitx.scm			\
   gnu/packages/feh.scm                          \
+  gnu/packages/figlet.scm			\
   gnu/packages/file.scm				\
   gnu/packages/firmware.scm			\
   gnu/packages/fish.scm				\
diff --git a/gnu/packages/figlet.scm b/gnu/packages/figlet.scm
new file mode 100644
index 0000000..7872711
--- /dev/null
+++ b/gnu/packages/figlet.scm
@@ -0,0 +1,49 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2015 Steve Sprang <scs@stevesprang.com>
+;;;
+;;; 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 figlet)
+  #:use-module (guix licenses)
+  #:use-module (guix packages)
+  #:use-module (guix download)
+  #:use-module (guix build-system gnu))
+
+(define-public figlet
+  (package
+    (name "figlet")
+    (version "2.2.5")
+    (source
+     (origin
+       (method url-fetch)
+       (uri (string-append
+             "ftp://ftp.figlet.org/pub/figlet/program/unix/figlet-" version ".tar.gz"))
+       (sha256 (base32
+                "0za1ax15x7myjl8jz271ybly8ln9kb9zhm1gf6rdlxzhs07w925z"))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:phases
+       (alist-replace
+        'configure
+        (lambda* (#:key outputs #:allow-other-keys)
+          (let ((out (assoc-ref outputs "out")))
+            (substitute* "Makefile"
+              (("/usr/local") out))))
+        %standard-phases)))
+    (synopsis "Program for making large letters out of ordinary text")
+    (description "FIGlet is a program for making large letters out of ordinary text.")
+    (home-page "http://www.figlet.org/")
+    (license bsd-3)))
-- 
2.4.3

Re: [PATCH] gnu: Add figlet.

[Ricardo Wurmus]

Hi Steve,

thank you for your first Guix package!  It looks great!  I do have a
couple of cosmetic comments, though.

> From 1abd103363bb12e7b8aa5f5ad1329c94b0af48e8 Mon Sep 17 00:00:00 2001
> From: Steve Sprang <scs@stevesprang.com>
> Date: Sat, 15 Aug 2015 20:08:38 -0700
> Subject: [PATCH] gnu: Add figlet.

> * gnu/packages/figlet.scm: New file.
> * gnu-system.am (GNU_SYSTEM_MODULES): Add it.

Looks good.  I just wonder if figlet could not be added to some existing
module instead of creating a new one.  Maybe “fontutils.scm”?

> +    (version "2.2.5")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (string-append
> +             "ftp://ftp.figlet.org/pub/figlet/program/unix/figlet-" version ".tar.gz"))

This line is a bit long.  How about this instead:

       (uri (string-append "ftp://ftp.figlet.org/pub/figlet/program"
                           "/unix/figlet-" version ".tar.gz"))

> +       (sha256 (base32
> +                "0za1ax15x7myjl8jz271ybly8ln9kb9zhm1gf6rdlxzhs07w925z"))))

I think it would look better like this:

       (sha256
        (base32 "0za1ax15x7myjl8jz271ybly8ln9kb9zhm1gf6rdlxzhs07w925z"))

> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:phases
> +       (alist-replace
> +        'configure
> +        (lambda* (#:key outputs #:allow-other-keys)
> +          (let ((out (assoc-ref outputs "out")))
> +            (substitute* "Makefile"
> +              (("/usr/local") out))))
> +        %standard-phases)))

I suggest using ‘(modify-phases ...)’ instead, as adding or removing
phases later on does not alter the indentation of other phases.

However, in this case ‘/usr/local’ doesn’t have to be patched away at
all.  You could just pass a make-flag to set ‘prefix’ to ‘(assoc-ref
outputs "out")’.

> +    (synopsis "Program for making large letters out of ordinary text")
> +    (description "FIGlet is a program for making large letters out of ordinary text.")

This line is too long.  You can automatically format it in Emacs with
‘M-q’.  Maybe the description could be a little longer to explain that
what figlet generates is some sort of ASCII art letters, because this
description could be misunderstood as operating on fonts.  Or maybe it’s
just me being dense ;)

> +    (home-page "http://www.figlet.org/")
> +    (license bsd-3)))

Looking at the sources it looks like not all files are under the BSD-3
license.  ‘inflate.c’, for example, is released under expat/X11;
‘getopt.c’ is public domain software — I’m not sure if this warrants
using a list for the license field.  Maybe someone else could weigh in
on this.

~~ Ricardo

Re: [PATCH] gnu: Add figlet.

[Steve Sprang]

[-- Attachment #1.1: Type: text/plain, Size: 3016 bytes --]

Thanks for the feedback! Here's my 2nd attempt.

-Steve

On Sat, Aug 15, 2015 at 10:07 PM, Ricardo Wurmus <rekado@elephly.net> wrote:

> Hi Steve,
>
> thank you for your first Guix package!  It looks great!  I do have a
> couple of cosmetic comments, though.
>
> > From 1abd103363bb12e7b8aa5f5ad1329c94b0af48e8 Mon Sep 17 00:00:00 2001
> > From: Steve Sprang <scs@stevesprang.com>
> > Date: Sat, 15 Aug 2015 20:08:38 -0700
> > Subject: [PATCH] gnu: Add figlet.
>
> > * gnu/packages/figlet.scm: New file.
> > * gnu-system.am (GNU_SYSTEM_MODULES): Add it.
>
> Looks good.  I just wonder if figlet could not be added to some existing
> module instead of creating a new one.  Maybe “fontutils.scm”?
>
> > +    (version "2.2.5")
> > +    (source
> > +     (origin
> > +       (method url-fetch)
> > +       (uri (string-append
> > +             "ftp://ftp.figlet.org/pub/figlet/program/unix/figlet-"
> version ".tar.gz"))
>
> This line is a bit long.  How about this instead:
>
>        (uri (string-append "ftp://ftp.figlet.org/pub/figlet/program"
>                            "/unix/figlet-" version ".tar.gz"))
>
> > +       (sha256 (base32
> > +
> "0za1ax15x7myjl8jz271ybly8ln9kb9zhm1gf6rdlxzhs07w925z"))))
>
> I think it would look better like this:
>
>        (sha256
>         (base32 "0za1ax15x7myjl8jz271ybly8ln9kb9zhm1gf6rdlxzhs07w925z"))
>
> > +    (build-system gnu-build-system)
> > +    (arguments
> > +     `(#:phases
> > +       (alist-replace
> > +        'configure
> > +        (lambda* (#:key outputs #:allow-other-keys)
> > +          (let ((out (assoc-ref outputs "out")))
> > +            (substitute* "Makefile"
> > +              (("/usr/local") out))))
> > +        %standard-phases)))
>
> I suggest using ‘(modify-phases ...)’ instead, as adding or removing
> phases later on does not alter the indentation of other phases.
>
> However, in this case ‘/usr/local’ doesn’t have to be patched away at
> all.  You could just pass a make-flag to set ‘prefix’ to ‘(assoc-ref
> outputs "out")’.
>
> > +    (synopsis "Program for making large letters out of ordinary text")
> > +    (description "FIGlet is a program for making large letters out of
> ordinary text.")
>
> This line is too long.  You can automatically format it in Emacs with
> ‘M-q’.  Maybe the description could be a little longer to explain that
> what figlet generates is some sort of ASCII art letters, because this
> description could be misunderstood as operating on fonts.  Or maybe it’s
> just me being dense ;)
>
> > +    (home-page "http://www.figlet.org/")
> > +    (license bsd-3)))
>
> Looking at the sources it looks like not all files are under the BSD-3
> license.  ‘inflate.c’, for example, is released under expat/X11;
> ‘getopt.c’ is public domain software — I’m not sure if this warrants
> using a list for the license field.  Maybe someone else could weigh in
> on this.
>
> ~~ Ricardo
>
>

[-- Attachment #1.2: Type: text/html, Size: 4285 bytes --]

[-- Attachment #2: add-figlet-2.patch --]
[-- Type: text/x-patch, Size: 2898 bytes --]

From 4e95678b96845418ef5d11a9b1c40f9beaaf85be Mon Sep 17 00:00:00 2001
From: Steve Sprang <scs@stevesprang.com>
Date: Sun, 16 Aug 2015 20:43:07 -0700
Subject: [PATCH] gnu: Add figlet.

* gnu/packages/figlet.scm: New file.
* gnu-system.am (GNU_SYSTEM_MODULES): Add it.
---
 gnu-system.am           |  1 +
 gnu/packages/figlet.scm | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)
 create mode 100644 gnu/packages/figlet.scm

diff --git a/gnu-system.am b/gnu-system.am
index 9f46f7b..bd053b7 100644
--- a/gnu-system.am
+++ b/gnu-system.am
@@ -96,6 +96,7 @@ GNU_SYSTEM_MODULES =				\
   gnu/packages/enlightenment.scm		\
   gnu/packages/fcitx.scm			\
   gnu/packages/feh.scm                          \
+  gnu/packages/figlet.scm			\
   gnu/packages/file.scm				\
   gnu/packages/firmware.scm			\
   gnu/packages/fish.scm				\
diff --git a/gnu/packages/figlet.scm b/gnu/packages/figlet.scm
new file mode 100644
index 0000000..f12c0c2
--- /dev/null
+++ b/gnu/packages/figlet.scm
@@ -0,0 +1,47 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2015 Steve Sprang <scs@stevesprang.com>
+;;;
+;;; 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 figlet)
+  #:use-module ((guix licenses) #:prefix license:)
+  #:use-module (guix packages)
+  #:use-module (guix download)
+  #:use-module (guix build-system gnu))
+
+(define-public figlet
+  (package
+    (name "figlet")
+    (version "2.2.5")
+    (source
+     (origin
+       (method url-fetch)
+       (uri (string-append "ftp://ftp.figlet.org/pub/figlet/program"
+                           "/unix/figlet-" version ".tar.gz"))
+       (sha256
+        (base32 "0za1ax15x7myjl8jz271ybly8ln9kb9zhm1gf6rdlxzhs07w925z"))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:phases
+       (modify-phases %standard-phases (delete 'configure))
+       #:make-flags
+       (list (string-append "prefix=" %output))))
+    (home-page "http://www.figlet.org/")
+    (synopsis "Program for making large letterforms out of ordinary screen
+characters")
+    (description "FIGlet is a program for making large ASCII art letterforms
+out of ordinary screen characters.")
+    (license license:bsd-3)))
-- 
2.4.3

Re: [PATCH] gnu: Add figlet.

[Ricardo Wurmus]

Hi Steve,

> Thanks for the feedback! Here's my 2nd attempt.

as others have said on IRC, putting figlet into its own module (for now)
because we don’t have one dedicated to “silly command line tools” is
probably okay :)

The only thing that I’m not entirely happy with is the length of the
synopsis, but I’m probably just too picky.

Unless someone else has any objections, I think this can be accepted.

Thanks!

~~ Ricardo

Re: [PATCH] gnu: Add figlet.

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> The only thing that I’m not entirely happy with is the length of the
> synopsis, but I’m probably just too picky.

I would replace “Program for making” with just “Make”.

> Unless someone else has any objections, I think this can be accepted.

Agreed.  Could you push it?

Thanks,
Ludo’.

Re: [PATCH] gnu: Add figlet.

[Ricardo Wurmus]

Ludovic Courtès <ludo@gnu.org> writes:

> Ricardo Wurmus <rekado@elephly.net> skribis:
>
>> The only thing that I’m not entirely happy with is the length of the
>> synopsis, but I’m probably just too picky.
>
> I would replace “Program for making” with just “Make”.

Done.

>> Unless someone else has any objections, I think this can be accepted.
>
> Agreed.  Could you push it?

Pushed!

~~ Ricardo