Re: [PATCH] Add fraggenescan.

[Ben Woodcroft]

Heya,

On 20/12/15 23:03, Ricardo Wurmus wrote:
> thanks for your patch!
and thank you.
> > +(define-public fraggenescan
> > +  (package
> > +    (name "fraggenescan")
> > +    (version "1.20")
> > +    (source
> > +     (origin
> > +       (method url-fetch)
> > +       (uri
> > +        (string-append "mirror://sourceforge/fraggenescan/"
> > +                       "FragGeneScan" version ".tar.gz"))
> > +       (sha256
> > +        (base32 "1zzigqmvqvjyqv4945kv6nc5ah2xxm1nxgrlsnbzav3f5c0n0pyj"))))
> > +    (build-system gnu-build-system)
> > +    (arguments
> > +     `(#:phases
> > +       (modify-phases %standard-phases
> > +         (delete 'configure)
> > +         (add-before 'build 'patch-run-script
> This phase does more than just patch the run script (it changes paths in
> two scripts and a C source file).  Could you find a better name?
ok, done.
> > +           (lambda* (#:key outputs #:allow-other-keys)
> > +             (let* ((out (string-append (assoc-ref outputs "out")))
> > +                    (share (string-append out "/share/fraggenescan/")))
> > +               (substitute* "run_FragGeneScan.pl"
> > +                 (("system\\(\"rm")
> > +                  (string-append "system(\"" (which "rm")))
> > +                 (("system\\(\"mv")
> > +                  (string-append "system(\"" (which "mv")))
> > +                 ;; This script and other programs expect the training files
> > +                 ;; to be in the non-standard location bin/train/XXX. Change
> > +                 ;; this to be share/fraggenescan/train/XXX instead.
> > +                 (("^\\$train.file = \\$dir.\\\"train/\\\".\\$FGS_train_file;")
> > +                  (string-append "$train_file = \""
> > +                                 share
> > +                                 "train/\".$FGS_train_file;")))
> I might look clearer if you captured a part of the matches,
>
> (("(^\\$train\\.file = )\\$dir\\.\\\"(train/\\\"\\.\\$FGS_train_file;)" _ pre 
> post)
>   (string-append pre "\"" share post))
>
> Or since there’s really just one line beginning with “$train.file” maybe
> you could do this:
>
> (("(^\\$train\\.file = ).*" _ prefix)
>   (string-append prefix "\"" share "train/\".$FGS_train_file;"))
>
> The regular expressions above look quite scary, so maybe the latter
> proposal is best here.
Indeed they are scary, and writing regexes for Guix rarely fails to trip 
me up. I'd actually prefer a less powerful alternative (just a regular 
string replacement), and better yet one that stops the build process 
when it matches 0 or 2 or more times. There's nothing like that around 
is there?

But for now I just took your suggestions about shortening the regexes.
> > +               (substitute* "run_hmm.c"
> > +                 (("^  strcat\\(train_dir, \\\"train/\\\"\\);")
> > +                  (string-append "  strcpy(train_dir, \"" share 
> > "/train/\");")))
> Why do you replace “strcat” with “strcpy” here?
The line above does a strcpy we don't want, so strcat would keep that. I 
could remove the line above if you want, but this effectively makes no 
difference to the running of the program.

[..]
> > +             #t))
> > +         (replace 'build
> > +           (lambda _ (and (zero? (system* "make" "clean"))
> > +                          (zero? (system* "make" "fgs")))))
> Why must “make clean” be run first?  I know the README says so, but is
> it really required?  If it is not you could just use the default build
> phase, possibly specifying “fgs” as the target.
Yeh the tarball comes with compiled files. I've added a comment.
> > +         (delete 'check)
> How about “#:tests? #f” instead?
>
> > +         (replace 'install
> > +           (lambda* (#:key outputs #:allow-other-keys)
> > +             (let* ((out (string-append (assoc-ref outputs "out")))
> > +                    (bin (string-append out "/bin/"))
> > +                    (share (string-append out "/share/fraggenescan/train")))
> > +               (install-file "run_FragGeneScan.pl" bin)
> > +               (install-file "FragGeneScan" bin)
> > +               (install-file "FGS_gff.py" bin)
> > +               (install-file "post_process.pl" bin)
> > +               (copy-recursively "train" share))))
> > +         (add-after 'install 'post-install-check
> > +           ;; In lieu of 'make check', run one of the examples and check the
> > +           ;; output files gets created.
> Oh, I see.  Maybe you could delete the “check” phase right before this,
> so that it’s obvious you are moving it after the “install” phase.
ok, done.
> > +           (lambda* (#:key outputs #:allow-other-keys)
> > +             (let* ((out (string-append (assoc-ref outputs "out")))
> > +                    (bin (string-append out "/bin/")))
> > +               (and (zero? (system* (string-append bin "run_FragGeneScan.pl")
> > +                             "-genome=./example/NC_000913.fna"
> > +                             "-out=./test2"
> > +                             "-complete=1"
> > +                             "-train=complete"))
> > +                    (file-exists? "test2.faa")
> > +                    (file-exists? "test2.ffn")
> > +                    (file-exists? "test2.gff")
> > +                    (file-exists? "test2.out"))))))))
> > +    (inputs
> > +     `(("perl" ,perl)
> > +       ("python" ,python-2))) ;not compatible with python 3.
> > +    (home-page "https://sourceforge.net/projects/fraggenescan/";)
> > +    (synopsis "Finds potentially fragmented genes in short reads")
> > +    (description
> > +     "FragGeneScan is a program for predicting bacterial and archaeal genes in
> > +short and error-prone DNA sequencing reads.  It can also be applied to predict
> > +genes in incomplete assemblies or complete genomes.")
> > +    (license license:gpl1)))
> I didn’t see any mention of a particular GPL version.  The README says
> this:
>
>      License
>      ============
>      Copyright (C) 2010 Mina Rho, Yuzhen Ye and Haixu Tang.
>      You may redistribute this software under the terms of the GNU General 
> Public License.
>
> This looks like it could be any version of the GPL (as it is implied
> when R packages just declare “GPL” as a license).  I would do this, but
> I’m not sure that’s okay:
>
>      ;; Released under any version of the GPL
>      (license license:gpl3+)
It seems your interpretation was better than mine. The authors said 
gpl3+ over email.

Thanks,
ben

0001-gnu-Add-fraggenescan.patch
Description: Text Data