Thanks for the feedback! Here's my 2nd attempt. -Steve
On Sat, Aug 15, 2015 at 10:07 PM, Ricardo Wurmus <rek...@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 <s...@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 > >
From 4e95678b96845418ef5d11a9b1c40f9beaaf85be Mon Sep 17 00:00:00 2001 From: Steve Sprang <s...@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 <s...@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
