[bug#72071] [PATCH 0/2] Fix hdf5-parallel-openmpi

[bug#72071] [PATCH 0/2] Fix hdf5-parallel-openmpi

[Luca Cirrottola]

Package hdf5-parallel-openmpi builds often fail with the error reported in https://issues.guix.gnu.org/68313 . The issue has been discussed in https://github.com/open-mpi/ompi/issues/11240 and https://github.com/HDFGroup/hdf5/issues/2196, and solved by https://github.com/HDFGroup/hdf5/pull/3421 that is included in HDF5 release 1.14.3. My proposed solution consists of two commits, the first for updating the HDF5 version in hdf5-parallel-openmpi to 1.14.3, the second for fixing a new test error due to the recipe modifying a file which is used as a test reference (my proposition is to remove this modification, but I don't know why it was introduced in the first place, so thanks for any comment about this).

Luca Cirrottola (2):
  Solve issue https://issues.guix.gnu.org/68313 by updating HDF5 version
    to 1.14.3 (containing the official fix
    https://github.com/HDFGroup/hdf5/pull/3421).
  Fix broken HDF5 test: Do not modify the file src/libhdf5.settings
    generated by the configure script, as it will be checked against a
    new version generated by the test code in
    test/test_check_version.sh.in.

 gnu/packages/maths.scm | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

-- 
2.34.1

[bug#72071] [PATCH 1/2] Solve issue https://issues.guix.gnu.org/68313 by updating HDF5 version to 1.14.3 (containing the official fix https://github.com/HDFGroup/hdf5/pull/3421).

[Luca Cirrottola]

---
 gnu/packages/maths.scm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
index 391c11891e..8116569bca 100644
--- a/gnu/packages/maths.scm
+++ b/gnu/packages/maths.scm
@@ -1814,7 +1814,7 @@ (define-public hdf5-1.10
 (define-public hdf5-1.14
   (package
     (inherit hdf5-1.8)
-    (version "1.14.0")
+    (version "1.14.3")
     (source
      (origin
        (method url-fetch)
@@ -1828,7 +1828,7 @@ (define-public hdf5-1.14
                                         (take (string-split version #\.) 2))
                                  "/src/hdf5-" version ".tar.bz2")))
        (sha256
-        (base32 "181bdh8hp7v9xqwcby3lknr92lxlicc2hqscba3f5nhf8lrr9rz4"))
+        (base32 "05zr11y3bivfwrbvzbky1q2gjf6r7n92cvvdnh5jilbmxljg49cl"))
        (patches (search-patches "hdf5-config-date.patch"))))))
 
 (define-public hdf5
@@ -2069,7 +2069,7 @@ (define-public hdf-eos5
     (license (license:non-copyleft home-page))))
 
 (define-public hdf5-parallel-openmpi
-  (package/inherit hdf5-1.10                      ;use the latest
+  (package/inherit hdf5-1.14                      ;use the latest
     (name "hdf5-parallel-openmpi")
     (inputs
      `(("mpi" ,openmpi)
-- 
2.34.1

[bug#72071] [PATCH 2/2] Fix broken HDF5 test: Do not modify the file src/libhdf5.settings generated by the configure script, as it will be checked against a new version generated by the test code in test/test_check_version.sh.in.

[Luca Cirrottola]

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

diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
index 8116569bca..2f12deba3b 100644
--- a/gnu/packages/maths.scm
+++ b/gnu/packages/maths.scm
@@ -1729,19 +1729,6 @@ (define-public hdf5-1.8
                (("libhdf5hl_fortran_la_LDFLAGS =")
                 (string-append "libhdf5hl_fortran_la_LDFLAGS = -Wl,-rpath="
                                (assoc-ref outputs "fortran") "/lib")))))
-         (add-after 'configure 'patch-settings
-           (lambda _
-             ;; libhdf5.settings contains the full path of the
-             ;; compilers used, and its contents are included in
-             ;; libhdf5.so.  We truncate the hashes to avoid
-             ;; unnecessary store references to those compilers:
-             (substitute* "src/libhdf5.settings"
-              (("(/gnu/store/)([a-zA-Z0-9]*)" all prefix hash)
-               (string-append prefix (string-take hash 10) "..."))
-              ;; Don't record the build-time kernel version to make the
-              ;; settings file reproducible.
-              (("Uname information:.*")
-               "Uname information: Linux\n"))))
          (add-after 'install 'patch-references
            (lambda* (#:key inputs outputs #:allow-other-keys)
              (let ((bin (string-append (assoc-ref outputs "out") "/bin"))
-- 
2.34.1

[bug#72071] [PATCH 0/2] Fix hdf5-parallel-openmpi

[Ludovic Courtès]

Hello Luca!

Luca Cirrottola <luca.cirrottola@inria.fr> skribis:

> Package hdf5-parallel-openmpi builds often fail with the error reported in https://issues.guix.gnu.org/68313 . The issue has been discussed in https://github.com/open-mpi/ompi/issues/11240 and https://github.com/HDFGroup/hdf5/issues/2196, and solved by https://github.com/HDFGroup/hdf5/pull/3421 that is included in HDF5 release 1.14.3. My proposed solution consists of two commits, the first for updating the HDF5 version in hdf5-parallel-openmpi to 1.14.3, the second for fixing a new test error due to the recipe modifying a file which is used as a test reference (my proposition is to remove this modification, but I don't know why it was introduced in the first place, so thanks for any comment about this).
>
> Luca Cirrottola (2):
>   Solve issue https://issues.guix.gnu.org/68313 by updating HDF5 version
>     to 1.14.3 (containing the official fix
>     https://github.com/HDFGroup/hdf5/pull/3421).

This first patch LGTM.

Do I get it right that there’s one test failure if we apply just this
one patch?

>   Fix broken HDF5 test: Do not modify the file src/libhdf5.settings
>     generated by the configure script, as it will be checked against a
>     new version generated by the test code in
>     test/test_check_version.sh.in.

Here, the ‘patch-settings’ phase was here (1) to ensure bit-for-bit
reproducible builds regardless of the kernel (the “Uname” change), and
(2) to avoid keeping references to the compiler (hdf5 would depend on
GCC & co., so its closure as reported by ‘guix size hdf5’ would be much
bigger than necessary).

I haven’t looked at ‘test_check_version.sh.in’, but can you think of a
way to preserve the changes ‘patch-settings’ makes while still allowing
that test to pass?

Thanks,
Ludo’.

[bug#72071] [PATCH 0/2] Fix hdf5-parallel-openmpi

[Luca Cirrottola]

[-- Attachment #1: Type: text/plain, Size: 3014 bytes --]

Hello Ludo'!

Yes, after applying the first patch with the package version update, a failure appears in "test_check_version" (I attach the log for future reference). I don't know if this happens now because of an update in the HDF5 package, or because the old version failed before reaching this test.
Anyway, this test compares two versions of a file generated according to the template in src/libhdf5.settings.in, the first one generated in the "configure" phase, the second one in the "check" phase. Since the "patch-settings" phase modifies the first file, the test fails.
Thanks for the explanations for the "patch-settings" phase! I will come back in the next message with a replacement for the second patch which hopefully does not break the expected behaviour.

Thanks,
Luca

----- Mail original -----
> De: "Ludovic Courtès" <ludovic.courtes@inria.fr>
> À: "Luca Cirrottola" <luca.cirrottola@inria.fr>
> Cc: 72071@debbugs.gnu.org
> Envoyé: Vendredi 19 Juillet 2024 18:01:30
> Objet: Re: [bug#72071] [PATCH 0/2] Fix hdf5-parallel-openmpi

> Hello Luca!
> 
> Luca Cirrottola <luca.cirrottola@inria.fr> skribis:
> 
>> Package hdf5-parallel-openmpi builds often fail with the error reported in
>> https://issues.guix.gnu.org/68313 . The issue has been discussed in
>> https://github.com/open-mpi/ompi/issues/11240 and
>> https://github.com/HDFGroup/hdf5/issues/2196, and solved by
>> https://github.com/HDFGroup/hdf5/pull/3421 that is included in HDF5 release
>> 1.14.3. My proposed solution consists of two commits, the first for updating
>> the HDF5 version in hdf5-parallel-openmpi to 1.14.3, the second for fixing a
>> new test error due to the recipe modifying a file which is used as a test
>> reference (my proposition is to remove this modification, but I don't know why
>> it was introduced in the first place, so thanks for any comment about this).
>>
>> Luca Cirrottola (2):
>>   Solve issue https://issues.guix.gnu.org/68313 by updating HDF5 version
>>     to 1.14.3 (containing the official fix
>>     https://github.com/HDFGroup/hdf5/pull/3421).
> 
> This first patch LGTM.
> 
> Do I get it right that there’s one test failure if we apply just this
> one patch?
> 
>>   Fix broken HDF5 test: Do not modify the file src/libhdf5.settings
>>     generated by the configure script, as it will be checked against a
>>     new version generated by the test code in
>>     test/test_check_version.sh.in.
> 
> Here, the ‘patch-settings’ phase was here (1) to ensure bit-for-bit
> reproducible builds regardless of the kernel (the “Uname” change), and
> (2) to avoid keeping references to the compiler (hdf5 would depend on
> GCC & co., so its closure as reported by ‘guix size hdf5’ would be much
> bigger than necessary).
> 
> I haven’t looked at ‘test_check_version.sh.in’, but can you think of a
> way to preserve the changes ‘patch-settings’ makes while still allowing
> that test to pass?
> 
> Thanks,
> Ludo’.

[-- Attachment #2: ypb72f7f2xc13a3sjb5jwg45gsjw1g-hdf5-parallel-openmpi-1.14.3.drv.gz --]
[-- Type: application/gzip, Size: 75819 bytes --]

[bug#72071] [PATCH 0/1] Split the patch-settings phase into copy-settings, patch-settings-for-build, restore-settings-for-check, patch-settings-for-install

[Luca Cirrottola]

Let the test_check_version test success by backing-up the original
settings file (to use it as a test reference) and swap it with the
patched file. Swap back the two files before installation.

Luca Cirrottola (1):
  Backup original settings, so they can be restored before the check
    phase and patched again before the install phase (since the
    install-data target depends on this file).

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

-- 
2.34.1

[bug#72071] [PATCH 1/1] Backup original settings, so they can be restored before the check phase and patched again before the install phase (since the install-data target depends on this file).

[Luca Cirrottola]

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

diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
index 8116569bca..6787f9bb49 100644
--- a/gnu/packages/maths.scm
+++ b/gnu/packages/maths.scm
@@ -1729,7 +1729,13 @@ (define-public hdf5-1.8
                (("libhdf5hl_fortran_la_LDFLAGS =")
                 (string-append "libhdf5hl_fortran_la_LDFLAGS = -Wl,-rpath="
                                (assoc-ref outputs "fortran") "/lib")))))
-         (add-after 'configure 'patch-settings
+         (add-after 'configure 'copy-settings
+           (lambda _
+             ;; Backup the original settings (because they are needed as a test
+             ;; reference during the "check" phase) before patching them in the
+             ;; "patch-settings-for-build" phase.
+             (copy-file "src/libhdf5.settings" "libhdf5_settings.orig")))
+         (add-before 'build 'patch-settings-for-build
            (lambda _
              ;; libhdf5.settings contains the full path of the
              ;; compilers used, and its contents are included in
@@ -1742,7 +1748,19 @@ (define-public hdf5-1.8
               ;; settings file reproducible.
               (("Uname information:.*")
                "Uname information: Linux\n"))))
-         (add-after 'install 'patch-references
+         (add-before 'check 'restore-settings-for-check
+           (lambda _
+             ;; Swap the file that has been patched in the
+             ;; "patch-settings-for-build" phase with the original one which
+             ;; has been saved in the "copy-settings" phase, so that it can be
+             ;; used as a test reference in the "check" phase.
+             (copy-file "src/libhdf5.settings" "libhdf5_settings.patched")
+             (rename-file "libhdf5_settings.orig" "src/libhdf5.settings")))
+         (add-before 'install 'patch-settings-for-install
+           (lambda _
+             ;; Restore the patched settings file before installation.
+             (rename-file "libhdf5_settings.patched" "src/libhdf5.settings")))
+        (add-after 'install 'patch-references
            (lambda* (#:key inputs outputs #:allow-other-keys)
              (let ((bin (string-append (assoc-ref outputs "out") "/bin"))
                    (zlib (assoc-ref inputs "zlib")))
-- 
2.34.1

[bug#72071] [PATCH 0/2] A clean series of patches

[Luca Cirrottola]

Hello Ludo', following your suggestion, I have implemented a simpler
solution that just skips off the problematic test (without moving
around files). I send a new patch series with the full modifications
to be applied to the recipe, containing one commit for the package
version update, and one commit for skipping the test.

Let me know if it looks better (no pressure).

Thanks,
Luca

Luca Cirrottola (2):
  Solve issue https://issues.guix.gnu.org/68313 by updating HDF5 version
    to 1.14.3 (containing the official fix
    https://github.com/HDFGroup/hdf5/pull/3421).
  Skip test_check_version since the patch-settings phase needs to modify
    the test reference file.

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

-- 
2.34.1

[bug#72071] [PATCH 1/2] Solve issue https://issues.guix.gnu.org/68313 by updating HDF5 version to 1.14.3 (containing the official fix https://github.com/HDFGroup/hdf5/pull/3421).

[Luca Cirrottola]

---
 gnu/packages/maths.scm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
index 391c11891e..8116569bca 100644
--- a/gnu/packages/maths.scm
+++ b/gnu/packages/maths.scm
@@ -1814,7 +1814,7 @@ (define-public hdf5-1.10
 (define-public hdf5-1.14
   (package
     (inherit hdf5-1.8)
-    (version "1.14.0")
+    (version "1.14.3")
     (source
      (origin
        (method url-fetch)
@@ -1828,7 +1828,7 @@ (define-public hdf5-1.14
                                         (take (string-split version #\.) 2))
                                  "/src/hdf5-" version ".tar.bz2")))
        (sha256
-        (base32 "181bdh8hp7v9xqwcby3lknr92lxlicc2hqscba3f5nhf8lrr9rz4"))
+        (base32 "05zr11y3bivfwrbvzbky1q2gjf6r7n92cvvdnh5jilbmxljg49cl"))
        (patches (search-patches "hdf5-config-date.patch"))))))
 
 (define-public hdf5
@@ -2069,7 +2069,7 @@ (define-public hdf-eos5
     (license (license:non-copyleft home-page))))
 
 (define-public hdf5-parallel-openmpi
-  (package/inherit hdf5-1.10                      ;use the latest
+  (package/inherit hdf5-1.14                      ;use the latest
     (name "hdf5-parallel-openmpi")
     (inputs
      `(("mpi" ,openmpi)
-- 
2.34.1

[bug#72071] [PATCH 2/2] Skip test_check_version since the patch-settings phase needs to modify the test reference file.

[Luca Cirrottola]

---
 gnu/packages/maths.scm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/maths.scm b/gnu/packages/maths.scm
index 8116569bca..4e69fb1eb9 100644
--- a/gnu/packages/maths.scm
+++ b/gnu/packages/maths.scm
@@ -1728,7 +1728,12 @@ (define-public hdf5-1.8
              (substitute* "hl/fortran/src/Makefile.in"
                (("libhdf5hl_fortran_la_LDFLAGS =")
                 (string-append "libhdf5hl_fortran_la_LDFLAGS = -Wl,-rpath="
-                               (assoc-ref outputs "fortran") "/lib")))))
+                               (assoc-ref outputs "fortran") "/lib")))
+             ;; Skip test_check_version since the "patch-settings" phase
+             ;; will modify the test reference.
+             (substitute* "test/test_check_version.sh.in"
+              (("TESTING\\(\\).*" all)
+               (string-append all "\nSKIP; exit 0\n")))))
          (add-after 'configure 'patch-settings
            (lambda _
              ;; libhdf5.settings contains the full path of the
-- 
2.34.1

bug#72071: [PATCH 0/2] A clean series of patches

[Ludovic Courtès]

Hi Luca,

Luca Cirrottola <luca.cirrottola@inria.fr> skribis:

> Hello Ludo', following your suggestion, I have implemented a simpler
> solution that just skips off the problematic test (without moving
> around files). I send a new patch series with the full modifications
> to be applied to the recipe, containing one commit for the package
> version update, and one commit for skipping the test.
>
> Let me know if it looks better (no pressure).
>
> Thanks,
> Luca
>
> Luca Cirrottola (2):
>   Solve issue https://issues.guix.gnu.org/68313 by updating HDF5 version
>     to 1.14.3 (containing the official fix
>     https://github.com/HDFGroup/hdf5/pull/3421).
>   Skip test_check_version since the patch-settings phase needs to modify
>     the test reference file.

I reorganized the changes a bit; in particular, since skipping the test
is necessary for 1.14.3 to build, I squashed the upgrade and the test
skip in the same commit.

Also, the test change would break hdf@1.8, so I moved that to hdf@1.14.

The result is:

  84c62cf166 * gnu: hdf5-parallel-openmpi: Update to 1.14.x.
  832f2bcb92 * gnu: hdf5@1.14: Update to 1.14.3.

Tested with:

  ./pre-inst-env guix build --no-grafts \
    hdf5@1.8 hdf5@1.14 hdf5-parallel-openmpi hdf5@1.10

Thanks!

Ludo’.