Re: [PATCH] Add FastTree

[Mark H Weaver]

Ben Woodcroft <address@hidden> writes:
> Updated patch attached.

Much better, thanks!

> In answer to my own question
>> During development of this patch I noticed badly specified system* 
> does not throw an error - is there a way to do this so?
>
> One way is to replace system* with (zero? (system* with that being the
> return value of the lambda - there's an example in the attached patch.

Yes, that's a good option.

Please see below for more comments.

> From d18f50e17547fa741569be109b31ef13c79b3d97 Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Sun, 21 Jun 2015 09:02:26 +1000
> Subject: [PATCH] gnu: Add fasttree

Nitpick: we prefer to add a period "." to the end of the summary line
above.

>
> * gnu/packages/bioinformatics.scm (fasttree): New variable.
> ---
>  gnu/packages/bioinformatics.scm | 60 
> +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>
> diff --git a/gnu/packages/bioinformatics.scm b/gnu/packages/bioinformatics.scm
> index 12c9175..0aacce7 100644
> --- a/gnu/packages/bioinformatics.scm
> +++ b/gnu/packages/bioinformatics.scm
> @@ -780,6 +780,66 @@ analysis (from RNA-Seq), transcription factor binding 
> quantification in
>  ChIP-Seq, and analysis of metagenomic data.")
>      (license license:artistic2.0)))
>  
> +(define-public fasttree
> +  (package
> +   (name "fasttree")
> +   (version "2.1.8")
> +   (source (origin
> +             (method url-fetch)
> +             (uri (string-append
> +                   "http://www.microbesonline.org/fasttree/FastTree-";
> +                   version ".c"
> +                   ))

In the Scheme world, it is considered bad form to leave close
parentheses on their own line.  Please move these to the previous line,
like this:

    version ".c"))

> +        (replace 'build
> +                 (lambda* (#:key source #:allow-other-keys)
> +                   (and (zero? (system* "gcc"
> +                                        "-O3"
> +                                        "-finline-functions"
> +                                        "-funroll-loops"
> +                                        "-Wall"
> +                                        "-o"
> +                                        "FastTree"
> +                                        source
> +                                        "-lm"))
> +                        (zero? (system* "gcc"
> +                                        "-DOPENMP"
> +                                        "-fopenmp"
> +                                        "-O3"
> +                                        "-finline-functions"
> +                                        "-funroll-loops"
> +                                        "-Wall"
> +                                        "-o"
> +                                        "FastTreeMP"
> +                                        source
> +                                        "-lm")))))

Is there any reason to build the single-threaded version?  Maybe it
would be better to omit it, and call the multi-threaded version
FastTree.  What do you think?

    Thanks,
      Mark