Re: [PATCH] gnu: Add minced.

[Ben Woodcroft <b.woodcroft@uq.edu.au>]

On 16/08/16 22:34, Marius Bakke wrote:
> Ben Woodcroft <b.woodcroft@uq.edu.au> 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