diff mbox series

[bug#44613] Fix build for bedtools

Message ID a82f869c91b6b33a5d063b505cab01e19b53b84d.camel@gnu.org
State Accepted
Headers show
Series [bug#44613] Fix build for bedtools | expand

Checks

Context Check Description
cbaines/submitting builds success
cbaines/issue success View issue
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job

Commit Message

Roel Janssen Nov. 13, 2020, 11:01 a.m. UTC
Dear Guix,

By updating samtools to 1.11, I introduced a build failure for
bedtools. More precisely, the tests for intersect break in precisely
this way:
https://github.com/arq5x/bedtools2/issues/814

With the following patches, I'd like to add samtools-1.9, htslib-1.9
(samtools depends on that) to fix this problem with bedtools.

Alternatively we could add a patch to disable the failing bedtools
tests.  I manually inspected the test results, and seem to match
perfectly (indicating that there's no problem with bedtools).

Kind regards,
Roel Janssen

Comments

zimoun Nov. 13, 2020, 12:39 p.m. UTC | #1
Hi Roel,

On Fri, 13 Nov 2020 at 12:01, Roel Janssen <roel@gnu.org> wrote:

> With the following patches, I'd like to add samtools-1.9, htslib-1.9
> (samtools depends on that) to fix this problem with bedtools.

Recently, investigating why the substitute of ’python-pysam’ was not
available, I decided then to give a try at fixing the TODO:

--8<---------------cut here---------------start------------->8---
              (snippet '(begin
                          ;; Drop bundled htslib. TODO: Also remove samtools
                          ;; and bcftools.
                          (delete-file-recursively "htslib")
                          #t))))
--8<---------------cut here---------------end--------------->8---

And the bundled version is 1.9 (if I remember correctly), therefore
because of:

--8<---------------cut here---------------start------------->8---
    (native-inputs
     `(("python-cython" ,python-cython)
       ;; Dependencies below are are for tests only.
       ("samtools" ,samtools)
       ("bcftools" ,bcftools)
       ("python-nose" ,python-nose)))
--8<---------------cut here---------------end--------------->8---

some tests are unhappy.

That’s said, these additions seem fine with me. :-)


All the best,
simon
Roel Janssen Nov. 13, 2020, 12:55 p.m. UTC | #2
Hi Simon,

On Fri, 2020-11-13 at 13:39 +0100, zimoun wrote:
> Hi Roel,
> 
> On Fri, 13 Nov 2020 at 12:01, Roel Janssen <roel@gnu.org> wrote:
> 
> > With the following patches, I'd like to add samtools-1.9, htslib-
> > 1.9
> > (samtools depends on that) to fix this problem with bedtools.
> 
> Recently, investigating why the substitute of ’python-pysam’ was not
> available, I decided then to give a try at fixing the TODO:
> 
> --8<---------------cut here---------------start------------->8---
>               (snippet '(begin
>                           ;; Drop bundled htslib. TODO: Also remove
> samtools
>                           ;; and bcftools.
>                           (delete-file-recursively "htslib")
>                           #t))))
> --8<---------------cut here---------------end--------------->8---
> 
> And the bundled version is 1.9 (if I remember correctly), therefore
> because of:
> 
> --8<---------------cut here---------------start------------->8---
>     (native-inputs
>      `(("python-cython" ,python-cython)
>        ;; Dependencies below are are for tests only.
>        ("samtools" ,samtools)
>        ("bcftools" ,bcftools)
>        ("python-nose" ,python-nose)))
> --8<---------------cut here---------------end--------------->8---
> 
> some tests are unhappy.
> 
> That’s said, these additions seem fine with me. :-)
> 

I also tried removing the bundled htslib for bedtools, but didn't go
this route for two reasons:
- The bundled htslib for bedtools seems "slightly modified" (I didn't
investigate further)
- Replacing the references to libhts.a with $(pkg-config htslib --
cflags --libs) produced various linker errors. So I stopped right
there.

I'm sure more tools will likely have failed because of the htslib
upgrade (sorry about this!), so having htslib-1.9 around for some time
may be a good fallback for now.

Just to double-check: Is it OK to push the proposed patches?

Kind regards,
Roel Janssen
zimoun Nov. 13, 2020, 1:34 p.m. UTC | #3
Hi Roel,

On Fri, 13 Nov 2020 at 13:55, Roel Janssen <roel@gnu.org> wrote:

> I also tried removing the bundled htslib for bedtools, but didn't go
> this route for two reasons:
> - The bundled htslib for bedtools seems "slightly modified" (I didn't
> investigate further)
> - Replacing the references to libhts.a with $(pkg-config htslib --
> cflags --libs) produced various linker errors. So I stopped right
> there.
>
> I'm sure more tools will likely have failed because of the htslib
> upgrade (sorry about this!), so having htslib-1.9 around for some time
> may be a good fallback for now.

Thank for your explanations.


> Just to double-check: Is it OK to push the proposed patches?

I have not tried them but they LGTM.

All the best,
simon

PS:
I am always confused if the removal should be done in ’origin’ or in the
’add-after 'unpack’ phase; especially when the bundle is free software.
Other said, what should an user expect when fetching with “guix build -S”?
Anyway! :-)
Roel Janssen Nov. 13, 2020, 2 p.m. UTC | #4
Hi Simon,

On Fri, 2020-11-13 at 14:34 +0100, zimoun wrote:
> Hi Roel,
> 
> On Fri, 13 Nov 2020 at 13:55, Roel Janssen <roel@gnu.org> wrote:
> 
> > I also tried removing the bundled htslib for bedtools, but didn't
> > go
> > this route for two reasons:
> > - The bundled htslib for bedtools seems "slightly modified" (I
> > didn't
> > investigate further)
> > - Replacing the references to libhts.a with $(pkg-config htslib --
> > cflags --libs) produced various linker errors. So I stopped right
> > there.
> > 
> > I'm sure more tools will likely have failed because of the htslib
> > upgrade (sorry about this!), so having htslib-1.9 around for some
> > time
> > may be a good fallback for now.
> 
> Thank for your explanations.
> 
> 
> > Just to double-check: Is it OK to push the proposed patches?
> 
> I have not tried them but they LGTM.
> 

Thanks for the quick review. I pushed the patches in
c3232fcc7785abc1057a0d4b5b1832f1e39c9c1b,
da4a38edad52f7bb5a8d41465d09f3f0197fd0b7, and
3ede804f6d4c38ff0b9a5705544a8c35f6827ff1.

Kind regards,
Roel Janssen
Marius Bakke Nov. 13, 2020, 3:08 p.m. UTC | #5
zimoun <zimon.toutoune@gmail.com> writes:

> PS:
> I am always confused if the removal should be done in ’origin’ or in the
> ’add-after 'unpack’ phase; especially when the bundle is free software.
> Other said, what should an user expect when fetching with “guix build -S”?
> Anyway! :-)

Unbundling is always better to do in a snippet.  It leads to less
bandwidth usage, and users can more easily inspect the (actual) code.

For other kinds of patching the boundary is less clear.  Generally,
Guix-specific tweaks should be in a phase, but "universal" bug fixes may
well be in a snippet.

I sometimes imagine a downstream distribution that use Guix sources, but
not the build scripts, to draw the line.
zimoun Nov. 13, 2020, 3:51 p.m. UTC | #6
Hi Marius,

Thank you for the explanations.


On Fri, 13 Nov 2020 at 16:08, Marius Bakke <marius@gnu.org> wrote:
> zimoun <zimon.toutoune@gmail.com> writes:
>
>> PS:
>> I am always confused if the removal should be done in ’origin’ or in the
>> ’add-after 'unpack’ phase; especially when the bundle is free software.
>> Other said, what should an user expect when fetching with “guix build -S”?
>> Anyway! :-)
>
> Unbundling is always better to do in a snippet.  It leads to less
> bandwidth usage, and users can more easily inspect the (actual) code.

