From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [PATCH] gnu: Add figlet. Date: Sun, 16 Aug 2015 07:07:18 +0200 Message-ID: <87twrzom7t.fsf@elephly.net> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:42514) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZQqAO-0005uR-Hc for guix-devel@gnu.org; Sun, 16 Aug 2015 01:07:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZQqAJ-0008LN-HX for guix-devel@gnu.org; Sun, 16 Aug 2015 01:07:32 -0400 Received: from sender163-mail.zoho.com ([74.201.84.163]:25765) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZQqAJ-0008Je-85 for guix-devel@gnu.org; Sun, 16 Aug 2015 01:07:27 -0400 In-reply-to: List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Steve Sprang Cc: guix-devel@gnu.org 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 > 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