Re: [PATCH] Add procmail

[Lukas Gradl <lgradl@openmailbox.org>]

On Mon, Feb 29, 2016 at 02:54:09PM +0100, Andy Wingo wrote:
> Hi Lukas,
> 
> On Sat 27 Feb 2016 10:16, Lukas Gradl <lgradl@openmailbox.org> writes:
> 
> > I am new to Guix and Scheme, so I would very much welcome any comments
> > you might have.
> 
> Welcome :)

Thank you!

> 
> > +       ;; the following patch fixes an ambiguous definition of
> > +       ;; getline() in formail.c. The patch is provided by Debian as
> > +       ;; patch 24
> 
> We tend to prefer full sentences in comments like this one, including
> ending punctuation.  Also as a GNU-ism, we are in the church of
> two-spaces-after-full-stops :)
>

OK, sounds good. Fixed.

> > +    (arguments
> > +     `(#:phases (alist-replace
> > +                 'configure
> > +                 (lambda _
> > +                   (substitute* "Makefile"
> > +                     (("/bin/sh")
> > +                      (which "sh")))
> > +                   (substitute* "Makefile"
> > +                     (("/usr")
> > +                      (assoc-ref %outputs "out")))
> > +                   (substitute* "Makefile"
> > +                     (("/bin/rm")
> > +                      (which "rm"))))
> > +                 %standard-phases)
> 
> There are many packages that still use this syntax, but the current
> Best Practice™ is to use `modify-phases'.  Search Guix for some
> examples.

I changed the patch to use 'modify-phases'. Thanks for pointing this
out, I like this much better than the 'alist-replace'!

> Also, substitute* can do all these clauses in one:
> 
>   (substitute* "Makefile"
>     (("/bin/sh")
>      (which "sh"))
>     (("/usr")
>      (assoc-ref %outputs "out"))
>     (("/bin/rm")
>      (which "rm")))
> 
> The * in substitute* is like the * in let*, meaning the substitutions
> will be done in order.  Also, the return value of substitute* is
> unspecified (see the Guile manual for more), so be sure to return a true
> value from this lambda by just ending the lambda with #t.
>

I updated this. Thank you for the explanation!

> > +       #:tests? #f)) ; no tests
> 
> Please mention why the tests are disabled.

I think procmail comes without tests.  There are no targets like 'tests'
or 'check' in the Makefile.  However, it does perform some test during
'make install' to verify if the filesystem has locking
capabilities.  These are performed before the actual build, so they do
not check if the build was successful.  I changed the comment to better
explain the situation.

> 
> > +    (build-system gnu-build-system)
> > +    (inputs `(("glibc" ,glibc)
> > +              ("exim" ,exim)))
> > +    (home-page "http://www.procmail.org/")
> > +    (synopsis "Versatile mail delivery agent (MDA)")
> > +    (description "Procmail is a mail delivery agent (MDA) featuring support
> > +for a variety of mailbox formats such as mbox, mh and maildir.  Incoming mail
> > +can be sorted into separate files/directories and arbitrary commands can be
> > +executed on mail arrival.  Procmail is considered stable, but is no longer
> > +maintained.")
> > +    (license gpl2+))) ; procmail allows to choose the
> > +                      ; nonfree Artistic License 1.0
> > +                      ; as alternative to the GPL2+.
> > +                      ; This option is not listed here.
> 
> This sort of a comment is best done as a two-semicolon comment, I think.
> 
> Looking pretty good!
>

Thank you! This was the last thing keeping me from switching to GuixSD.

I will send an updated patch shortly.

Thank you for your review!

Best,
Lukas