Re: [PATCH] Add diamond

[Mark H Weaver]

Ben Woodcroft <address@hidden> writes:

> From e24f4de21ee3c367c38e431626f4796795eb9e18 Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Wed, 17 Jun 2015 11:49:27 +1000
> Subject: [PATCH] gnu: Add diamond.
>
> * gnu/packages/bioinformatics.scm (diamond): New variable.
> ---
>  gnu/packages/bioinformatics.scm | 46 
> +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/gnu/packages/bioinformatics.scm b/gnu/packages/bioinformatics.scm
> index 0d8a6d5..94db812 100644
> --- a/gnu/packages/bioinformatics.scm
> +++ b/gnu/packages/bioinformatics.scm

Please add a copyright line for yourself to the top of the file.

> @@ -28,6 +28,7 @@
>    #:use-module (guix build-system python)
>    #:use-module (guix build-system trivial)
>    #:use-module (gnu packages)
> +  #:use-module (gnu packages algebra)
>    #:use-module (gnu packages base)
>    #:use-module (gnu packages boost)
>    #:use-module (gnu packages compression)
> @@ -615,6 +616,51 @@ file formats including SAM/BAM, Wiggle/BigWig, BED, 
> GFF/GTF, VCF.")
>  other types of unwanted sequence from high-throughput sequencing reads.")
>      (license license:expat)))
>  
> +(define-public diamond
> +  (package
> +    (name "diamond")
> +    (version "0.7.9")
> +    (source (origin
> +              (method url-fetch)
> +              (uri
> +               (string-append
> +                "https://github.com/bbuchfink/diamond/archive/v"; version 
> ".tar.gz"))

Please add:

              (file-name (string-append name "-" version ".tar.gz"))

to this 'origin' form, because otherwise the filename of the source
tarball in the store will be /gnu/store/…-v0.7.9.tar.gz without the name
of the package.  By default it is named based on the last path component
of the source URI.

> +              (sha256
> +               (base32
> +                "0hfkcfv9f76h5brbyw9fyvmc0l9cmbsxrcdqk0fa9xv82zj47p15"))))

It would be good to remove the pre-compiled binary by adding the
following snippet to the 'origin' form:

--8<---------------cut here---------------start------------->8---
              (snippet '(begin
                          (delete-file "bin/diamond")
                          #t))
--8<---------------cut here---------------end--------------->8---

> +    (build-system gnu-build-system)
> +    (arguments
> +     '(#:tests? #f ;no "check" target
> +             #:phases

Please use spaces instead of tabs, and align "#:phases" directly under
"#:tests?".

> +             (alist-cons-after
> +              'unpack 'enter-source-dir
> +              (lambda _ (chdir "src"))

Phase procedures should return a boolean indicating whether the phase
succeeded, but the return value of 'chdir' is unspecified.  So you
should add #t after (chdir "src").

> +              (alist-replace
> +               'install
> +               (lambda* (#:key outputs #:allow-other-keys)
> +                        (let ((bin (string-append
> +                                    (assoc-ref outputs "out") "/bin")))
> +                          (mkdir-p bin)
> +                          (copy-file "../bin/diamond" (string-append bin 
> "/diamond"))))

Our convention is to have the body of a lambda indented two columns more
than the lambda itself, so the open paren of the 'let' should be under
the 'a' in 'lambda'.  Add #t to the end.  Please try to keep lines <= 80
columns.

> +               (alist-delete 'configure %standard-phases)))))

It would also be good to use our new and improved 'modify-phases'
syntax.  Search for it in gnu/packages/*.scm for examples.

> +    (native-inputs
> +     `(("bc" ,bc)))
> +    (inputs
> +     `(("boost" ,boost)
> +       ("zlib" ,zlib)))
> +    (home-page "https://github.com/bbuchfink/diamond";)
> +    (synopsis "Accelerated BLAST compatible local sequence aligner")
> +    (description
> +     "DIAMOND is a BLAST-compatible local aligner for mapping protein and
> +translated DNA query sequences against a protein reference database (BLASTP
> +and BLASTX alignment mode). The speedup over BLAST is up to 20,000 on short

Please put two spaces between sentences.

> +reads at a typical sensitivity of 90-99% relative to BLAST depending on the
> +data and settings.")
> +    (license (license:non-copyleft (string-append 
> "https://github.com/bbuchfink";
> +                                               "/diamond/blob/v"
> +                                               version
> +                                               "/src/COPYING")))))

    (license (license:non-copyleft "file://src/COPYING"
                                   "See src/COPYING in the distribution."))))

I wouldn't normally do this, but I went ahead and tested it with these
changes and here's what it comes out to:

--8<---------------cut here---------------start------------->8---
(define-public diamond
  (package
    (name "diamond")
    (version "0.7.9")
    (source (origin
              (method url-fetch)
              (uri (string-append
                    "https://github.com/bbuchfink/diamond/archive/v";
                    version ".tar.gz"))
              (file-name (string-append name "-" version ".tar.gz"))
              (sha256
               (base32
                "0hfkcfv9f76h5brbyw9fyvmc0l9cmbsxrcdqk0fa9xv82zj47p15"))
              (snippet '(begin
                          (delete-file "bin/diamond")
                          #t))))
    (build-system gnu-build-system)
    (arguments
     '(#:tests? #f  ;no "check" target
       #:phases
       (modify-phases %standard-phases
         (add-after 'unpack 'enter-source-dir
                    (lambda _
                      (chdir "src")
                      #t))
         (delete 'configure)
         (replace 'install
                  (lambda* (#:key outputs #:allow-other-keys)
                    (let ((bin (string-append (assoc-ref outputs "out")
                                              "/bin")))
                      (mkdir-p bin)
                      (copy-file "../bin/diamond"
                                 (string-append bin "/diamond"))
                      #t))))))
    (native-inputs
     `(("bc" ,bc)))
    (inputs
     `(("boost" ,boost)
       ("zlib" ,zlib)))
    (home-page "https://github.com/bbuchfink/diamond";)
    (synopsis "Accelerated BLAST compatible local sequence aligner")
    (description
     "DIAMOND is a BLAST-compatible local aligner for mapping protein and
translated DNA query sequences against a protein reference database (BLASTP
and BLASTX alignment mode).  The speedup over BLAST is up to 20,000 on short
reads at a typical sensitivity of 90-99% relative to BLAST depending on the
data and settings.")
    (license (license:non-copyleft "file://src/COPYING"
                                   "See src/COPYING in the distribution."))))
--8<---------------cut here---------------end--------------->8---

but feel free to do it a bit differently :)  Anyway, can you send an
updated patch?  Don't forget your copyright notice.

     Thanks!
       Mark