From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Sprang Subject: Re: [PATCH] gnu: Add figlet. Date: Mon, 17 Aug 2015 15:47:14 -0700 Message-ID: References: <87twrzom7t.fsf@elephly.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary=001a1134f93e39d850051d899598 Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:53513) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZRTBW-0001Rd-Mw for guix-devel@gnu.org; Mon, 17 Aug 2015 18:47:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZRTBU-0006PR-Kv for guix-devel@gnu.org; Mon, 17 Aug 2015 18:47:18 -0400 Received: from mail-oi0-x236.google.com ([2607:f8b0:4003:c06::236]:34554) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZRTBU-0006PF-Bq for guix-devel@gnu.org; Mon, 17 Aug 2015 18:47:16 -0400 Received: by oip136 with SMTP id 136so89708241oip.1 for ; Mon, 17 Aug 2015 15:47:14 -0700 (PDT) In-Reply-To: <87twrzom7t.fsf@elephly.net> 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: Ricardo Wurmus Cc: guix-devel@gnu.org --001a1134f93e39d850051d899598 Content-Type: multipart/alternative; boundary=001a1134f93e39d84a051d899596 --001a1134f93e39d84a051d899596 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Thanks for the feedback! Here's my 2nd attempt. -Steve On Sat, Aug 15, 2015 at 10:07 PM, Ricardo Wurmus 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 > > 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 =E2=80=9Cfontutils.scm=E2=80= =9D? > > > + (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 =E2=80=98(modify-phases ...)=E2=80=99 instead, as adding = or removing > phases later on does not alter the indentation of other phases. > > However, in this case =E2=80=98/usr/local=E2=80=99 doesn=E2=80=99t have t= o be patched away at > all. You could just pass a make-flag to set =E2=80=98prefix=E2=80=99 to = =E2=80=98(assoc-ref > outputs "out")=E2=80=99. > > > + (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 > =E2=80=98M-q=E2=80=99. 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=E2= =80=99s > 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. =E2=80=98inflate.c=E2=80=99, for example, is released under exp= at/X11; > =E2=80=98getopt.c=E2=80=99 is public domain software =E2=80=94 I=E2=80=99= m not sure if this warrants > using a list for the license field. Maybe someone else could weigh in > on this. > > ~~ Ricardo > > --001a1134f93e39d84a051d899596 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
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!=C2=A0 It looks great!=C2=A0 I do hav= e a
couple of cosmetic comments, though.

> From 1abd103363bb12e7b8aa5f5ad1329c94b0af48e8 Mon Sep 17 00:00:00 2001=
> From: Steve Sprang <scs@stev= esprang.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.=C2=A0 I just wonder if figlet could not be added to some existi= ng
module instead of creating a new one.=C2=A0 Maybe =E2=80=9Cfontutils.scm=E2= =80=9D?

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

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

=C2=A0 =C2=A0 =C2=A0 =C2=A0(uri (string-append "ftp://ft= p.figlet.org/pub/figlet/program"
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0"/unix/figlet-" version ".tar.gz&quo= t;))

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(sha256 (base32
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "0za1ax1= 5x7myjl8jz271ybly8ln9kb9zhm1gf6rdlxzhs07w925z"))))

I think it would look better like this:

=C2=A0 =C2=A0 =C2=A0 =C2=A0(sha256
=C2=A0 =C2=A0 =C2=A0 =C2=A0 (base32 "0za1ax15x7myjl8jz271ybly8ln9kb9zh= m1gf6rdlxzhs07w925z"))

