From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Woodcroft Subject: Re: [PATCH] gnu: Add minced. Date: Tue, 16 Aug 2016 14:59:59 +1000 Message-ID: <72c865bb-0ce4-4c51-6029-d8398711135a@uq.edu.au> References: <87d1la12we.fsf@ike.i-did-not-set--mail-host-address--so-tickle-me> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="------------D8F7FAB8DF86063609D33C11" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:41758) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZWU2-0006ZQ-8F for guix-devel@gnu.org; Tue, 16 Aug 2016 01:00:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bZWTy-0007yf-0A for guix-devel@gnu.org; Tue, 16 Aug 2016 01:00:13 -0400 Received: from mailhub2.soe.uq.edu.au ([130.102.132.209]:33172 helo=newmailhub.uq.edu.au) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bZWTx-0007uN-DY for guix-devel@gnu.org; Tue, 16 Aug 2016 01:00:09 -0400 In-Reply-To: <87d1la12we.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 This is a multi-part message in MIME format. --------------D8F7FAB8DF86063609D33C11 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Hi Marius, Excellent to see others interested in packaging microbial bioinformatics tools. 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. 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. 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. > + (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? ben --------------D8F7FAB8DF86063609D33C11 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit

Hi Marius,

Excellent to see others interested in packaging microbial bioinformatics tools.


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.

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.

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.


+    (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?
ben
--------------D8F7FAB8DF86063609D33C11--