Re: 01/01: gnu: Add pbzip2.

[Mark H Weaver]

Hi Efraim,

Efraim Flashner <address@hidden> writes:

> On Thu, 22 Oct 2015 11:17:11 -0400
> Mark H Weaver <address@hidden> wrote:
>
>> Efraim Flashner <address@hidden> writes:
>> 
>> > efraim pushed a commit to branch master
>> > in repository guix.
>> >
>> > commit 5d47eab0242d6f89a6837123141acdae68745328
>> > Author: Efraim Flashner <address@hidden>
>> > 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 <address@hidden>
>> >  ;;; Copyright © 2015 Leo Famulari <address@hidden>
>> >  ;;; Copyright © 2015 Jeff Mickey <address@hidden>
>> > +;;; Copyright © 2015 Efraim Flashner <address@hidden>
>> >  ;;;
>> >  ;;; 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.

It's been a week with no further suggestions, so I suggest that you
proceed to push the fixes.

     Thanks,
       Mark