From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Woodcroft Subject: Re: [PATCH] gnu: Add minced. Date: Wed, 17 Aug 2016 21:01:38 +1000 Message-ID: <3c37dec8-60f2-fddc-ba2e-5151d2e37f64@uq.edu.au> References: <87d1la12we.fsf@ike.i-did-not-set--mail-host-address--so-tickle-me> <72c865bb-0ce4-4c51-6029-d8398711135a@uq.edu.au> <87zioczycq.fsf@ike.i-did-not-set--mail-host-address--so-tickle-me> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:45885) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZyba-00083R-Lg for guix-devel@gnu.org; Wed, 17 Aug 2016 07:01:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bZybW-0003fz-BD for guix-devel@gnu.org; Wed, 17 Aug 2016 07:01:53 -0400 Received: from mailhub1.soe.uq.edu.au ([130.102.132.208]:37875 helo=newmailhub.uq.edu.au) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZybV-0003fi-Na for guix-devel@gnu.org; Wed, 17 Aug 2016 07:01:50 -0400 In-Reply-To: <87zioczycq.fsf@ike.i-did-not-set--mail-host-address--so-tickle-me> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Marius Bakke , guix-devel@gnu.org On 16/08/16 22:34, Marius Bakke wrote: > Ben Woodcroft writes: > >> Hi Marius, >> >> Excellent to see others interested in packaging microbial bioinformatics >> tools. > You may want to look here before packaging other microbio tools: > > https://github.com/MRC-CLIMB/guix-climb > > I'm currently working on upstreaming most of these :) Cool, and they generally look in quite good shape too. I'll be away next week, but I'd be happy to review them after that. >> I tried this in a container and it seems that it calls out to a few >> programs preventing it from working: >> >> [env]# minced -h >> /gnu/store/8asw2i54x53rrwdr8qw4j2rpbkc9fqzz-profile/bin/minced: line 7: >> dirname: command not found >> /gnu/store/8asw2i54x53rrwdr8qw4j2rpbkc9fqzz-profile/bin/minced: line 9: >> dirname: command not found >> /gnu/store/8asw2i54x53rrwdr8qw4j2rpbkc9fqzz-profile/bin/minced: line 11: >> basename: command not found >> >> So I think that (in order of preference), the source files themselves >> should be patched with the absolute paths to these tools, the binary >> should be wrapped, or coreutils should be propagated. For the first two >> options, coreutils should be an input. > Good catch. Since the "minced" executable is just a wrapper script, I > opted to build my own wrapper to avoid the coreutils dependency. Good idea. > >> Now some small comments on the patch itself. >> >>> + (arguments >>> + `(#:test-target "test" >>> + #:phases >>> + (modify-phases %standard-phases >>> + (delete 'configure) >>> + (add-before 'check 'fix-test >>> + (lambda _ >>> + ;; Fix test for latest version. >>> + (substitute* "t/Aquifex_aeolicus_VF5.expected" >>> + (("minced:0.1.6") "minced:0.2.0")))) >> It might be more future-proof to use '(string-append "minced:" >> ,version)' instead of hard-coding 0.2.0. > I don't think this will be a problem in future releases. And it can also > cause problems, in case a user sets version to e.g. a commit hash. OK. >> Also, this phase (and the next two) should end in #t since the return >> value of substitute* is undefined. >> >> >>> + (add-before 'install 'qualify-java-path >>> + (lambda* (#:key inputs #:allow-other-keys) >>> + (substitute* "minced" >>> + ;; Set full path to java binary in wrapper script. >>> + (("^java") (string-append (assoc-ref inputs "jre") >>> + "/bin/java"))))) >>> + (replace 'install >>> + ;; No install target. >>> + (lambda* (#:key outputs #:allow-other-keys) >>> + (let* ((out (assoc-ref outputs "out")) >>> + (bin (string-append out "/bin"))) >>> + (for-each (lambda (file) >>> + (install-file file bin)) >>> + (list "minced" "minced.jar")))))))) >>> + (native-inputs >>> + `(("jdk", icedtea "jdk"))) >>> + (inputs >>> + `(("jre", icedtea))) >> The commas should go after the space. > Oops, I should stop submitting patches late in the night! > >>> + (home-page"https://github.com/ctSkennerton/minced") >>> + (synopsis "Mining CRISPRs in Environmental Datasets") >>> + (description >>> + "MinCED is a program to find Clustered Regularly Interspaced Short >>> +Palindromic Repeats (CRISPRs) in full genomes or environmental datasets such >>> +as metagenomes, in which sequence size can be anywhere from 100 to 800 bp.") >> That description which you took from the README is a little dated at the >> end. How about this? >> >> "MinCED is a program to find Clustered Regularly Interspaced Short >> Palindromic Repeats (CRISPRs) in both full genomes and shorter metagenomic sequences." >> >> The rest LGTM. Thanks. Can you send an updated patch please? > Please find updated patch below. Thanks for the feedback! Pushed as '318c0ae' after adding a space in the description and only using only using the "out" of icedtea in the inputs. Thanks. ben