Well, I do not know.  For example, I could do this workflow:

 guix environment bedtools
 tar -xvf $(guix build -S bedtools)
 make

which probably fails because removing the bundles often needs some extra
tweaks.  Concretely, see python-pysam for instance:

--8<---------------cut here---------------start------------->8---
              (snippet '(begin
                          ;; Drop bundled htslib. TODO: Also remove samtools
                          ;; and bcftools.
                          (delete-file-recursively "htslib")
                          #t))))
[...]
       #:phases
       (modify-phases %standard-phases
         (add-before 'build 'set-flags
           (lambda* (#:key inputs #:allow-other-keys)
             (setenv "HTSLIB_MODE" "external")
             (setenv "HTSLIB_LIBRARY_DIR"
                     (string-append (assoc-ref inputs "htslib") "/lib"))
             (setenv "HTSLIB_INCLUDE_DIR"
                     (string-append (assoc-ref inputs "htslib") "/include"))
--8<---------------cut here---------------end--------------->8---

Then, I am not convince that:

  guix build bedtools --with-git-url=http://example.org

works too.  Or ’--with-source=’ as well.  I remember a discussion
initiated by Mark and Maxim about this: snippet vs phases but I am not
able to reach it.


> For other kinds of patching the boundary is less clear.  Generally,
> Guix-specific tweaks should be in a phase, but "universal" bug fixes may
> well be in a snippet.

I agree that non-free and bug fixes should go to snippet.  Then I am
still confused and my feelings are mixed about Guix specific tweaks.


> I sometimes imagine a downstream distribution that use Guix sources, but
> not the build scripts, to draw the line.

It seems a good criteria to draw the line.  And in the case of bedtools
or python-pysam or many others, ’snippet’ removes (free software)
bundles because of an implicit and non-uniform Guix policy that a
downstream distribution could choose differently.

Well, my mind is not clear about this topic. :-)

All the best,
simon
diff mbox series

Patch

From 220604159383b09e11a7b0bed51237257c7c0442 Mon Sep 17 00:00:00 2001
From: Roel Janssen <roel@gnu.org>
Date: Fri, 13 Nov 2020 11:28:43 +0100
Subject: [PATCH 1/3] gnu: Add htslib-1.9.

* gnu/packages/bioinformatics.scm (htslib-1.9): New variable.
---
 gnu/packages/bioinformatics.scm | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/gnu/packages/bioinformatics.scm b/gnu/packages/bioinformatics.scm
index 06972dee51..ba86333fc3 100644
--- a/gnu/packages/bioinformatics.scm
+++ b/gnu/packages/bioinformatics.scm
@@ -4250,6 +4250,19 @@  data.  It also provides the @command{bgzip}, @command{htsfile}, and
     ;; the rest is released under the Expat license
     (license (list license:expat license:bsd-3))))
 
+(define-public htslib-1.9
+  (package (inherit htslib)
+    (name "htslib")
+    (version "1.9")
+    (source (origin
+              (method url-fetch)
+              (uri (string-append
+                    "https://github.com/samtools/htslib/releases/download/"
+                    version "/htslib-" version ".tar.bz2"))
+              (sha256
+               (base32
+                "16ljv43sc3fxmv63w7b2ff8m1s7h89xhazwmbm1bicz8axq8fjz0"))))))
+
 ;; This package should be removed once no packages rely upon it.
 (define htslib-1.3
   (package
-- 
2.29.2