[bug#35666] [PATCH 0/2] Build a thread-safe hdf5 library

[bug#35666] [PATCH 0/2] Build a thread-safe hdf5 library

[Ludovic Courtès]

Hello!

A colleague of mine noticed that our ‘hdf5’ library wasn’t
thread-safe.  Turns out there’s an option to make it thread-safe (oh!),
it’s turned off by default (oh?!), and when you pass it ‘configure’
invites you to turn off no less than C++, Fortran, and the high-level
interface (d’oh!).

It also tells you that, if you insist, you can go ahead and pass
‘--enable-unsupported’, but you’re on your own.

We found that Debian chose to pass ‘--enable-unsupported’, and indeed
that seems to be saner than providing a variant that does very little,
but does it in a thread-safe way.

Thoughts?

Thanks,
Ludo’.

Ludovic Courtès (2):
  gnu: hdf5: Build a thread-safe library.
  gnu: hdf5: Add dependency on Perl.

 gnu/packages/maths.scm | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

-- 
2.21.0

[bug#35666] [PATCH 1/2] gnu: hdf5: Build a thread-safe library.

[Ludovic Courtès]

From: Ludovic Courtès <ludovic.courtes@inria.fr>

* gnu/packages/maths.scm (hdf5)[arguments]: Pass "--enable-threadsafe
--with-pthread --enable-unsupported".
---
 gnu/packages/maths.scm | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
index d59028599f..2c3889ece2 100644
--- a/gnu/packages/maths.scm
+++ b/gnu/packages/maths.scm
@@ -781,7 +781,17 @@ incompatible with HDF5.")
      `(;; Some of the users, notably Flann, need the C++ interface.
        #:configure-flags '("--enable-cxx"
                            "--enable-fortran"
-                           "--enable-fortran2003")
+                           "--enable-fortran2003"
+
+                           ;; Build a thread-safe library.  Unfortunately,
+                           ;; 'configure' invites you to either turn off C++,
+                           ;; Fortran, and the high-level interface (HL), or
+                           ;; to pass '--enable-unsupported'.  Debian
+                           ;; packagers chose to pass '--enable-unsupported'
+                           ;; and we follow their lead here.
+                           "--enable-threadsafe"
+                           "--with-pthread"
+                           "--enable-unsupported")
        ;; Use -fPIC to allow the R bindings to link with the static libraries
        #:make-flags (list "CFLAGS=-fPIC"
                           "CXXFLAGS=-fPIC")
-- 
2.21.0

[bug#35666] [PATCH 2/2] gnu: hdf5: Add dependency on Perl.

[Ludovic Courtès]

From: Ludovic Courtès <ludovic.courtes@inria.fr>

* gnu/packages/maths.scm (hdf5)[native-inputs]: Add PERL.
---
 gnu/packages/maths.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
index 2c3889ece2..7ea94d1060 100644
--- a/gnu/packages/maths.scm
+++ b/gnu/packages/maths.scm
@@ -774,7 +774,8 @@ incompatible with HDF5.")
     (inputs
      `(("zlib" ,zlib)))
     (native-inputs
-     `(("gfortran" ,gfortran)))
+     `(("gfortran" ,gfortran)
+       ("perl" ,perl)))                 ;part of the test machinery needs Perl
     (outputs '("out"       ; core library
                "fortran")) ; fortran interface
     (arguments
-- 
2.21.0

[bug#35666] [PATCH 0/2] Build a thread-safe hdf5 library

[Ricardo Wurmus]

Ludovic Courtès <ludo@gnu.org> writes:

> A colleague of mine noticed that our ‘hdf5’ library wasn’t
> thread-safe.  Turns out there’s an option to make it thread-safe (oh!),
> it’s turned off by default (oh?!), and when you pass it ‘configure’
> invites you to turn off no less than C++, Fortran, and the high-level
> interface (d’oh!).

Oh!

> It also tells you that, if you insist, you can go ahead and pass
> ‘--enable-unsupported’, but you’re on your own.
>
> We found that Debian chose to pass ‘--enable-unsupported’, and indeed
> that seems to be saner than providing a variant that does very little,
> but does it in a thread-safe way.

What other effects does “--enable-unsupported” have?  I see that in
Fedora “--enable-threadsafe” was removed in 2008 because it’s
“incompatible with --enable-cxx and --enable-fortran”.

Instead they seem to be building different flavours: one with
--enable-fortran, another with --enable-cxx, yet another with MPI and
--enable-parallel.

Do we have contact to the hdf5 developers to ask what the implications
of “enable-unsupported” are?

--
Ricardo

[bug#35666] [PATCH 0/2] Build a thread-safe hdf5 library

[Ludovic Courtès]

Hi!

Ricardo Wurmus <rekado@elephly.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[…]

>> It also tells you that, if you insist, you can go ahead and pass
>> ‘--enable-unsupported’, but you’re on your own.
>>
>> We found that Debian chose to pass ‘--enable-unsupported’, and indeed
>> that seems to be saner than providing a variant that does very little,
>> but does it in a thread-safe way.
>
> What other effects does “--enable-unsupported” have?  I see that in
> Fedora “--enable-threadsafe” was removed in 2008 because it’s
> “incompatible with --enable-cxx and --enable-fortran”.

“--enable-unsupported” allows you to force a build that combines C++,
Fortran, and thread-safety.  If you don’t pass that flag, you have to
choose between thread-safety and C++/Fortran¹.  A tough choice!

> Instead they seem to be building different flavours: one with
> --enable-fortran, another with --enable-cxx, yet another with MPI and
> --enable-parallel.

Problem is, my colleagues have code that expects both C++ and
thread-safety (as crazy as it might seem).  They were using the Debian
package until now and hadn’t realized about this.

> Do we have contact to the hdf5 developers to ask what the implications
> of “enable-unsupported” are?

I think it’s a warranty-void kind of flag: by passing it, the user
asserts they understand they’re using a configuration not “officially
supported” by the HDF Group, meaning that if it’s buggy, we’re on our
own.

Thoughts?

Ludo’.

¹ You would think it’s an April fool’s day prank, but it’s not!  We’re
  in May, at least in my timezone.

[bug#35666] [PATCH 0/2] Build a thread-safe hdf5 library

[Eric Bavier]

I think this should be fine, though I've not heard of anyone who has relied on this feature.  The "unsupported" part here is that the posix lock used for thread-safety is not hoisted into the higher-level API calls.  So if your colleague is using the C++ interface and expecting thread-safety, they are out of luck.  So the disclaimer is that only the low-level C interface gains thread-safety, and the rest are no better.

Eric Bavier, Scientific Libraries, Cray Inc.

________________________________________
From: Ludovic Courtès <ludovic.courtes@inria.fr>
Sent: Friday, May 10, 2019 08:07
To: Ricardo Wurmus
Cc: Eric Bavier; pgarlick@tourbillion-technology.com; 35666@debbugs.gnu.org
Subject: Re: [bug#35666] [PATCH 0/2] Build a thread-safe hdf5 library

Hi!

Ricardo Wurmus <rekado@elephly.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[…]

>> It also tells you that, if you insist, you can go ahead and pass
>> ‘--enable-unsupported’, but you’re on your own.
>>
>> We found that Debian chose to pass ‘--enable-unsupported’, and indeed
>> that seems to be saner than providing a variant that does very little,
>> but does it in a thread-safe way.
>
> What other effects does “--enable-unsupported” have?  I see that in
> Fedora “--enable-threadsafe” was removed in 2008 because it’s
> “incompatible with --enable-cxx and --enable-fortran”.

“--enable-unsupported” allows you to force a build that combines C++,
Fortran, and thread-safety.  If you don’t pass that flag, you have to
choose between thread-safety and C++/Fortran¹.  A tough choice!

> Instead they seem to be building different flavours: one with
> --enable-fortran, another with --enable-cxx, yet another with MPI and
> --enable-parallel.

Problem is, my colleagues have code that expects both C++ and
thread-safety (as crazy as it might seem).  They were using the Debian
package until now and hadn’t realized about this.

> Do we have contact to the hdf5 developers to ask what the implications
> of “enable-unsupported” are?

I think it’s a warranty-void kind of flag: by passing it, the user
asserts they understand they’re using a configuration not “officially
supported” by the HDF Group, meaning that if it’s buggy, we’re on our
own.

Thoughts?

Ludo’.

¹ You would think it’s an April fool’s day prank, but it’s not!  We’re
  in May, at least in my timezone.

[bug#35666] [PATCH 0/2] Build a thread-safe hdf5 library

[Paul Garlick]

Hi Ludo and Ricardo,

> Thoughts?
> 

I notice in the Debian rules file [1] that the "--enable-threadsafe"
flag is listed as  one of the SERIAL_FLAGS.  In the Guix case the
package hdf5-parallel-openmpi inherits its configure flags from the
hdf5 (serial) package.

Will we also have to explicitly delete the "--enable-threadsafe" flag
from the hdf5-parallel-openmpi definition?

Best regards,

Paul.

[1] https://sources.debian.org/src/hdf5/1.10.4+repack-10/debian/rules/

[bug#35666] [PATCH 0/2] Build a thread-safe hdf5 library

[Ludovic Courtès]

Hi Eric,

Eric Bavier <bavier@cray.com> skribis:

> I think this should be fine, though I've not heard of anyone who has
> relied on this feature.  The "unsupported" part here is that the posix
> lock used for thread-safety is not hoisted into the higher-level API
> calls.  So if your colleague is using the C++ interface and expecting
> thread-safety, they are out of luck.  So the disclaimer is that only
> the low-level C interface gains thread-safety, and the rest are no
> better.

I’m not sure I understand.  Do you mean that, just because you use the
C++ API instead of the C API, the library is not thread-safe?

They do see crashes vanish when using the library compiled with
‘--enable-threadsafe’, and reliably so.

Thanks,
Ludo’.

bug#35666: [PATCH 0/2] Build a thread-safe hdf5 library

[Ludovic Courtès]

Hi Paul,

Paul Garlick <pgarlick@tourbillion-technology.com> skribis:

> I notice in the Debian rules file [1] that the "--enable-threadsafe"
> flag is listed as  one of the SERIAL_FLAGS.  In the Guix case the
> package hdf5-parallel-openmpi inherits its configure flags from the
> hdf5 (serial) package.
>
> Will we also have to explicitly delete the "--enable-threadsafe" flag
> from the hdf5-parallel-openmpi definition?

Good point, I think so.

I did that and pushed it as 549d15712fdc1f58ce0dd11117eb79535ec19f2c.

Let me know if anything is amiss!

Ludo’.

[bug#35666] [PATCH 0/2] Build a thread-safe hdf5 library

[Ricardo Wurmus]

Ludovic Courtès <ludovic.courtes@inria.fr> writes:

>> Do we have contact to the hdf5 developers to ask what the implications
>> of “enable-unsupported” are?
>
> I think it’s a warranty-void kind of flag: by passing it, the user
> asserts they understand they’re using a configuration not “officially
> supported” by the HDF Group, meaning that if it’s buggy, we’re on our
> own.

I don’t object to adding the option.  It sounds like you confirmed that
it fixes serious problems in practise, so I think it’s a good idea to
enable it.

--
Ricardo

[bug#35666] [PATCH 0/2] Build a thread-safe hdf5 library

[Eric Bavier]

> I’m not sure I understand.  Do you mean that, just because you use the
> C++ API instead of the C API, the library is not thread-safe?

The thread-safety of the C++ interface itself is not guaranteed/"supported".

> They do see crashes vanish when using the library compiled with
> ‘--enable-threadsafe’, and reliably so.

Great.  I'm not familiar enough with the C++ interface code to say which areas might cause problems in a threaded context.  It's likely that whatever problems they were seeing before was not at the interface layer but deeper.

Eric Bavier, Scientific Libraries, Cray Inc.

________________________________________
From: Ludovic Courtès <ludovic.courtes@inria.fr>
Sent: Tuesday, May 14, 2019 02:28
To: Eric Bavier
Cc: Ricardo Wurmus; 35666@debbugs.gnu.org; pgarlick@tourbillion-technology.com
Subject: Re: [bug#35666] [PATCH 0/2] Build a thread-safe hdf5 library

Hi Eric,

Eric Bavier <bavier@cray.com> skribis:

> I think this should be fine, though I've not heard of anyone who has
> relied on this feature.  The "unsupported" part here is that the posix
> lock used for thread-safety is not hoisted into the higher-level API
> calls.  So if your colleague is using the C++ interface and expecting
> thread-safety, they are out of luck.  So the disclaimer is that only
> the low-level C interface gains thread-safety, and the rest are no
> better.

I’m not sure I understand.  Do you mean that, just because you use the
C++ API instead of the C API, the library is not thread-safe?

They do see crashes vanish when using the library compiled with
‘--enable-threadsafe’, and reliably so.

Thanks,
Ludo’.