From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [PATCH] gnu: add the rc shell package Date: Fri, 10 Jul 2015 15:59:30 +0200 Message-ID: References: <871tggddtr.fsf@codemac.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:35020) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZDYqH-0005wi-Ob for guix-devel@gnu.org; Fri, 10 Jul 2015 09:59:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZDYq7-0004dl-7a for guix-devel@gnu.org; Fri, 10 Jul 2015 09:59:53 -0400 Received: from pegasus.bbbm.mdc-berlin.de ([141.80.25.20]:43743) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZDYq6-0004aJ-Sh for guix-devel@gnu.org; Fri, 10 Jul 2015 09:59:43 -0400 In-Reply-To: <871tggddtr.fsf@codemac.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: Jeff Mickey Cc: guix-devel@gnu.org Hi Jeff, thanks for the contribution! Below are some comments about your patch, mostly relating to formatting. > From 5deadfb23d8235101220310d0c47626c1d4c219f Mon Sep 17 00:00:00 2001 > From: Jeff Mickey > Date: Thu, 9 Jul 2015 17:39:42 -0700 > Subject: [PATCH] gnu: add the rc shell package > > * gnu/packages/rc.scm (rc): Add the rc package definition > > This patch adds the rc shell package to guix. It is byron's rc, not pla= n9 rc - > and on other distributions 'rc' refers to byron's rc and 'plan9port' or= some > other meta package install the plan9 set of tools which includes rc. > > It has a zlib license. We usually don=E2=80=99t add comments like that to the commit message (in= stead they go into the cover email to the mailing list). Also, when creating a new file we don=E2=80=99t name the variable that is added, but declare = this file to be new. I wonder if for this shell we could have a common module called =E2=80=9Cshells.scm=E2=80=9D where we could merge in =E2=80=9Czsh.scm=E2=80= =9D and possibly other shells. Anyway, here would be a commit message that is more in line with the others: ~~~8<~~~~ gnu: Add the rc shell. * gnu/packages/rc.scm: New file. ~~~8<~~~~ > + (source (origin > + (method url-fetch) > + (uri (string-append "https://github.com/rakitzis/rc/tarb= all/" > + "c884da53a7c885d46ace2b92de78946855b= 18e92")) > + (sha256 > + (base32 "05hlnqcxaw08m1xypk733hajwaap5pr354ndmrm86k0fli= sjk0fw")))) I see that there are no release tarballs. When we take an arbitrary commit we usually add a comment. Also, we normally use the =E2=80=98git-= fetch=E2=80=99 method instead of =E2=80=98url-fetch=E2=80=99. > + (build-system gnu-build-system) > + (arguments `(#:configure-flags > + '("--with-edit=3Dgnu") > + #:phases > + (modify-phases %standard-phases > + (add-before 'configure 'autoreconf (lambda _ > + (zero? (system* "autoreconf" "-vfi"))))) > + #:tests? #f)) Please add a comment to explain why the tests are disabled (no =E2=80=9Cc= heck=E2=80=9D target or failing tests?). The alignment and length of the lines makes it hard to read. How about this instead: (arguments `(#:tests? #f ;no "check" target #:configure-flags '("--with-edit=3Dgnu") #:phases (modify-phases %standard-phases (add-before 'configure 'autoreconf (lambda _ (zero? (system* "autoreconf" "-vfi"))))))) > + (inputs `(("readline" ,readline) > + ("perl" ,perl))) This is oddly aligned. > + (native-inputs `(("autoconf" ,autoconf) > + ("automake" ,automake) > + ("libtool" ,libtool) > + ("pkg-config" ,pkg-config))) Same here. > + (synopsis "An alternative implementation of the plan 9 rc shell.") > + (description > + "This is a reimplementation for Unix, by Byron Rakitzis, of > +the Plan 9 shell. It has a small feature set similar to a traditional = Bourne > +shell, but with a much cleaner and simpler syntax.") Please use two spaces after a period. I=E2=80=99m not sure if the description is okay; =E2=80=9Cmuch cleaner an= d simpler syntax=E2=80=9D sounds a little too partial to me. Note that you can use =E2=80=9Cguix lint=E2=80=9D to check your package d= efinition for the most common problems. ~~ Ricardo