Re: [PATCH] gnu: Add freebayes.

[Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de>]

Roel Janssen <roel@gnu.org> writes:

> One of the problems with the patch is probably the bulk of dependencies
> dragged in (for example, vcflib).  They use specific versions so they
> are tied to this package (so that's why I cannot package them separately).

Yeah, this is worrying.

> From 38302e8cac77275694c8793933be414ec26906ec Mon Sep 17 00:00:00 2001
> From: Roel Janssen <roel@gnu.org>
> Date: Tue, 8 Mar 2016 16:38:46 +0100
> Subject: [PATCH] gnu: Add freebayes.

> * gnu/packages/bioinformatics.scm (freebayes): New variable.

[...]

> +(define-public freebayes
> +  (let ((commit "3ce827d8ebf89bb3bdc097ee0fe7f46f9f30d5fb"))
> +    (package
> +      (name "freebayes")
> +      (version (string-append "v1.0.2-" (string-take commit 7)))

The version should not start with “v” and it should include a numeric
revision because later git commits may be sorted lower than the current
commit.

(let ((commit ....)
      (revision "1"))
  ...
  (version (string-append "1.0.2-" revision "." commit))
  ...)

Why does it have to be a git clone, though?  I see that only six commits
were made to master since release 1.0.2.  If fetching from git must
happen it would be good to have a reason in a comment.

> +      (native-inputs
> +       `(("cmake" ,cmake)
> +         ("htslib" ,htslib)
> +         ("zlib" ,zlib)

“htslib” and “zlib” sound like regular inputs.

> +         ("python" ,python-2)
> +         ("perl" ,perl)

These maybe as well.

> +         ("bamtools-src"
> +          ,(origin
> +             (method url-fetch)
> +             (uri (string-append "https://github.com/ekg/bamtools/archive/"
> +                  "e77a43f5097ea7eee432ee765049c6b246d49baa" ".tar.gz"))
> +             (file-name "bamtools-src.tar.gz")
> +             (sha256
> +              (base32 "0rqymka21g6lfjfgxzr40pxz4c4fcl77jpy1np1li70pnc7h2cs1"))))

We already have bamtools, I think.  Is there no way to link with that
version?  Does it have to be this arbitrary-looking commit?

> +         ("vcflib-src"
> +          ,(origin
> +             (method url-fetch)
> +             (uri (string-append "https://github.com/vcflib/vcflib/archive/"
> +                  "5ac091365fdc716cc47cc5410bb97ee5dc2a2c92" ".tar.gz"))
> +             (file-name "vcflib-5ac0913.tar.gz")
> +             (sha256
> +              (base32 "0ywshwpif059z5h0g7zzrdfzzdj2gr8xvwlwcsdxrms3p9iy35h8"))))

> +         ;; These are submodules for the vcflib version used in freebayes
> +         ("tabixpp-src"
> +         ("intervaltree-src"
> +         ("smithwaterman-src"
> +         ("multichoose-src"
> +         ("fsom-src"
> +         ("filevercmp-src"
> +         ("fastahack-src"

If these are submodules of this particular version of vcflib I think it
would be better to create a separate vcflib package where these
submodules are included.  If ever possible you would then coerce
freebayes to link with that version of vcflib.

Note that vcflib doesn’t *have* to be exported via define-public.  It
would be nice, though, if we could get a regular vcflib package as a
side-effect of all this.

> +      (arguments
> +       `(#:tests? #f

Please quickly comment on why the tests are disabled.

> +         #:phases
> +         (modify-phases %standard-phases
> +           (delete 'configure)
> +           (delete 'check)

You won’t need this when tests are disabled already.

> +           (add-after 'unpack 'unpack-submodule-sources
> +             (lambda* (#:key inputs #:allow-other-keys)
> +               (let ((unpack (lambda (source target)
> +                               (with-directory-excursion target
> +                                 (zero? (system* "tar" "xvf"
> +                                                 (assoc-ref inputs source)
> +                                                 "--strip-components=1"))))))
> +                 (and
> +                  (unpack "bamtools-src" "bamtools")
> +                  (unpack "vcflib-src" "vcflib")
> +                  (unpack "intervaltree-src" "intervaltree")
> +                  (unpack "fastahack-src" "vcflib/fastahack")
> +                  (unpack "filevercmp-src" "vcflib/filevercmp")
> +                  (unpack "fsom-src" "vcflib/fsom")
> +                  (unpack "intervaltree-src" "vcflib/intervaltree")
> +                  (unpack "multichoose-src" "vcflib/multichoose")
> +                  (unpack "smithwaterman-src" "vcflib/smithwaterman")
> +                  (unpack "tabixpp-src" "vcflib/tabixpp")))))
> +           (add-after 'unpack-submodule-sources 'fix-makefile
> +             (lambda* (#:key inputs #:allow-other-keys)
> +               ;; We don't have the .git folder to get the version tag from.
> +               ;; For this checkout of the code, it's v1.0.0.
> +               (substitute* '("vcflib/Makefile")
> +                 (("^GIT_VERSION.*") "GIT_VERSION = v1.0.0"))))
> +           (replace
> +            'build
> +            (lambda* (#:key inputs make-flags #:allow-other-keys)
> +              (and
> +               ;; Compile Bamtools before compiling the main project.
> +               (with-directory-excursion "bamtools"
> +                 (system* "mkdir" "build")
> +                 (with-directory-excursion "build"
> +                   (and (zero? (system* "cmake" "../"))
> +                        (zero? (system* "make")))))
> +               ;; Compile vcflib before we compiling the main project.
> +               (with-directory-excursion "vcflib"
> +                 (with-directory-excursion "tabixpp"
> +                   (zero? (system* "make")))
> +                 (zero? (system* "make" "CC=gcc"
> +                   (string-append "CFLAGS=\"" "-Itabixpp "
> +                     "-I" (assoc-ref inputs "htslib") "/include " "\"") "all")))
> +               (with-directory-excursion "src"
> +                 (zero? (system* "make"))))))

This seems too hackish for my taste.  It would be so much nicer if
bamtools and vcflib were built in separate packages.  This would make it
much clearer what hacks are required for what package and you could use
the cmake-build-system for bamtools and the gnu-build-system for vcflib.

You might be able to force freebayes to use those separate packages by
overriding BAMTOOLS_ROOT and VCFLIB_ROOT in “src/Makefile” or by
replacing “$(BAMTOOLS_ROOT)/lib/libbamtools.a” with the path to the
actual “libbamtools.a” in the store.  Looking at “src/Makefile” the
entanglement isn’t that bad and we should be able to resolve it.

If you need assistance with this I could help you.

> +           (replace
> +            'install

Please leave “'install” on the same line.

~~ Ricardo