> +=C2=A0 =C2=A0 (build-system gnu-build-system)
> +=C2=A0 =C2=A0 (arguments
> +=C2=A0 =C2=A0 =C2=A0`(#:phases
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0(alist-replace
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 'configure
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 (lambda* (#:key outputs #:allow-other-key= s)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (let ((out (assoc-ref outputs &quo= t;out")))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (substitute* "Makefile= "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (("/usr/local&q= uot;) out))))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 %standard-phases)))

I suggest using =E2=80=98(modify-phases ...)=E2=80=99 instead, as adding or= removing
phases later on does not alter the indentation of other phases.

However, in this case =E2=80=98/usr/local=E2=80=99 doesn=E2=80=99t have to = be patched away at
all.=C2=A0 You could just pass a make-flag to set =E2=80=98prefix=E2=80=99 = to =E2=80=98(assoc-ref
outputs "out")=E2=80=99.

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

This line is too long.=C2=A0 You can automatically format it in Emacs with<= br> =E2=80=98M-q=E2=80=99.=C2=A0 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.=C2=A0 Or maybe it= =E2=80=99s
just me being dense ;)

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

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

~~ Ricardo


--001a1134f93e39d84a051d899596-- --001a1134f93e39d850051d899598 Content-Type: text/x-patch; charset=UTF-8; name="add-figlet-2.patch" Content-Disposition: attachment; filename="add-figlet-2.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_idgigk8b0 RnJvbSA0ZTk1Njc4Yjk2ODQ1NDE4ZWY1ZDExYTliMWM0MGY5YmVhYWY4NWJlIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBTdGV2ZSBTcHJhbmcgPHNjc0BzdGV2ZXNwcmFuZy5jb20+CkRh dGU6IFN1biwgMTYgQXVnIDIwMTUgMjA6NDM6MDcgLTA3MDAKU3ViamVjdDogW1BBVENIXSBnbnU6 IEFkZCBmaWdsZXQuCgoqIGdudS9wYWNrYWdlcy9maWdsZXQuc2NtOiBOZXcgZmlsZS4KKiBnbnUt c3lzdGVtLmFtIChHTlVfU1lTVEVNX01PRFVMRVMpOiBBZGQgaXQuCi0tLQogZ251LXN5c3RlbS5h bSAgICAgICAgICAgfCAgMSArCiBnbnUvcGFja2FnZXMvZmlnbGV0LnNjbSB8IDQ3ICsrKysrKysr KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrCiAyIGZpbGVzIGNoYW5nZWQs IDQ4IGluc2VydGlvbnMoKykKIGNyZWF0ZSBtb2RlIDEwMDY0NCBnbnUvcGFja2FnZXMvZmlnbGV0 LnNjbQoKZGlmZiAtLWdpdCBhL2dudS1zeXN0ZW0uYW0gYi9nbnUtc3lzdGVtLmFtCmluZGV4IDlm NDZmN2IuLmJkMDUzYjcgMTAwNjQ0Ci0tLSBhL2dudS1zeXN0ZW0uYW0KKysrIGIvZ251LXN5c3Rl bS5hbQpAQCAtOTYsNiArOTYsNyBAQCBHTlVfU1lTVEVNX01PRFVMRVMgPQkJCQlcCiAgIGdudS9w YWNrYWdlcy9lbmxpZ2h0ZW5tZW50LnNjbQkJXAogICBnbnUvcGFja2FnZXMvZmNpdHguc2NtCQkJ XAogICBnbnUvcGFja2FnZXMvZmVoLnNjbSAgICAgICAgICAgICAgICAgICAgICAgICAgXAorICBn bnUvcGFja2FnZXMvZmlnbGV0LnNjbQkJCVwKICAgZ251L3BhY2thZ2VzL2ZpbGUuc2NtCQkJCVwK ICAgZ251L3BhY2thZ2VzL2Zpcm13YXJlLnNjbQkJCVwKICAgZ251L3BhY2thZ2VzL2Zpc2guc2Nt CQkJCVwKZGlmZiAtLWdpdCBhL2dudS9wYWNrYWdlcy9maWdsZXQuc2NtIGIvZ251L3BhY2thZ2Vz L2ZpZ2xldC5zY20KbmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMC4uZjEyYzBjMgot LS0gL2Rldi9udWxsCisrKyBiL2dudS9wYWNrYWdlcy9maWdsZXQuc2NtCkBAIC0wLDAgKzEsNDcg QEAKKzs7OyBHTlUgR3VpeCAtLS0gRnVuY3Rpb25hbCBwYWNrYWdlIG1hbmFnZW1lbnQgZm9yIEdO VQorOzs7IENvcHlyaWdodCDCqSAyMDE1IFN0ZXZlIFNwcmFuZyA8c2NzQHN0ZXZlc3ByYW5nLmNv bT4KKzs7OworOzs7IFRoaXMgZmlsZSBpcyBwYXJ0IG9mIEdOVSBHdWl4LgorOzs7Cis7OzsgR05V IEd1aXggaXMgZnJlZSBzb2Z0d2FyZTsgeW91IGNhbiByZWRpc3RyaWJ1dGUgaXQgYW5kL29yIG1v ZGlmeSBpdAorOzs7IHVuZGVyIHRoZSB0ZXJtcyBvZiB0aGUgR05VIEdlbmVyYWwgUHVibGljIExp Y2Vuc2UgYXMgcHVibGlzaGVkIGJ5Cis7OzsgdGhlIEZyZWUgU29mdHdhcmUgRm91bmRhdGlvbjsg ZWl0aGVyIHZlcnNpb24gMyBvZiB0aGUgTGljZW5zZSwgb3IgKGF0Cis7OzsgeW91ciBvcHRpb24p IGFueSBsYXRlciB2ZXJzaW9uLgorOzs7Cis7OzsgR05VIEd1aXggaXMgZGlzdHJpYnV0ZWQgaW4g dGhlIGhvcGUgdGhhdCBpdCB3aWxsIGJlIHVzZWZ1bCwgYnV0Cis7OzsgV0lUSE9VVCBBTlkgV0FS UkFOVFk7IHdpdGhvdXQgZXZlbiB0aGUgaW1wbGllZCB3YXJyYW50eSBvZgorOzs7IE1FUkNIQU5U QUJJTElUWSBvciBGSVRORVNTIEZPUiBBIFBBUlRJQ1VMQVIgUFVSUE9TRS4gIFNlZSB0aGUKKzs7 OyBHTlUgR2VuZXJhbCBQdWJsaWMgTGljZW5zZSBmb3IgbW9yZSBkZXRhaWxzLgorOzs7Cis7Ozsg WW91IHNob3VsZCBoYXZlIHJlY2VpdmVkIGEgY29weSBvZiB0aGUgR05VIEdlbmVyYWwgUHVibGlj IExpY2Vuc2UKKzs7OyBhbG9uZyB3aXRoIEdOVSBHdWl4LiAgSWYgbm90LCBzZWUgPGh0dHA6Ly93 d3cuZ251Lm9yZy9saWNlbnNlcy8+LgorCisoZGVmaW5lLW1vZHVsZSAoZ251IHBhY2thZ2VzIGZp Z2xldCkKKyAgIzp1c2UtbW9kdWxlICgoZ3VpeCBsaWNlbnNlcykgIzpwcmVmaXggbGljZW5zZTop CisgICM6dXNlLW1vZHVsZSAoZ3VpeCBwYWNrYWdlcykKKyAgIzp1c2UtbW9kdWxlIChndWl4IGRv d25sb2FkKQorICAjOnVzZS1tb2R1bGUgKGd1aXggYnVpbGQtc3lzdGVtIGdudSkpCisKKyhkZWZp bmUtcHVibGljIGZpZ2xldAorICAocGFja2FnZQorICAgIChuYW1lICJmaWdsZXQiKQorICAgICh2 ZXJzaW9uICIyLjIuNSIpCisgICAgKHNvdXJjZQorICAgICAob3JpZ2luCisgICAgICAgKG1ldGhv ZCB1cmwtZmV0Y2gpCisgICAgICAgKHVyaSAoc3RyaW5nLWFwcGVuZCAiZnRwOi8vZnRwLmZpZ2xl dC5vcmcvcHViL2ZpZ2xldC9wcm9ncmFtIgorICAgICAgICAgICAgICAgICAgICAgICAgICAgIi91 bml4L2ZpZ2xldC0iIHZlcnNpb24gIi50YXIuZ3oiKSkKKyAgICAgICAoc2hhMjU2CisgICAgICAg IChiYXNlMzIgIjB6YTFheDE1eDdteWpsOGp6MjcxeWJseThsbjlrYjl6aG0xZ2Y2cmRseHpoczA3 dzkyNXoiKSkpKQorICAgIChidWlsZC1zeXN0ZW0gZ251LWJ1aWxkLXN5c3RlbSkKKyAgICAoYXJn dW1lbnRzCisgICAgIGAoIzpwaGFzZXMKKyAgICAgICAobW9kaWZ5LXBoYXNlcyAlc3RhbmRhcmQt cGhhc2VzIChkZWxldGUgJ2NvbmZpZ3VyZSkpCisgICAgICAgIzptYWtlLWZsYWdzCisgICAgICAg KGxpc3QgKHN0cmluZy1hcHBlbmQgInByZWZpeD0iICVvdXRwdXQpKSkpCisgICAgKGhvbWUtcGFn ZSAiaHR0cDovL3d3dy5maWdsZXQub3JnLyIpCisgICAgKHN5bm9wc2lzICJQcm9ncmFtIGZvciBt YWtpbmcgbGFyZ2UgbGV0dGVyZm9ybXMgb3V0IG9mIG9yZGluYXJ5IHNjcmVlbgorY2hhcmFjdGVy cyIpCisgICAgKGRlc2NyaXB0aW9uICJGSUdsZXQgaXMgYSBwcm9ncmFtIGZvciBtYWtpbmcgbGFy Z2UgQVNDSUkgYXJ0IGxldHRlcmZvcm1zCitvdXQgb2Ygb3JkaW5hcnkgc2NyZWVuIGNoYXJhY3Rl cnMuIikKKyAgICAobGljZW5zZSBsaWNlbnNlOmJzZC0zKSkpCi0tIAoyLjQuMwoK --001a1134f93e39d850051d899598--