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 :)

> 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.

> 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.

> 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!