From mboxrd@z Thu Jan  1 00:00:00 1970
From: Alex Kost <alezost@gmail.com>
Subject: Re: [PATCH] Add FastTree
Date: Fri, 19 Jun 2015 13:13:08 +0300
Message-ID: <87oakcqa4r.fsf@gmail.com>
References: <5583C3A2.1080906@uq.edu.au>
Mime-Version: 1.0
Content-Type: text/plain
Return-path: <guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org>
Received: from eggs.gnu.org ([2001:4830:134:3::10]:50472)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <alezost@gmail.com>) id 1Z5tIP-0000YG-Qm
	for guix-devel@gnu.org; Fri, 19 Jun 2015 06:13:14 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <alezost@gmail.com>) id 1Z5tIL-0005nz-En
	for guix-devel@gnu.org; Fri, 19 Jun 2015 06:13:13 -0400
Received: from mail-la0-x232.google.com ([2a00:1450:4010:c03::232]:36133)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <alezost@gmail.com>) id 1Z5tIL-0005nX-7K
	for guix-devel@gnu.org; Fri, 19 Jun 2015 06:13:09 -0400
Received: by lacny3 with SMTP id ny3so71171653lac.3
	for <guix-devel@gnu.org>; Fri, 19 Jun 2015 03:13:08 -0700 (PDT)
In-Reply-To: <5583C3A2.1080906@uq.edu.au> (Ben Woodcroft's message of "Fri, 19
	Jun 2015 17:24:18 +1000")
List-Id: "Development of GNU Guix and the GNU System distribution."
	<guix-devel.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/archive/html/guix-devel>
List-Post: <mailto:guix-devel@gnu.org>
List-Help: <mailto:guix-devel-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/guix-devel>,
	<mailto:guix-devel-request@gnu.org?subject=subscribe>
Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org
Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org
To: Ben Woodcroft <b.woodcroft@uq.edu.au>
Cc: "guix-devel@gnu.org" <guix-devel@gnu.org>

Ben Woodcroft (2015-06-19 10:24 +0300) wrote:

> Hi,

Hi, I didn't try your patch, so I don't have real comments, just some
general cosmetic notes, if you don't mind.

[...]
> +   (source (origin
> +             (method url-fetch)
> +             (uri (string-append
> +                   "http://www.microbesonline.org/fasttree/FastTree-"
> +                   version
> +                   ".c"
> +                   ))
Please put it on one line:

                      version ".c"))

> +             (sha256
> +              (base32
> +               "0dzqc9vr9iiiw21y159xfjl2z90vw0y7r4x6456pcaxiy5hd2wmi"))))
> +   (build-system trivial-build-system)
> +   (arguments
> +    '(#:modules ((guix build utils))
> +      #:builder
> +      (begin
> +        (use-modules (guix build utils) (system base compile))
> +        (let ((source (assoc-ref %build-inputs "source"))
> +              (gcc (assoc-ref %build-inputs "gcc"))
> +              (glibc (assoc-ref %build-inputs "glibc"))
> +              (binutils (assoc-ref %build-inputs "binutils"))
> +              (out    (assoc-ref %outputs "out")))          
Please remove trailing spaces                        ^^^^^^^^^^

And it would be more readable to align this 'let':

        (let ((source   (assoc-ref %build-inputs "source"))
              (gcc      (assoc-ref %build-inputs "gcc"))
              (glibc    (assoc-ref %build-inputs "glibc"))
              (binutils (assoc-ref %build-inputs "binutils"))
              (out      (assoc-ref %outputs "out")))


> +          (setenv "PATH" (string-append binutils "/bin:" gcc "/bin"))
> +          (setenv "LIBRARY_PATH" (string-append glibc "/lib"))
> +          (let ((bin (string-append out "/bin")))
> +            (mkdir-p bin)
> +            (system* "gcc"
> +                     "-O3"
> +                     "-finline-functions"
> +                     "-funroll-loops"
> +                     "-Wall"
> +                     "-o"
> +                     (string-append bin "/FastTree")
> +                     source
> +                     "-lm")
> +            (system* "gcc"
> +                     "-DOPENMP"
> +                     "-fopenmp"
> +                     "-O3"
> +                     "-finline-functions"
> +                     "-funroll-loops"
> +                     "-Wall"
> +                     "-o"
> +                     (string-append bin "/FastTreeMP")
> +                     source
> +                     "-lm"))))))
> +   (native-inputs
> +    `(("gcc", gcc-5.1)
> +      ("binutils" ,binutils)
> +      ("glibc" ,glibc)))
> +   (home-page "http://www.microbesonline.org/fasttree")
> +   (synopsis "FastTree infers approximately-maximum-likelihood
> +phylogenetic trees from alignments of nucleotide or protein sequences")

Synopsis is too long; it should be a one-liner.  You may use "guix lint
fasttree" to perform some common checks.

-- 
Alex