Re: [PATCH] gnu: Add hdf4

[Ludovic Courtès]

Hello!

Thomas Danckaert <address@hidden> skribis:

> this patch continues the work done by Jeremy Robst to package HDF4
>
> (https://lists.gnu.org/archive/html/guix-devel/2016-06/msg00069.html)

Nice!

>  - Back then, people reported occasional build failures. I've disabled
>    parallel tests and don't have any problems on my system anymore
>    (maybe someone else can test?).

Will do.

>  - I've added a patch to allow a shared build with Fortran interface,
>    and some substitutions to remove bogus RPATH settings from the
>    Makefiles (otherwise, build directories are included in the RPATH).
>
>  - I've also added a variant “hdf4-alt”: by default, HDF4 includes a
>    netCDF API to access HDF4 files, which clashes with the symbols in
>    the real netCDF library when you try to use both libraries in your
>    program.  This variant uses the “--disable-netcdf” configure flag
>    to avoid this.  We could call it “hdf4-nonetcdf”, but the name
>    “hdf4-alt” is used by Debian, so maybe it's good to use the same?

Would “hdf4-minimal” sound appropriate here?  That’s a convention we use
in similar cases.  Otherwise “hdf4-alt” is fine with me.

> From dd7eca9a3cfec364a05aa0f52b941f09b4dea039 Mon Sep 17 00:00:00 2001
> From: Thomas Danckaert <address@hidden>
> Date: Wed, 28 Sep 2016 10:34:58 +0200
> Subject: [PATCH] gnu: Add hdf4
>
> * gnu/packages/maths.scm (hdf4, hdf4-alt): New variables.
> * gnu/packages/patches/hdf4-reproducibility.patch: New file.
> * gnu/packages/patches/hdf4-shared-fortran.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add patches.

Please credit Jeremy in a “Co-authored-by” tag.

I only have cosmetic comments:

> +             (substitute*
> +                 (map (lambda (dir) (string-append dir "/Makefile.in"))
> +                      '("hdf" "hdf/examples" "hdf/fortran" "hdf/src"
> +                        "hdf/test" "hdf/util" "mfhdf" "mfhdf/dumper"
> +                        "mfhdf/examples" "mfhdf/fortran" "mfhdf/hdfimport"
> +                        "mfhdf/hdiff" "mfhdf/hrepack" "mfhdf/libsrc"
> +                        "mfhdf/ncgen" "mfhdf/ncdump" "mfhdf/nctest"
> +                        "mfhdf/test" "mfhdf/xdr"))

Maybe simply (find-files "." "^Makefile\\.in$")?

> --- /dev/null
> +++ b/gnu/packages/patches/hdf4-reproducibility.patch
> @@ -0,0 +1,47 @@
> +Remove/patch unreproducible config data.
> +---
> + configure           | 9 ++++++++-
> + libhdf4.settings.in | 6 +++---
> + 2 files changed, 11 insertions(+), 4 deletions(-)
> +
> +diff --git a/configure b/configure
> +index eb9f346..ebab94d 100755
> +--- a/configure
> ++++ b/configure
> +@@ -23163,7 +23163,14 @@ H4_VERSION="`cut -d' ' -f3 $srcdir/README.txt | 
> head -1`"
> + 
> + 
> + ## Configuration date
> +- CONFIG_DATE="`date`"
> ++CONFIG_DATE="`date -u`"
> ++if test -n "$SOURCE_DATE_EPOCH"; then
> ++  CONFIG_DATE=`date -u -d "@$SOURCE_DATE_EPOCH" 2>/dev/null \
> ++               || date -u -r "$SOURCE_DATE_EPOCH" 2>/dev/null`
> ++  if test -z "$CONFIG_DATE"; then
> ++    as_fn_error $? "malformed SOURCE_DATE_EPOCH" "$LINENO" 5
> ++  fi
> ++fi

It’d be enough to just do:

- CONFIG_DATE="`date`"
+ CONFIG_DATE="$SOURCE_DATE_EPOCH"

The smaller the patch, the better.  :-)

Otherwise LGTM.  Could you send an updated patch?

Thank you!

Ludo’.