On Thu, 22 Oct 2015 11:17:11 -0400
Mark H Weaver <mhw@netris.org> wrote:

> Efraim Flashner <efraim@flashner.co.il> writes:
> 
> > efraim pushed a commit to branch master
> > in repository guix.
> >
> > commit 5d47eab0242d6f89a6837123141acdae68745328
> > Author: Efraim Flashner <efraim@flashner.co.il>
> > Date:   Thu Oct 22 13:12:07 2015 +0300
> >
> >     gnu: Add pbzip2.
> >     
> >     * gnu/packages/compression.scm (pbzip2): New variable.  
> 
> Thanks for this, but did you post it to guix-devel for review?
> It would be good to do so in the future.
Oops, definately forgot that this time. I'll be better about that in the future.

> 
> Please see below for comments.
> 
> > ---
> >  gnu/packages/compression.scm |   33 +++++++++++++++++++++++++++++++++
> >  1 files changed, 33 insertions(+), 0 deletions(-)
> >
> > diff --git a/gnu/packages/compression.scm b/gnu/packages/compression.scm
> > index 941844b..0bb3919 100644
> > --- a/gnu/packages/compression.scm
> > +++ b/gnu/packages/compression.scm
> > @@ -7,6 +7,7 @@
> >  ;;; Copyright © 2015 Ricardo Wurmus <rekado@elephly.net>
> >  ;;; Copyright © 2015 Leo Famulari <leo@famulari.name>
> >  ;;; Copyright © 2015 Jeff Mickey <j@codemac.net>
> > +;;; Copyright © 2015 Efraim Flashner <efraim@flashner.co.il>
> >  ;;;
> >  ;;; This file is part of GNU Guix.
> >  ;;;
> > @@ -225,6 +226,38 @@ decompression.")
> >                                    "See LICENSE in the distribution."))
> >        (home-page "http://www.bzip.org/"))))
> >  
> > +(define-public pbzip2
> > +  (package
> > +    (name "pbzip2")
> > +    (version "1.1.12")
> > +    (source (origin
> > +             (method url-fetch)
> > +             (uri (string-append "https://launchpad.net/pbzip2/1.1/" version
> > +                                "/+download/" name "-" version ".tar.gz"))  
> 
> The quote (") on the line above should be aligned with the quote on the
> line above it.
Ok

> 
> Also, instead of hard coding "1.1" in the URI, please use
> 'version-major+minor' instead.  You'll find many examples of it in
> gnu/packages/*.scm
Ok, found one :). I wasn't sure what (version-major+minor version) was before

> 
> > +             (sha256
> > +              (base32
> > +               "1vk6065dv3a47p86vmp8hv3n1ygd9hraz0gq89gvzlx7lmcb6fsp"))))
> > +    (build-system gnu-build-system)
> > +    (inputs
> > +     `(("bzip2", bzip2)))
> > +    (arguments
> > +     `(#:tests? #f ; no tests
> > +       #:phases (modify-phases %standard-phases
> > +                  (replace 'configure
> > +                           (lambda* (#:key outputs #:allow-other-keys)
> > +                                    (substitute* "Makefile"
> > +                                    (("/usr") (assoc-ref outputs "out")))
> > +                                    #t)))))  
> 
> The alignment of the lines above is very confusing, to say the least.
> 
> Anyway, it would be better to simply remove the 'configure' phase and
> instead add this:
> 
>   #:make-flags (list (string-append "PREFIX=" %output))
That sure is shorter than what I had there before. fixed.

> 
> > +    (home-page "http://compression.ca/pbzip2/")
> > +    (synopsis "Parallel bzip2 implementation")
> > +    (description
> > +     "Pbzip2 is a parallel implementation of the bzip2 block-sorting file
> > +compressor that uses pthreads and achieves near-linear speedup on SMP machines.
> > +The output of this version is fully compatible with bzip2 v1.0.2 (ie: anything
> > +compressed with pbzip2 can be decompressed with bzip2).")  
> 
> s/ie:/i.e./
ok

> 
> > +    (license (license:non-copyleft "file://COPYING"
> > +                                "See COPYING in the distribution."))))  
> 
> Please align the open quotes.
ok

> 
>      Thanks,
>        Mark

I'll hold onto my patch for a little bit longer to see if anyone else has any
suggestions, and then I'll push the fixes.

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted