Re: [PATCH] gnu: add the rc shell package

[Mark H Weaver <mhw@netris.org>]

Jeff Mickey <j@codemac.net> writes:

> From 85e9fef8f3eec0b434c909627cd68d5584597aa0 Mon Sep 17 00:00:00 2001
> From: Jeff Mickey <j@codemac.net>
> Date: Thu, 9 Jul 2015 17:39:42 -0700
> Subject: [PATCH] gnu: add the rc shell package

Please make the first summary line:

  gnu: Add rc.

> * gnu/packages/rc.scm: New file.

When adding new files, you need to add an associated line to
'GNU_SYSTEM_MODULES' in gnu-system.am.  Please keep that list sorted.
Then you can add this line to the commit log below the other one:

* gnu-system.am (GNU_SYSTEM_MODULES): Add it.

> ---
>  gnu/packages/rc.scm | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 gnu/packages/rc.scm
>
> diff --git a/gnu/packages/rc.scm b/gnu/packages/rc.scm
> new file mode 100644
> index 0000000..6bd2b51
> --- /dev/null
> +++ b/gnu/packages/rc.scm
> @@ -0,0 +1,77 @@
> +;;; GNU Guix --- Functional package management for GNU
> +;;; Copyright © 2013, 2014, 2015 Ludovic Courtès <ludo@gnu.org>

Please remove Ludovic's copyright line and add your own.

> +;;;
> +;;; 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 rc)
> +  #:use-module (gnu packages autotools)
> +  #:use-module (gnu packages perl)
> +  #:use-module (gnu packages pkg-config)
> +  #:use-module (gnu packages readline)
> +  #:use-module (guix build gnu-build-system)
> +  #:use-module (guix build utils)

You don't need (guix build gnu-build-system) or (guix build utils).

> +  #:use-module (guix build-system gnu)
> +  #:use-module (guix download)

You don't need (guix download) either.

> +  #:use-module (guix git-download)
> +  #:use-module (guix licenses)
> +  #:use-module (guix packages))
> +
> +(define-public rc
> +  (package
> +    (name "rc")
> +    (version "1.7.4")
> +    (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +                    (url "git://github.com/rakitzis/rc.git")
> +                    ;; commit name 'release: rc-1.7.4'
> +                    (commit "c884da53a7c885d46ace2b92de78946855b18e92")))

Please add the following field to the 'origin' form, so that the source
code directory in /gnu/store will have a more descriptive name:

              (file-name (string-append name "-" version "-checkout"))

> +              (sha256
> +               (base32
> +                "00mgzvrrh9w96xa85g4gjbsvq02f08k4jwjcdnxq7kyh5xgiw95l"))))
> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:configure-flags
> +       '("--with-edit=gnu")
> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-before
> +          'configure 'autoreconf

The 'autoreconf' phase should go after 'unpack', rather than before
'configure'.  The reason is that there are some other phases between
'unpack' and 'configure' that will patch things in the generated files,
and those things tend to be important on non-Intel platforms.

> +          (lambda _ (zero? (system* "autoreconf" "-vfi"))))
> +         (add-before
> +          'autoreconf 'rewrite-paths
> +          (lambda _
> +            (substitute* "trip.rc"
> +              (("/bin/pwd") (which "pwd"))
> +              (("/bin/sh") (which "sh")))))

Phase procedures should return a boolean indicating whether the phase
was successfull.  'substitute*' returns an unspecified value.
Therefore, you should add #t after the call to 'substitute*'.

Also, it would be nice to line up right-hand-sides.

> +         ;; this removes a single test which checks that for sure rm is in
> +         ;; /bin/rm
> +         (add-after
> +          'rewrite-paths 'patch-triprc
> +          (lambda _ (zero? (system* "sed" "-i" "282,284d" "trip.rc")))))

Editing the file by removing certain line numbers is too brittle.  When
this package is updated, lines 282-284 may correspond to a different
area of the code, causing it to silently remove the wrong lines.

Also, since both of these phases are patching trip.rc, how about
combining them into one phase?  How about something like this?

--8<---------------cut here---------------start------------->8---
         (add-before
          'autoreconf 'patch-trip.rc
          (lambda _
            (substitute* "trip.rc"
              (("/bin/pwd") (which "pwd"))
              (("/bin/sh")  (which "sh"))
              (("/bin/rm")  (which "rm"))
              (("/bin\\)")  (string-append (dirname (which "rm")) ")")))
            #t)))))
--8<---------------cut here---------------end--------------->8---

This will keep this test intact, but patch it so that it expects 'rm' to
be found where it is located in /gnu/store rather than in /bin.

> +       #:tests? #t)) ;; trip.rc explicity tests for /bin

Please remove the #:tests? #t, since that is the default.  I guess this
comment is outdated also.

> +    (inputs `(("readline" ,readline)
> +              ("perl" ,perl)))
> +    (native-inputs `(("autoconf" ,autoconf)
> +                     ("automake" ,automake)
> +                     ("libtool" ,libtool)
> +                     ("pkg-config" ,pkg-config)))
> +    (synopsis "An alternative implementation of the plan 9 rc shell.")

By convention, the synopsis has no period at the end (since it is not a
sentence), and no article at the beginning.  If you run "guix lint rc"
it will warn you about these issues, and some other problems as well.
How about this instead:

  (synopsis "Alternative implementation of the plan 9 rc shell")

> +    (description
> +     "This is a reimplementation for Unix, by Byron Rakitzis, of the Plan 9

s/Unix/POSIX/

> +shell.  It has a small feature set similar to a traditional Bourne shell.")
> +    (home-page "http://github.com/rakitzis/rc")
> +    (license zlib)))

Can you send an updated patch?

     Thanks,
       Mark